code-423n4 / 2024-04-renzo-findings

11 stars 8 forks source link

Sandwich attack when `sweepERC20` is called #385

Open howlbot-integration[bot] opened 5 months ago

howlbot-integration[bot] commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L274-L349

Vulnerability details

Impact

A sandwich attack on the sweepERC20 function could lead to the theft of a portion of ERC20 tokens accumulated in the DepositQueue contract.

Proof of Concept

Attack Method: By exploiting the sandwich attack, a portion of ERC20 token rewards can be stolen. This is because the accumulated ERC20 tokens in DepositQueue are not considered when calculating TVL (Total Value Locked). However, when sweepERC20 is invoked by the admin, these tokens are transferred to the operator delegator to be deposited into a strategy and affect the TVL calculation.

    function sweepERC20(IERC20 token) external onlyERC20RewardsAdmin {
        uint256 balance = IERC20(token).balanceOf(address(this));
        if (balance > 0) {
            uint256 feeAmount = 0;

            // Sweep fees if configured
            if (feeAddress != address(0x0) && feeBasisPoints > 0) {
                feeAmount = (balance * feeBasisPoints) / 10000;
                IERC20(token).safeTransfer(feeAddress, feeAmount);

                emit ProtocolFeesPaid(token, feeAmount, feeAddress);
            }

            // Approve and deposit the rewards
            token.approve(address(restakeManager), balance - feeAmount);
            restakeManager.depositTokenRewardsFromProtocol(token, balance - feeAmount);

            // Add to the total earned
            totalEarned[address(token)] = totalEarned[address(token)] + balance - feeAmount;

            // Emit the rewards event
            emit RewardsDeposited(IERC20(address(token)), balance - feeAmount);
        }
    }

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Deposits/DepositQueue.sol#L252-L277

    function depositTokenRewardsFromProtocol(
        IERC20 _token,
        uint256 _amount
    ) external onlyDepositQueue {
        // Get the TVLs for each operator delegator and the total TVL
        (, uint256[] memory operatorDelegatorTVLs, uint256 totalTVL) = calculateTVLs();

        // Determine which operator delegator to use
        IOperatorDelegator operatorDelegator = chooseOperatorDelegatorForDeposit(
            operatorDelegatorTVLs,
            totalTVL
        );

        // Transfer the tokens to this address
        _token.safeTransferFrom(msg.sender, address(this), _amount);

        // Approve the tokens to the operator delegator
        _token.safeApprove(address(operatorDelegator), _amount);

        // Deposit the tokens into EigenLayer
        operatorDelegator.deposit(_token, _amount);
    }

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L645-L668

TVL Calculation: The TVL calculation in the RestakeManager contract only considers the ETH balance of DepositQueue, not ERC20 tokens. After sweepERC20 is called, the ERC20 tokens are deposited into a strategy, impacting the TVL calculation.

function calculateTVLs() public view returns (uint256[][] memory, uint256[] memory, uint256) {
    //.....
     uint256 operatorBalance = operatorDelegators[i].getTokenBalanceFromStrategy(
        collateralTokens[j]
    );

    //......
    totalTVL += address(depositQueue).balance;
}

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L274-L349

Attack Scenario

  1. Attacker deposits A value of collateral token into the protocol, and he receives ezETHBalanceOfAttacker = A * totalSupplyOfEzETH / TVL as ezETH.
  2. Admin calls sweepERC20, causing the TVL to increase due to the accumulated ERC20 tokens. Suppose, the added value to TVL is B.
  3. Attacker requests to withdraw by transferring his ezETH balance. The value to be redeemed will be: ezETHBalanceOfAttacker * (TVL + A + B)/(totalSupplyOfEzETH + ezETHBalanceOfAttacker). By replacing ezETHBalanceOfAttacker with A * totalSupplyOfEzETH / TVL as explained in step 1, the value that the attacker will gain will be A * B / (TVL + A). This shows that A / (TVL + A) portion of B (which is accumulated ERC20 value as reward) is stolen by the attacker.

Tools Used

Recommended Mitigation Steps

There are two recomendations:

First: Include the ERC20 balance of DepositQueue into the TVL after deduction of fee amount.

    function calculateTVLs() public view returns (uint256[][] memory, uint256[] memory, uint256) {
        //...
            for (uint256 j = 0; j < tokenLength; ) {
                //...
                uint balance = collateralTokens[j].balanceOf(depositQueue);
                uint feeAmount;
                if (!depositQueueTokenBalanceRecorded && balance > 0) {
                    if (feeAddress != address(0x0) && feeBasisPoints > 0) {
                        feeAmount = (balance * feeBasisPoints) / 10000;
                    }
                    totalDepositQueueValue += renzoOracle.lookupTokenValue(
                        collateralTokens[j],
                        balance - feeAmount
                    );
                }
            }

        //...

        totalTVL += address(depositQueue).balance + totalDepositQueueValue;
    }

Second: Whenever TVL is to be calculated, it checks the ERC20 balance of DepositQueue. If it is nonzero, then automatically calls sweepERC20.

    function calculateTVLs() public view returns (uint256[][] memory, uint256[] memory, uint256) {
        //...
            for (uint256 j = 0; j < tokenLength; ) {
                //...
                uint balance = collateralTokens[j].balanceOf(depositQueue);
                if (!depositQueueTokenBalanceRecorded && balance > 0) {
                    depositQueue.sweepERC20(collateralTokens[j]);
                }
            }

        //...
    }

Assessed type

Context

c4-judge commented 4 months ago

alcueca marked the issue as satisfactory

c4-judge commented 4 months ago

alcueca changed the severity to QA (Quality Assurance)