Changing coolDownPeriod will affect already queued withdrawals

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Withdraw/WithdrawQueue.sol#L287 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Withdraw/WithdrawQueue.sol#L129-L133

Vulnerability details

Bug description

Users that want to withdraw from Renzo can’t do it directly, and will be subject to a two-step process, where they will first need to create the withdrawal request by calling withdraw in the withdraw queue, and then actually obtain the tokens by triggering claim after waiting for a specified cooldown period:

File: WithdrawQueue.sol

function withdraw(uint256 _amount, address _assetOut) external nonReentrant {

        ...

        ...

        // add withdraw request for msg.sender
        withdrawRequests[msg.sender].push(
            WithdrawRequest( 
                _assetOut,
                withdrawRequestNonce,
                amountToRedeem,
                _amount,
                block.timestamp
            )
        );

function claim(uint256 withdrawRequestIndex) external nonReentrant {
          ...

        WithdrawRequest memory _withdrawRequest = withdrawRequests[msg.sender][
            withdrawRequestIndex
        ];
        if (block.timestamp - _withdrawRequest.createdAt < coolDownPeriod) revert EarlyClaim(); 

                ...

}

As we can see, when creating a withdrawal block.timestamp will be passed as the createdAt field in the WithdrawRequest struct. Then, _withdrawRequest.createdAt will be substracted from block.timestamp to see if the minimum coolDownPeriod has passed.

The problem with this approach is that if coolDownPeriod is changed, then it will affect the currently queued withdrawals. This should not be the case, as currently queued withdrawals should only be limited by the cooldown period set in the moment of withdrawal.

For example, a situation could arise where a user submits a withdrawal request and the cooldown period in that moment is set to 7 days. After 4 days (3 days remaining to be able to claim the assets), the cooldown period is changed to 14 days. This will affect the user that initially queued the withdrawal for 7 days, making its waiting period extend by an additional 7 days.

Impact

Medium. Users that initially queued withdrawals with a certain duration could end up waiting more time than the expected to withdraw their assets.

Recommended Mitigation

Consider storing the end timestamp in a cooldownFinish field instead of storing the current block.timestamp in the createdAt when creating the withdrawRequests struct, and checking against it when performing the claim, like the following:

File: WithdrawQueue.sol

function withdraw(uint256 _amount, address _assetOut) external nonReentrant {

        ...

        ...

        // add withdraw request for msg.sender
        withdrawRequests[msg.sender].push(
            WithdrawRequest( 
                _assetOut,
                withdrawRequestNonce,
                amountToRedeem,
                _amount,
-                block.timestamp
+                block.timestamp + cooldownPeriod
            )
        );

function claim(uint256 withdrawRequestIndex) external nonReentrant {
          ...

        WithdrawRequest memory _withdrawRequest = withdrawRequests[msg.sender][
            withdrawRequestIndex
        ];
-        if (block.timestamp - _withdrawRequest.createdAt < coolDownPeriod) revert EarlyClaim(); 
+        if (block.timestamp < _withdrawRequest.cooldownFinish) revert EarlyClaim(); 

                ...

}

Tools used

Manual review

Assessed type

Other

[c4-judge]

alcueca marked the issue as duplicate of #607

[c4-judge]

alcueca marked the issue as unsatisfactory: Invalid

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-b