Renzo - TheFabled's results

Lines of code

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

Vulnerability details

Description

In the calculateTVLs function of the RestakeManager.sol contract. The function miscalculates the Total Value Locked (TVL) for the withdrawal queue by incorrectly using the index of operatorDelegators (i) instead of the collateralTokens array index (j) when fetching the token address to calculate its value.

Impact

The impact of this issue is significant as it leads to an incorrect calculation of the total value in the withdrawal queue, potentially resulting in distorted TVL metrics for the protocol. This could cause inaccurate information to be presented to users and possibly affect decision-making processes related to staking, governance, and the general economics of the protocol.

Proof of Concept

The incorrect code snippet is as follows:

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

The correct code should reference the collateralTokens[j] for both the token address and its balance held by withdrawQueue as demonstrated below:

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

Tools Used

The issue was identified through manual code review and cross-referencing the logical flow of the contract functions.

Recommended Mitigation Steps

To mitigate this issue, the following steps are recommended:

• Correct the index from i to j when obtaining the token address within the lookupTokenValue function call.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Withdraw/WithdrawQueue.sol#L139

Vulnerability details

Description

The WithdrawQueue contract inherits from OpenZeppelin's PausableUpgradeable contract but does not properly implement the pause mechanism on its functions. The pause() and unpause() functions are present, allowing an authorized account (in this case, the withdraw queue admin) to toggle the paused state of the contract. However, the contract's core functionality does not utilize the whenNotPaused and whenPaused modifiers provided by PausableUpgradeable. This means that the contract operations such as withdrawing funds or filling the withdraw buffer may still be executed even when the contract is expected to be paused, rendering the pause feature ineffective for its intended emergency stop mechanism.

Impact

The main impact of this issue is that the admin cannot effectively pause the contract's sensitive operations in the event of an emergency, such as a detected vulnerability, ongoing attack, or other critical situations that may require halting operations to prevent potential loss of funds or further damage. This vulnerability exposes the contract to increased risks, as the emergency stop functionality cannot be relied upon when needed.

Proof of Concept

Within the WithdrawQueue contract, the pause and unpause functions simply toggle the state variable _paused through internal calls to _pause() and _unpause() without enforcing the actual paused state in any of the contract's critical functions.

However, we do not see the use of modifiers whenNotPaused or whenPaused in the provided code snippet of the WithdrawQueue contract. This suggests that the critical functions of the contract are not protected from being called while the contract is in a paused state.

Expected implementation:

function withdraw(uint256 _amount, address _assetOut) external nonReentrant whenNotPaused {
    // ...function logic
}

// Any other critical functions should have similar modifier checks

Since none of the critical functions use these modifiers, the contract can continue to operate as if it is not paused, which is a significant lapse in the security considerations for emergency situations.

Tools Used

Manual code review was the primary method used to identify this lack of pause functionality enforcement.

Recommended Mitigation Steps

To address this issue, it is recommended that:

1. The whenNotPaused modifier should be applied to all of the contract's functions that should not be executable while the contract is paused. This typically includes all state-changing functions that can move funds or alter critical data.

2. The whenPaused modifier should be used in functions designed to only function when the contract is paused if such functions exist.

Assessed type

Access Control