Renzo - baz1ka's results

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Delegation/OperatorDelegator.sol#L329

Vulnerability details

Impact

Incorrect calculation of TVL can lead to minting/burning incorrect amount of shares to user.

Proof of Concept

OperatorDelegator.getTokenBalanceFromStrategy() function is supposed to return

underlying token amount from the amount of shares + queued withdrawal shares

Condition whether to return just underlying token amount from the amount of shares or underlying token amount from the amount of shares with queued withdrawal shares is incorrect:

    /// @dev Gets the underlying token amount from the amount of shares + queued withdrawal shares
    function getTokenBalanceFromStrategy(IERC20 token) external view returns (uint256) {
        return
@>          queuedShares[address(this)] == 0
                ? tokenStrategyMapping[token].userUnderlyingView(address(this))
                : tokenStrategyMapping[token].userUnderlyingView(address(this)) +
                    tokenStrategyMapping[token].sharesToUnderlyingView(
                        queuedShares[address(token)]
                    );
    }

In fact, it checks if queuedShares mapping has address of this contract (OperatorDelegator). This is wrong and doesn't make any sense as OperatorDelegator is not an ERC20 token and can not be deposited into strategy.

Recommended Mitigation Steps

Correctly check whether OperatorDelegator has queued shares for token or not:

@@ -326,7 +335,7 @@ contract OperatorDelegator is
    /// @dev Gets the underlying token amount from the amount of shares + queued withdrawal shares
    function getTokenBalanceFromStrategy(IERC20 token) external view returns (uint256) {
        return
-           queuedShares[address(this)] == 0
+           queuedShares[address(token)] == 0
                ? tokenStrategyMapping[token].userUnderlyingView(address(this))
                : tokenStrategyMapping[token].userUnderlyingView(address(this)) +
                    tokenStrategyMapping[token].sharesToUnderlyingView(

Assessed type

Other

Lines of code

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

Vulnerability details

Summary

RestakeManager.calculateTVLs() calculates incorrect amount of TVL.

Impact

Calculating totalWithdrawalQueueValue for incorrect token can lead to different TVL than it's supposed to be which may result in getting different amount of shares. Can be can be negative for the user (if price of collateralTokens[i] is greater than collateralTokens[j]) or can be negatve for the protocol (if price of collateralTokens[i] is smaller than collateralTokens[j]).

Proof of Concept

Values returned from RestakeManager.calculateTVLs() areused in taking calculation of how many shares would be minted/burned to user. However, when this function calculates value of tokens (in ETH) in WithdrawQueue it gets incorrect address of token to get value for.

    // record token value of withdraw queue
    if (!withdrawQueueTokenBalanceRecorded) {
        totalWithdrawalQueueValue += renzoOracle.lookupTokenValue(
@>          collateralTokens[i],
            collateralTokens[j].balanceOf(withdrawQueue)
        );
    }

Instead of getting value of token in current loop iteration, it gets token value of incorrect collateralTokens[i]. In this case index i is always 0 because it's index from outer loop. That means it calculates token value of first token with amount of token it should have been calculated. RestakeManager.calculateTVLs() is used in RestakeManager.deposit(), RestakeManager.depositETH(), RestakeManager.depositTokenRewardsFromProtocol(), BalancerRateProvider.getRate(), WithdrawQueue.withdraw() which are main fund flow functions and they are dependent on correct return value from RestakeManager.calculateTVLs().

Recommended Mitigation Steps

Get collateral token from array by correct index:

@@ -315,7 +315,7 @@ contract RestakeManager is Initializable, ReentrancyGuardUpgradeable, RestakeMan
    // record token value of withdraw queue
    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#L543-L546 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L546-L549 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L562

Vulnerability details

Impact

User can't deposit ERC20 collateral token into the RestakeManager if WithdrawQueue isn't filled with necessary (buffer) amount of tokens. This deficit can occur when user that already has deposits withdraw their funds and withdrawal buffer deficit is not filled in time.

Proof of Concept

To deposit ERC20 tokens into RestakeManager user must call RestakeManager.deposit() which makes next actions:

• Check if WithdrawQueue buffer needs to be filled:

    uint256 bufferToFill = depositQueue.withdrawQueue().getBufferDeficit(
        address(_collateralToken)
    );
    if (bufferToFill > 0) {

• If bufferToFill > 0 then conditional code executes to fill withdraw buffer deficit:

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

• In case _amount <= bufferToFill bufferToFill would be equal to _amount.
• On next line bufferToFill (which is in our case equal to _amount) is subtracted from deposited _amount and in our case, it would be 0.
• Then this _amount which is 0 is deposited to OperatorDelegator contract:

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

But this execution would revert as OperatorDelegator.deposit() doesn't accept _amount of 0:

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

Recommended Mitigation Steps

Deposit into operatorDelegator only if _amount is more than 0:

@@ -558,8 +558,15 @@ contract RestakeManager is Initializable, ReentrancyGuardUpgradeable, RestakeMan
        // Approve the tokens to the operator delegator
        _collateralToken.safeApprove(address(operatorDelegator), _amount);

        // Call deposit on the operator delegator
-       operatorDelegator.deposit(_collateralToken, _amount);
+       if (_amount != 0) {
+           operatorDelegator.deposit(_collateralToken, _amount);
+       }

Assessed type

Invalid Validation