Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

0xrobsol - Users may incorrectly believe their withdrawal succeeded when transfer fails due to gas limit constraints. #160

Open
sherlock-admin3 opened this issue Sep 21, 2024 · 0 comments

Comments

@sherlock-admin3
Copy link

sherlock-admin3 commented Sep 21, 2024

0xrobsol

Medium

Users may incorrectly believe their withdrawal succeeded when transfer fails due to gas limit constraints.

Summary

The withdrawAmountVariablePending function uses transfer to send ETH to the recipient, which is risky because it doesn't handle potential failures robustly. The transfer function imposes a 2,300 gas limit on the recipient, but there are multiple reasons why a transfer may fail beyond gas limitations. When transfer fails, the transaction reverts, but the contract does not explicitly catch the failure, which may cause the user to incorrectly believe the withdrawal succeeded. This issue is exacerbated as the function is called during the finalizeVaultOngoingVariableWithdrawals process, potentially leaving users in a state where they think they’ve withdrawn funds when, in fact, they haven’t.

Vulnerability Detail

The issue arises from using transfer, which forwards a fixed 2,300 gas to the recipient. This can cause failures due to gas limitations, missing fallback functions, reverting logic, self-destructed contracts, ETH-rejecting contracts, or insufficient balance. The critical problem is that the contract does not explicitly catch or handle the transfer failure, meaning if the transfer fails, users may think their withdrawal succeeded when, in fact, no ETH was transferred.

Impact

  • Failed Withdrawals: Users will believe their withdrawal succeeded because the contract logic does not explicitly catch and handle the failure of the transfer. This could lead to users losing access to their funds, resulting in user frustration and potential financial loss.

  • Potential Denial of Service: Certain users, particularly those using smart contract wallets or multisig wallets, may repeatedly experience failed withdrawals due to gas limitations or the lack of proper fallback functions, effectively preventing them from accessing their funds.

Code Snippet

https://github.com/sherlock-audit/2024-08-saffron-finance/blob/main/lido-fiv/contracts/LidoVault.sol#L656

Tool used

Manual Review

Recommendation

Replace the use of transfer with call, which provides more flexibility in gas handling and allows for better error handling. Additionally, implement proper error handling to ensure that if the transfer fails, the contract can catch the failure and revert the transaction in a user-friendly way.

@sherlock-admin4 sherlock-admin4 changed the title Clumsy Raisin Tarantula - Users may incorrectly believe their withdrawal succeeded when transfer fails due to gas limit constraints. 0xrobsol - Users may incorrectly believe their withdrawal succeeded when transfer fails due to gas limit constraints. Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant