Renzo - Shaheen's results

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L491 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L592

Vulnerability details

The deposit() & depositETH functions of the RestakeManager contract, enables users to deposit assets into the protocol, getting ezETH tokens in return. The function doesn't have any type of slippage control; this is relevant in the context of the deposit function, since the amount of tokens received by the user is determined by an interaction with an oracle, meaning that the amount received in return may vary indefinitely while the request is waiting to be executed.

Also the users will have no defense against price manipulations attacks, if they where to be found after the protocol's deployment.

Proof of Concept

As can be observed by looking at it's parameters and implementation, the deposit function of the RestakeManager contract, doesn't have any type of slippage protection:

    function deposit(IERC20 _collateralToken, uint256 _amount, uint256 _referralId) public nonReentrant notPaused {
        // Verify collateral token is in the list - call will revert if not found
        uint256 tokenIndex = getCollateralTokenIndex(_collateralToken);

        // Get the TVLs for each operator delegator and the total TVL
        (uint256[][] memory operatorDelegatorTokenTVLs, uint256[] memory operatorDelegatorTVLs, uint256 totalTVL) = calculateTVLs();

        // Get the value of the collateral token being deposited
        uint256 collateralTokenValue = renzoOracle.lookupTokenValue(_collateralToken, _amount);

        .....

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

        // Transfer the collateral token to this address
        _collateralToken.safeTransferFrom(msg.sender, address(this), _amount);

        // Check the withdraw buffer and fill if below buffer target
        uint256 bufferToFill = depositQueue.withdrawQueue().getBufferDeficit(address(_collateralToken));
        if (bufferToFill > 0) {
            bufferToFill = (_amount <= bufferToFill) ? _amount : bufferToFill;
            // update amount to send to the operator Delegator
            _amount -= bufferToFill;

            // safe Approve for depositQueue
            _collateralToken.safeApprove(address(depositQueue), bufferToFill);

            // fill Withdraw Buffer via depositQueue
            ///@audit-issue M 3 transfers happening this will be problematic for stETH, maybe this will never work with stETH and stETH pools remain empty 
            depositQueue.fillERC20withdrawBuffer(address(_collateralToken), bufferToFill);
        }
        ///@audit-ok isn't this is wrong because if(bufferToFill>0) then bufferToFill tokens are already transferred to depositQueue - all good because amount is getting reduced `_amount -= bufferToFill`
        ///> maybe putting this inside a if() statement will be good, I mean safeApprove & deposit should only run when `amount > 0`
        ///@audit-issue M if() is important here otherwise when amount will be reduced to zero, this whole trx will revert due to the tokenAmount zero check present in the OD's deposit! `InvalidZeroInput`
        // Approve the tokens to the operator delegator
        _collateralToken.safeApprove(address(operatorDelegator), _amount);

        // Call deposit on the operator delegator
        operatorDelegator.deposit(_collateralToken, _amount);

        ///@audit-ok calculating mint amount independent of the asset type? - lookupTokenValue gives collateralTokenValue based on the asset type
        // Calculate how much ezETH to mint
        uint256 ezETHToMint = renzoOracle.calculateMintAmount(
            totalTVL,
            collateralTokenValue,
            ezETH.totalSupply()
        );
        ///@audit-issue M slippage check missing!
        // Mint the ezETH
        ezETH.mint(msg.sender, ezETHToMint);

        // Emit the deposit event
       
 emit Deposit(msg.sender, _collateralToken, _amount, ezETHToMint, _referralId);
    }

Impact

Users have no control over how many ezETH tokens they will get in return for depositing in the contract.

Tools Used

Shaheen's Vision

Recommended Mitigation Steps

And additional parameter minAmount could be added to the deposit function, to let users decide the minimum amount of tokens to be received, with a relative check after minting.

