xETH - Versus Contest - ronnyx2017's results

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:

1. 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);
    }

2. 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

Judge has assessed an item in Issue #19 as 2 risk. The relevant finding follows:

1. The first staker of the wxETH can get all the unlocked rewards immediately in the same block. code lines: https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L202-L216

For the first staker of the wxETH, the totalSupply of the wxETH is 0. So he can wrap the xETH to wxETH as 1:1.

function exchangeRate() public view returns (uint256) { /// @dev if there are no tokens minted, return the initial exchange rate uint256 _totalSupply = totalSupply(); if (_totalSupply == 0) { return INITIAL_EXCHANGE_RATE; } If the drip is started before the first staker, the unlocked funds can be withdrew immediately in the same block by calling unstake.

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L323-L325 https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L335-L351

Vulnerability details

Impact

The AMO2.rebalanceUp uses AMO2.bestRebalanceUpQuote function to avoid MEV attack when removing liquidity with only one coin. But the bestRebalanceUpQuote does not calculate the slippage correctly in this case, which is vulnerable to be attacked by MEV sandwich.

Proof of Concept

AMO2.rebalanceUp can be executed sucessfully only if the preRebalanceCheck returns true.

        bool isRebalanceUp = preRebalanceCheck();
        if (!isRebalanceUp) revert RebalanceUpNotAllowed();

So as the logic in the preRebalanceCheck, if isRebalanceUp = ture, the token balances in the curve should satisfy the following condition

xETHBal / (stETHBal + xETHBal) > REBALANCE_UP_THRESHOLD 

The default value of REBALANCE_UP_THRESHOLD is 0.75 :

uint256 public REBALANCE_UP_THRESHOLD = 0.75E18;

It's always greater than 0.5 . So when removing liquidity for only xETH, which is the rebalanceUp function actually doing, the slippage will be positive instead of negative. You will receive more than 1 uint of xETH when you burn the lp token of 1 virtual price.

But the slippage caculation is incorrect in the bestRebalanceUpQuote:

    // min received caculate in bestRebalanceUpQuote
        bestQuote.min_xETHReceived = applySlippage(
            (vp * defenderQuote.lpBurn) / BASE_UNIT
        );

    // applySlippage function
    function applySlippage(uint256 amount) internal view returns (uint256) {
        return (amount * (BASE_UNIT - maxSlippageBPS)) / BASE_UNIT;
    }

It uses the target amount subtracted slippage as the min token amount received. It is equivalent to expanding the slippage in the opposite direction. It makes the contractQuote can't protect the defenderQuote, increasing the risk of a large sandwich.

Tools Used

Manual review

Recommended Mitigation Steps

rebalanceUp and rebalanceDown should use different slippage calculation methods in the applySlippage.

Assessed type

MEV

Lines of code

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

Vulnerability details

Impact

Loss all the stEth and xEth lp tokens.

Proof of Concept

The CVXStaker.withdrawAllAndUnwrap can be called by the admin. And if the sendToOperator param is true, all the lp tokens of the CVXStaker contract (include lp tokens staked in the CVX and left in the CVXStaker itself ) will be withdrew to the operator, which is the AMO2 contract.

IBaseRewardPool(cvxPoolInfo.rewards).withdrawAllAndUnwrap(claim);
if (sendToOperator) {
    uint256 totalBalance = clpToken.balanceOf(address(this));
    clpToken.safeTransfer(operator, totalBalance);
}

There is not a function can manipulate ERC20 tokens directly in the AMO2 contract. But there are 6 functions can interact with Curve LP:

1 & 2 & 3: rebalanceUp & removeLiquidity & removeLiquidityOnlyStETH , these three functions have similar checks at the beginning:

        uint256 amoBalance = cvxStaker.stakedBalance();

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

Because all the lp tokens in the staker have been withdrew to the AMO2 itself, so amoBalance = 0 and these three functions will always revert.

4 & 5 & 6: rebalanceDown & addLiquidity & addLiquidityOnlyStETH, these three functions have similar code for adding liquidity:

        lpOut = curvePool.add_liquidity(amounts, minLpOut);
        IERC20(address(curvePool)).safeTransfer(address(cvxStaker), lpOut);
        cvxStaker.depositAndStake(lpOut);

The lp token amount finally sent out from the AMO2 contract is from the return value of the curvePool.add_liquidity function. So these three funtions can only mint and stake new lp tokens. They are unable to control the lp tokens stuck in the contract itself.

Tools Used

Manual review

Recommended Mitigation Steps

Add an external function to stake or unwrap(remove liquidity) the lp tokens in the contract.

Assessed type

DoS

Low issues

1. The first staker of the wxETH can get all the unlocked rewards immediately in the same block.

code lines: https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L202-L216

For the first staker of the wxETH, the totalSupply of the wxETH is 0. So he can wrap the xETH to wxETH as 1:1.

function exchangeRate() public view returns (uint256) {
    /// @dev if there are no tokens minted, return the initial exchange rate
    uint256 _totalSupply = totalSupply();
    if (_totalSupply == 0) {
        return INITIAL_EXCHANGE_RATE;
    }

If the drip is started before the first staker, the unlocked funds can be withdrew immediately in the same block by calling unstake.

2. Lack of initialization check about cvxPoolInfo in the CVXStaker

code lines: https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L126-L132

If the cvxPoolInfo has not been initialized before calling CVXStaker.depositAndStake, which means the cvxPoolInfo.pid = 0, some unexpected token transactions will be made.

Because the pid 0 is a valid and active pool in the CVX booster.

        if (!isCvxShutdown()) {
            clpToken.safeIncreaseAllowance(address(booster), amount);
            booster.deposit(cvxPoolInfo.pId, amount, true);
        }

3. CVXStaker.setCvxPoolInfo lacks address validation

code lines: https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L62-L72

In fact, the token and rewards address can be got by calling booster.poolInfo(cvxPoolInfo.pId). It ensures that the addresses are valid and consistent.

Non-critical issue

Conceptual error in document comments

code lines: https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L55-L61

CVXStaker.setCvxPoolInfo:

    /**
     * @dev Sets the CVX pool information.
     * @param _pId The pool ID of the CVX pool.
     * @param _token The address of the CLP token.
     * @param _rewards The address of the CVX reward pool.
     * Only the contract owner can call this function.
     */
    function setCvxPoolInfo(
        uint32 _pId,
        address _token,
        address _rewards
    )

The param _token should be the wrapped DepositToken of the CVX pool, instead of the LP token.

1. No need to set the lastReport in the wxETH.constructor

code lines: https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L73

Because the `dripEnabled` is false as default, the drip won't be started until the owner calls the startDrip function, which will set the drip lastReport to current block.

2. No need to call the drip modifier in the startDrip function.

code lines: https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L170-L174

If dripEnabled is true, the first line of the startDrip will revert directly:

function startDrip() external onlyOwner drip {
    /// @dev if the drip is already running, revert
    if (dripEnabled) revert DripAlreadyRunning();

So the dripEnabled must be false. If dripEnabled is false, the drip modifier will return directly without any changes:

function _accrueDrip() private {
    /// @dev if drip is disabled, no need to accrue
    if (!dripEnabled) return;

3. Unused variable in the storage

code lines: https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#LL68C36-L68C36

The global var cvxPoolInfo.token is never used in the CVXStaker contract.

4. CVXStaker.getReward should check if the reward balance is 0 before calling external erc20 transfer.

code lines: https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L191-L196

If the reward balance is 0, it's no need to call external erc20 transfer function.

5. Should check lockedFunds & dripRatePerBlock != 0 before startDrip

code lines: https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L222-L256

If lockedFunds is 0, dripEnabled will be set to false again at next drip.

If dripRatePerBlock is 0, drip can't unlock any funds but increaces gas consumption after startDrip.