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

2 stars 2 forks source link

During deposit if buffer deficit is larger than deposit amount transactions will revert. #722

Closed c4-bot-7 closed 5 months ago

c4-bot-7 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Delegation/OperatorDelegator.sol#L143 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L543 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L547 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Delegation/OperatorDelegator.sol#L148

Vulnerability details

Impact

During a deposit the restaking manager first tries to fill the buffer deficit in the withdrawal queue. The remaining amount is forwarded to the operator delegator. However, if the buffer deficit is larger than the deposited amount, the remianing amount for the operator delegator will be 0 and the transaction will revert because the operator delegator does not accept 0 amount deposits.

As a result of this user transactions will revert unexpectedly whenever an amount smaller than the buffer deficit is being deposited.

Proof of Concept

First notice that operator delegator deposit function reverts if the input deposit tokenAmount is 0:

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Delegation/OperatorDelegator.sol#L143

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

Now, suppose a user wants to deposit an amount say 1e18 and there is a buffer deficit in the withdrawal queue of 2e18 tokens. The restaking manager checks how much the withdrawal queue buffer deficit is, and first tries to fill it up before forwarding the amount to the operator delegator:

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

    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
        depositQueue.fillERC20withdrawBuffer(address(_collateralToken), bufferToFill);
    }

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

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

So, with the bufferToFill being at 2e18 and _amount at 1e18, the lines:

        bufferToFill = (_amount <= bufferToFill) ? _amount : bufferToFill;
        // update amount to send to the operator Delegator
        _amount -= bufferToFill;

Will set the bufferToFill to 1e18 and _amount to 0.

So when the amount is deposited in the operator delegator, it will be passed in as 0, and since the deposit function in the OperatorDelegator contract reverts if the amount is 0, the transaction will fail.

Tools Used

Manual Review

Recommended Mitigation Steps

Only call deposit on the operator delegator if the remaining amount is larger than 0.

Assessed type

Invalid Validation

DadeKuma commented 5 months ago

@howlbot accept