Liquid Staking

Summary

In the _finalizeWithdrawals() function of the WithdrawalPool contract, the assert statement is used to check that all the shares to withdraw are fully accounted for after processing queued withdrawals. While the intent of using assert() is to enforce this critical condition, its use may not be appropriate in this context. In line with industry standards, assert() should only be used for internal consistency checks that are never supposed to fail unless there is a bug in the code, whereas require() is more suitable for validating user inputs and external interactions. Misusing assert() can lead to fund losses for users as assert() does not refund the remaning amount of gas resulting in users paying much more than needed as well as to improper error handling in production environments.

Vulnerability Details

The specific usage of assert() occurs at the end of the function:

This assertion ensures that the sharesToWithdraw variable has been fully decremented during the iteration over the queued withdrawals. If this condition is not met, it would indicate a serious issue in the logic of the function, as the entire withdrawal amount should be accounted for. However, the use of assert() instead of require() poses several risks:

1. Gas Refund Behavior: If the assert() fails, the transaction will revert without returning any remaining gas to the caller, as described in Solidity’s documentation. In contrast, require() would return the remaining gas, leading to a more graceful failure in case of an error.

2. Misuse of assert(): As discussed in the following resources:

   • CodeForgeek: assert vs require in Solidity

   • Explanation of assert vs require

The general guideline is that assert() should only be used for checking invariants that should never fail under any circumstance unless a bug exists in the contract. On the other hand, require() is more appropriate for validating conditions that could fail because of invalid inputs, unexpected external state, or business logic validation. In this case, checking whether all shares are accounted for may be more appropriate for require(), as failure could occur due to external factors or business logic errors.

Impact

Financial loss for users as well as to incorrect error handling because if the assert() fails, it will indicate an internal inconsistency and terminate execution without an explanatory message, which could make debugging difficult.

Tools Used

Manual code review

Recommendations

Replace assert() with require(): The final check should use require() instead of assert() to ensure that the condition is met without unnecessarily consuming all the gas. Additionally, provide a clear error message to explain why the condition failed: