Inconsistent check for LP balance in AMO

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L256-L259 https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L600-L604 https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L632-L636

Vulnerability details

Inconsistent check for LP balance in AMO

While pulling LP tokens from the CVXStaker contract, the AMO queries the current available balance using the staked balance, which is inconsistent with the implementation of the withdraw function.

Impact

Curve LP tokens owned by the AMO contract are staked in a Convex pool that is handled using the CVXStaker contract. When liquidity needs to be removed from the Curve pool, the AMO contract needs to first withdraw the LP tokens from the CVXStaker contract.

The rebalanceUp(), removeLiquidity() and removeLiquidityOnlyStETH() functions present in the AMO contract deal with removing liquidity from the Curve pool. In their implementations, all of them query the available LP balance using the stakedBalance() function of CVXStaker. Taking the rebalanceUp() function as an example (other cases are similar), we can see the following:

https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L256-L261

...
256:         uint256 amoLpBal = cvxStaker.stakedBalance();
257: 
258:         // if (amoLpBal == 0 || quote.lpBurn > amoLpBal) revert LpBalanceTooLow();
259:         if (quote.lpBurn > amoLpBal) revert LpBalanceTooLow();
260: 
261:         cvxStaker.withdrawAndUnwrap(quote.lpBurn, false, address(this));
...

The implementation of stakedBalance() basically delegates the call to fetch the staked balance in the Convex reward pool contract:

https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L204-L206

204:     function stakedBalance() public view returns (uint256 balance) {
205:         balance = IBaseRewardPool(cvxPoolInfo.rewards).balanceOf(address(this));
206:     }

However, this check is not correct. As we can see in the implementation of withdrawAndUnwrap(), the CVXStaker contract consider not only staked tokens, but also available balance held in the contract itself:

https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L142-L161

142:     function withdrawAndUnwrap(
143:         uint256 amount,
144:         bool claim,
145:         address to
146:     ) external onlyOperatorOrOwner {
147:         // Optimistically use CLP balance in this contract, and then try and unstake any remaining
148:         uint256 clpBalance = clpToken.balanceOf(address(this));
149:         uint256 toUnstake = (amount < clpBalance) ? 0 : amount - clpBalance;
150:         if (toUnstake > 0) {
151:             IBaseRewardPool(cvxPoolInfo.rewards).withdrawAndUnwrap(
152:                 toUnstake,
153:                 claim
154:             );
155:         }
156: 
157:         if (to != address(0)) {
158:             // unwrapped amount is 1 to 1
159:             clpToken.safeTransfer(to, amount);
160:         }
161:     }

Line 148 considers potentially available LP tokens in the contract, and withdraws the remaining amount from Convex.

This means that checking against stakedBalance() is too restrictive and incorrect, and can potentially lead to situations in which the required LP tokens are enough, but the check in line 259 of the AMO contract will revert the operation.

Proof of concept

As an example, let's take a call to the rebalanceUp() function and assume that the quoted lpBurn amount is 4. The CVXStaker contract has 3 LP tokens staked in Convex and 2 tokens held as balance in the contract.

In this situation, the condition quote.lpBurn > amoLpBal will be true, as amoLpBal = cvxStaker.stakedBalance() = 3, which evaluates the condition to 5 > 3, causing a revert in the transaction.

However, the operation would succeed if the check weren't there, as withdrawAndUnwrap() will first consider the 2 tokens already present in the CVXStaker contract and withdraw the remaining 2 from the Convex pool, successfully fulfilling the requested amount.

Recommendation

The validation to ensure available LP tokens in rebalanceUp(), removeLiquidity() and removeLiquidityOnlyStETH() should not only consider stakedBalance() but also available LP tokens present in the CVXStaker contract. Alternatively, the check can be removed as the call to withdrawAndUnwrap() will eventually fail if the available tokens are not enough.

Assessed type

Invalid Validation

[c4-judge]

kirk-baird marked the issue as primary issue

[c4-judge]

kirk-baird marked the issue as selected for report

[c4-sponsor]

vaporkane marked the issue as sponsor confirmed