-    function deposit(IERC20 _collateralToken, uint256 _amount, uint256 _referralId) public nonReentrant notPaused {
+    function deposit(IERC20 _collateralToken, uint256 _amount, uint256 _referralId, uint256 minAmt) public nonReentrant notPaused {
        // Verify collateral token is in the list - call will revert if not found
        uint256 tokenIndex = getCollateralTokenIndex(_collateralToken);

        // Get the TVLs for each operator delegator and the total TVL
        (uint256[][] memory operatorDelegatorTokenTVLs, uint256[] memory operatorDelegatorTVLs, uint256 totalTVL) = calculateTVLs();

        // Get the value of the collateral token being deposited
        uint256 collateralTokenValue = renzoOracle.lookupTokenValue(_collateralToken, _amount);

        .....
        
        // Calculate how much ezETH to mint
        uint256 ezETHToMint = renzoOracle.calculateMintAmount(
            totalTVL,
            collateralTokenValue,
            ezETH.totalSupply()
        );
        // Mint the ezETH
+       require(ezETHToMint >= minAmt, "Slippage Protection");
        ezETH.mint(msg.sender, ezETHToMint);

        // Emit the deposit event
        emit Deposit(msg.sender, _collateralToken, _amount, ezETHToMint, _referralId);
    }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L549 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L562

Vulnerability details

RestakeManager.deposit() function first checks, if there is withdraw buffer to be filled or not, if there is then it first fills that buffer and then forwards the tokens to get deposited into the EigenLayer Strategy.

As you can see, the _amount gets transferred from the user to the contract. Then getBufferDeficit() is called to get the bufferToFill amount. If the bufferToFill is greater than zero then that amount is sent to the depositQueue (actually withdrawQueue) using fillERC20withdrawBuffer function. And the _amount gets subtracted. Then the updated _amount is deposited:

    function deposit(IERC20 _collateralToken, uint256 _amount,uint256 _referralId) public nonReentrant notPaused {
        ...
        _collateralToken.safeTransferFrom(msg.sender, address(this), _amount);
        uint256 bufferToFill = depositQueue.withdrawQueue().getBufferDeficit(address(_collateralToken));
        if (bufferToFill > 0) {
            bufferToFill = (_amount <= bufferToFill) ? _amount : bufferToFill;
@>           _amount -= bufferToFill;
            _collateralToken.safeApprove(address(depositQueue), bufferToFill);
            depositQueue.fillERC20withdrawBuffer(address(_collateralToken), bufferToFill);
        }
        _collateralToken.safeApprove(address(operatorDelegator), _amount);
@>       operatorDelegator.deposit(_collateralToken, _amount);
        ...
    }

The problem is that when the _amount will be less than the bufferToFill amount then after subtraction _amount will became Zero. So, OperatorDelegator.deposit() will revert InvalidZeroInput Error, which will revert all the trxs.

(According to the setup.ts, the buffer will be set to 10_000 ethers. It's not confirmed what will be the buffer value will be for collateral tokens but it will be big enough to support the withdrawals. The point I'm trying to raise is that, the bufferToFill amount will be huge for normal users)

Impact

Broken Functionality - Users are not gonna be able to deposit tokens less than the bufferToFill amount

Tools Used

Shaheen's Eyes

Recommended Mitigation Steps

Simple make sure that deposit() only call OD.deposit(), if _amount is greater than zero:

    function deposit(IERC20 _collateralToken, uint256 _amount,uint256 _referralId) public nonReentrant notPaused {
        ...
        _collateralToken.safeTransferFrom(msg.sender, address(this), _amount);
        uint256 bufferToFill = depositQueue.withdrawQueue().getBufferDeficit(address(_collateralToken));
        if (bufferToFill > 0) {
            bufferToFill = (_amount <= bufferToFill) ? _amount : bufferToFill;
            _amount -= bufferToFill;
            _collateralToken.safeApprove(address(depositQueue), bufferToFill);
            depositQueue.fillERC20withdrawBuffer(address(_collateralToken), bufferToFill);
        }
+       if(_amount > 0){
            _collateralToken.safeApprove(address(operatorDelegator), _amount);
            operatorDelegator.deposit(_collateralToken, _amount);
+       }
        ...
    }

Assessed type

Other