code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L256-L261 https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L599-L606 https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L631-L638 https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L204-L206

Vulnerability details

Impact

The lp tokens held by CVXStaker can't be able to used or withdrew by AMO2. Although the jam is not permanent and the owner of the CVXStaker can use recoverToken function to withdraw them, it will cause the functions about removing liquidity break down in some cases.

Proof of Concept

The functions about removing liquidity, rebalanceUp / removeLiquidity / removeLiquidityOnlyStETH , have similar codes for checking if AMO owns enough LP to burn:

        uint256 amoBalance = cvxStaker.stakedBalance();

        if (lpAmount > amoBalance) {
            revert LpBalanceTooLow();
        }

        cvxStaker.withdrawAndUnwrap(lpAmount, false, address(this));

And they will call the cvxStaker.withdrawAndUnwrap function to pull lp tokens after the check.

The LP balance of the AMO is from cvxStaker.stakedBalance():

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

It only returns the lp balance staked in the cvx reward pool. However, the lp tokens can be held by the CVXStaker itself in some cases, instead of staking in the CVX pool. This part of the lp tokens can be withdrew normally in the cvxStaker.withdrawAndUnwrap function:

    uint256 clpBalance = clpToken.balanceOf(address(this));
    uint256 toUnstake = (amount < clpBalance) ? 0 : amount - clpBalance;
    if (toUnstake > 0) {
        ...
    }

    if (to != address(0)) {
        // unwrapped amount is 1 to 1
        clpToken.safeTransfer(to, amount);
    }

But because the amoBalance loses this part of tokens in the check, the AMO2 can't withdraw this part of tokens from the CVXStaker.

There are some cases that the lp tokens can stay in the CVXStaker contract instead of sending to the CVX reward pool. For example:

The CVX pool is currently shutdown when the AMO is calling CVXStaker.depositAndStake:

AMO transfers the lp tokens to the cvxStaker first before calling depositAndStake:

    IERC20(address(curvePool)).safeTransfer(
        address(cvxStaker),
        lpAmountOut
    );
    cvxStaker.depositAndStake(lpAmountOut);

If CVX pool is currently shutdown, depositAndStake will not deposit the lp tokens received to the CVX reward pool:

    // Only deposit if the aura pool is open. Otherwise leave the CLP Token in this contract.
    if (!isCvxShutdown()) {
        clpToken.safeIncreaseAllowance(address(booster), amount);
        booster.deposit(cvxPoolInfo.pId, amount, true);
    }

The owner calls withdrawAndUnwrap or withdrawAllAndUnwrap without to address or sendToOperator flag:

function withdrawAndUnwrap(
    uint256 amount,
    bool claim,
    address to
) external onlyOperatorOrOwner {
    ...
    if (to != address(0)) {
        // unwrapped amount is 1 to 1
        clpToken.safeTransfer(to, amount);
    }
}

function withdrawAllAndUnwrap(
    bool claim,
    bool sendToOperator
) external onlyOwner {
    ...
    if (sendToOperator) {
        uint256 totalBalance = clpToken.balanceOf(address(this));
        clpToken.safeTransfer(operator, totalBalance);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

The lp balance of the AMO2 should be staked balance + lp balance of the CVXStaker .

Assessed type

Invalid Validation

c4-judge commented 1 year ago

kirk-baird marked the issue as duplicate of #33