Renzo - 14si2o_Flint's results

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L274-L358 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Withdraw/WithdrawQueue.sol#L202-L263

Vulnerability details

Impact

The calculateTVLs() function in RestakeManager incorrectly calculates the TVL due to incorrect mixing of indexes in a nested loop. As a result, the amount to redeem calculated in the WithdrawQueue:: withdraw function will not be correct.

In the calculateTVLs()function, there is a nested loop with i for operatorDelegator and j for collateralToken. In the loop that counts the balance of the collateral tokens, the ERC20 token is index with i while the balance is indexed with j. This means that instead of looping through all the collateral tokens, the balance of each collateral token is counted as the balance of i collateral token.

Since the different collateral tokens are not equal in value and the different operatorDelegators will have different balances for each respective collateral token, the TVL calculated will be completely wrong.

Proof of Concept

Assume: 4 operator delegators and 3 collateral tokens odLength = [0,1,2,3] tokenLength = [token1,token2,token3]

In the below function we first have a for loop that cycles through the operators with the i as index. In this loop we find a nested loop which tries to add the value of all the collateral tokens to the tvl. It uses j as index.

Yet in this second loop both indexes are mixed. Which twists the calculations.

When i = 0, it tries to loop through the 3 collateral tokens to add the value to the TVL.

first loop: j = 0 : collateralTokens[0], collateralTokens[0].balanceOf() 2nd loop: j = 1 : collateralTokens[0], collateralTokens[1].balanceOf() 3rd loop: j = 2 : collateralTokens[0], collateralTokens[2].balanceOf()

So for the 1st operator, we get 0 for the 2nd & 3rd collateral token, while vastly overestimating the balance of the 1st collateral token. So for the 2nd operator, we get 0 for the 1st & 3rd collateral token, while vastly overestimating the balance of the 2nd collateral token. So for the 3rd operator, we get 0 for the 1st & 2nd collateral token, while vastly overestimating the balance of the 3rd collateral token.

In RestakeManager :: calculateTVLs()

                if (!withdrawQueueTokenBalanceRecorded) { 
                    totalWithdrawalQueueValue += renzoOracle.lookupTokenValue(
@>                    collateralTokens[i], 
                        collateralTokens[j].balanceOf(withdrawQueue)
                    );
                }

Since the different collateral tokens are not equal in value and the different operatorDelegators will have different balances for each respective collateral token, the TVL calculated will be completely wrong.

The impact is a definite loss of protocol users and/or the protocol, since the function is used to calculate the amount to redeem in the withdraw().

Tools Used

Manual Review

Recommended Mitigation Steps

                if (!withdrawQueueTokenBalanceRecorded) { 
                    totalWithdrawalQueueValue += renzoOracle.lookupTokenValue(
-                        collateralTokens[i], 
+                        collateralTokens[j], 
                        collateralTokens[j].balanceOf(withdrawQueue)
                    );
                }

Assessed type

Loop

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L491-L576 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Delegation/OperatorDelegator.sol#L140-L154

Vulnerability details

Impact

When a user calls RestakeManager :: deposit, the function first checks the if there is a buffer to be filled and subtracts the bufferToFill from the deposit amount.

If bufferToFill >= _amount, amount is set to 0 which means the operatorDelegator.deposit() will be called with 0 for _amount. This is a major problem since that function has a require that revert on zero deposit.

As a result, whenever a buffer needs to be filled, all deposits smaller than the buffer will revert.

Proof of Concept

Assume Alice deposits 5 eth in restakeManager. The buffer to fill is 6 eth. _amount = 5 eth bufferToFill = 6 eth

From the below code if statements we get: bufferToFill > 0 _amount <= bufferToFill ? Yes => bufferToFill = _amount _amount -= bufferTofill => setting _amount to 0

As a result the operatorDelegator.deposit(_collateralToken, _amount) function will be called with 0 for _amount.

        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
            depositQueue.fillERC20withdrawBuffer(address(_collateralToken),  bufferToFill);
        }
        // Approve the tokens to the operator delegator
        _collateralToken.safeApprove(address(operatorDelegator), _amount);
        // @audit-info approve the reduced amount (can be 0)

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

In the deposit function we see a clear if statement that will cause a revert whenever tokenamount = 0.

    function deposit(
        IERC20 token,
        uint256 tokenAmount
    ) external nonReentrant onlyRestakeManager returns (uint256 shares) {
        if (address(tokenStrategyMapping[token]) == address(0x0) || tokenAmount == 0) 
            revert InvalidZeroInput();

        // Move the tokens into this contract
        token.safeTransferFrom(msg.sender, address(this), tokenAmount);

        return _deposit(token, tokenAmount);
    }

As a result, whenever the deposit is smaller than the buffer to be refilled, the deposit will revert and the user cannot interact with the protocol.

Tools Used

Manual Review

Recommended Mitigation Steps

Either remove the 0 amount deposit require from the OperatorDelegator contract or change the buffer logic so that _amount will never be zero.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L244-L259 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L274-L358 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Withdraw/WithdrawQueue.sol#L202-L263

Vulnerability details

Impact

The removeCollateralToken function breaks the calculateTVLs function since collateralTokens.length is used in the calculateTVLs function to determine the balance of the collateral tokens. If the removed collateral token is not the last token in the collateralTokens array before removal, the TVL calculation will use the removed collateral balance while not counting active collateral balance.

As a result, the TVL calculation will be wrong and the amount protocol users will be able to withdraw will be completely incorrect.

Proof of Concept

Assume: collateralTokens = [token1,token2,token3,token4] collateralTokens.length = 4 OperatorDelegator Alice with balances for each token. [100,10,27,66]

Admin calls removeCollateralToken for token3.
collateralTokens = [token1,token2,token4] collateralTokens.length = 3 OperatorDelegator Alice still have the same balances. [100,10,27,66]

The calculateTVLs function is used to to calculate the TVL based on collateralTokens.length. TVL = 0 collateralTokens.length = 3 Loop 1: TVL+= 100 Loop 2: TVL+= 10 Loop 3: TVL+= 27

TVL = 137 The balance of the removed collateral token3 is counted as valid TVL while the active token4 collateral token is not counted.

Tools Used

Manual Review

Recommended Mitigation Steps

Before a collateral token is removed, all operatorDelegators must be required to completely empty their balance of said token. Only then should the function be called. This would require substantial additional logic to correctly implement this.

Assessed type

Loop