sherlock-audit / 2024-02-smilee-finance-judging

2 stars 1 forks source link

cheatcode - Malicious User Could Block Other Users From Completing Regular Withdrawals. #154

Closed sherlock-admin4 closed 7 months ago

sherlock-admin4 commented 8 months ago

cheatcode

medium

Malicious User Could Block Other Users From Completing Regular Withdrawals.

Summary

In the completeWithdraw function, there is no explicit check to prevent a malicious user from repeatedly initiating withdrawals and never completing them. If a malicious user repeatedly calls initiateWithdraw and never completeWithdraw, it could potentially block the entire vault from progressing to the next epoch, effectively preventing all other users from completing their withdrawals.

Vulnerability Detail

https://github.com/sherlock-audit/2024-02-smilee-finance/blob/main/smilee-v2-contracts/src/Vault.sol#L523C5-L537C6

function completeWithdraw() external whenNotPaused {
    VaultLib.Withdrawal storage withdrawal = withdrawals[msg.sender];

    // Checks if there is an initiated withdrawal request
    if (withdrawal.shares == 0) {
        revert WithdrawNotInitiated();
    }

    // At least one epoch must have passed since the start of the withdrawal
    if (withdrawal.epoch == getEpoch().current) {
        revert WithdrawTooEarly();
    }

    _completeWithdraw();
}

Impact

DoS

Code Snippet

https://github.com/sherlock-audit/2024-02-smilee-finance/blob/main/smilee-v2-contracts/src/Vault.sol#L523C5-L537C6

Tool used

Manual Review

Recommendation

Introduce a limit on the maximum number of uncompleted withdrawals a user can have at any given time. This limit could be enforced in the initiateWithdraw function, preventing users from initiating new withdrawals if they have already reached the maximum allowed uncompleted withdrawals.

Another possible mitigation could be to introduce a timeout mechanism for uncompleted withdrawals. If a user fails to complete a withdrawal within a certain number of epochs, the initiated withdrawal could be automatically canceled, freeing up the vault to progress to the next epoch.

sherlock-admin3 commented 7 months ago

3 comment(s) were left on this issue during the judging contest.

panprog commented:

invalid, initiateWithdrawal doesn't impact any other users

tsvetanovv commented:

There is a check in the function that checks if there is already a withdrawal

takarez commented:

invalid