code-423n4 / 2023-06-stader-findings

1 stars 1 forks source link

Insufficient Slippage Control in UserWithdrawalManager's ETH Withdrawal #403

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#L117-L159 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#L135 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#L155

Vulnerability details

medium

Links:

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#L117-L159 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#L135 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#L155

Title: Insufficient Slippage Control in UserWithdrawalManager's ETH Withdrawal

Impact

Users experience significant slippage during ETH withdrawals from the UserWithdrawalManager.
When a user requests a withdrawal of their ETH, they transfer their ethX tokens to the UserWithdrawalManager,
with the transferred amount recorded as ethXAmount. Subsequently, when the user finalizes the withdrawal, the lockedEthX tokens are burned, and the amount of ETH transferred to the user is calculated based on the current exchange rate.

However, if there is a drastic change in the exchange rate between ETHX and ETH between the withdrawal request and the finalization, users will receive significantly less ETH than expected.
This lack of slippage control exposes users to potential losses in the amount of ETH they receive.

Proof of concept

The first user initiates the withdrawal process by calling the requestWithdraw() function of the UserWithdrawalManager contract. This function transfers the user's ETHx tokens to the UserWithdrawalManager.

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#L95

The transferred amount is saved in the userWithdrawRequests data structure,
which keeps track of withdrawal requests.

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#L104

After the user's timelock period expires, the withdrawal request is finalized.
This occurs when the finalizeUserWithdrawalRequest() function is invoked.
During finalization, the ETH amount to be transferred is calculated based on the current exchange rate, which is obtained.

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#L136

If there has been a drastic change in the exchange rate between the time of the withdrawal request and the finalization, the user will receive significantly fewer tokens than originally expected.
This discrepancy arises due to the lack of slippage control, potentially resulting in a notable loss for the user.

Tools Used

VS Code

Recommended Mitigation Steps

add the minEthAmount received parameter to make sure the receiver does not receive x amount of ETH, transaction revert

Assessed type

Math

c4-judge commented 1 year ago

Picodes marked the issue as primary issue

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #222

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)