MED - Brahma-Fi - Curve miscalculations may cause user withdraws to fail

Bug Description

To pull funds from trade executors, in order to satisfy user withdrawls, initiateWithdraw() function is used. For Convex positions, the called _withdraw() method receives _amount argument which it must have available. If there is not enough cash in the contract, it trades the minimum LP tokens necessary for USDC:

 uint256 lpTokensToConvert = Math.min(
            _USDCValueInLpToken(usdcValueOfLpTokensToConvert),
            lpToken.balanceOf(address(this))
        );
        // if lp tokens are required to convert, then convert to usdc and update amountToWithdraw
        if (lpTokensToConvert > 0) {
            _convertLpTokenIntoUSDC(lpTokensToConvert);
        }

Also, if there's not enough LP tokens available they will be unstaked before this stage, with this calculation of unstake amount:

               uint256 amountToUnstake = usdcValueOfLpTokensToConvert -
                    lpTokenBalance;
                // unstake convex position partially
                // this is min between actual staked balance and calculated amount, to ensure overflow
                uint256 lpTokensToUnstake = Math.min(
                    _USDCValueInLpToken(amountToUnstake),
                    baseRewardPool.balanceOf(address(this))
                );

The bug is that the _USDCValueInLpToken used to convert USDC amount to LP token amount uses the Curve interface incorrectly:

return fraxPool.calc_token_amount([0, _value], true);

In fact, the bool parameter isDeposit should be set to false, to simulate amount of LP tokens needed to be sent to withdraw _value amount. When set to false, the function returns amount of LP received when sending _value.

This interface is used incorrectly to calculate both unstaked amount and LP redeem amount.

In all tests that I ran, the correct conversion returns a higher LP amount then the current implementation, which means the _withdraw function indeed does not withdraw enough money from the fraxPool (example in POC section).

Impact

Batch withdraws will likely fail because not enough funds were withdrawn from trade executors.

Risk Breakdown

Difficulty to Exploit: Easy

Weakness: API misuse

Recommendation

Change _USDCValueInLpToken function to : return fraxPool.calc_token_amount([0, _value], false); All users of this function require a withdraw simulation, so this is fine, but make sure to add forWithdraw in the function name so that a similar confusion won't occur in the future.

References

Curve docs: https://curve.readthedocs.io/exchange-lp-tokens.html

Proof of Concept

Checked on current frax pool state:

1. Vault currently has 1000 USDC in cash

2. Users request withdraw total of 11000 USDC

3. Keeper calls Frax trade executor's initiateWithdraw(10000USDC)

4. Executor current cash = 100 USDC

5. usdcValueOfLpTokensToConvert = 9900

6. _USDCValueInLpToken(9900) returns 9894.451298 (should return 9894.4514)

7. _convertLpTokenIntoUSDC(9894.451298) converts LPs and receives 9897.796962

8. withdrawFromExecutor(9900) does not succeed because executor has converted less than 9900 USDC worth of LP tokens.

9. Users cannot withdraw desired amount.

---

Brahma.Fi closed the report saying the funds are still accessible by ConvexTradeExecutor, which can call initiateWithdraw() again for any amount. However, without being aware of this bug, developers would have a hard time understanding the issue, as even the Withdraw(amount) event displays a wrong amount, and using the curve API incorrectly as described will always withdraw too much or too little from curve. It's also not part of keeper's process to repair incorrect calculations. Therefore, this report should have received a medium reward.