ni8mare - `withdrawTokens` in `WinnablesTicketManager` is prone to DOS attacks

[sherlock-admin3]

ni8mare

High

withdrawTokens in WinnablesTicketManager is prone to DOS attacks

Summary

Due to an improper check in withdrawTokens, the function becomes prone to DOS attacks, preventing the admin from withdrawing tokens when they want to.

Vulnerability Detail

Look at the withdrawTokens function:

    function withdrawTokens(address tokenAddress, uint256 amount) external onlyRole(0) {
        IERC20 token = IERC20(tokenAddress);
        uint256 balance = token.balanceOf(address(this));
        if (amount < balance) revert InsufficientBalance();
        token.safeTransfer(msg.sender, amount);
    }

The check here - if (amount < balance) revert InsufficientBalance(); is incorrect. It should be (amount > balance). This prevents the admin from withdrawing the number of tokens they want. They will only ever be able to withdraw balance amount of tokens. This breaks the intended functionality.

In addition, the function is also prone to DOS attacks. When a user sees the withdrawTokens transaction in the mempool, where the admin is trying to withdraw amount==balance number of tokens(which is the only possible way to use it), a malicious actor would transfer 1 wei to the contract, such that the amount becomes less than the balance now. Hence, the function reverts. The actor can keep doing these attacks as long as they want (easily more than 1 week), as the cost is negligible (only 1 wei + gas cost on Avalanche), preventing the admin from withdrawing tokens.

Impact

Admin can be denied from withdrawing tokens.

Code Snippet

https://github.com/sherlock-audit/2024-08-winnables-raffles/blob/main/public-contracts/contracts/WinnablesTicketManager.sol#L292

Tool used

Manual Review

Recommendation

Use the correct check: if (amount > balance) revert InsufficientBalance();

[NishithPat]

Escalate

The admin will only be able to withdraw when amount == balance. Looking at just this might make this issue have a low severity. But, please read the last paragraph:

In addition, the function is also prone to DOS attacks. When a user sees the withdrawTokens transaction in the mempool, where the admin is trying to withdraw amount==balance number of tokens(which is the only possible way to use it), a malicious actor would transfer 1 wei to the contract, such that the amount becomes less than the balance now. Hence, the function reverts. The actor can keep doing these attacks as long as they want (easily more than 1 week), as the cost is negligible (only 1 wei + gas cost on Avalanche), preventing the admin from withdrawing tokens.

I am essentially talking about a front-running issue, which makes this a high-impact issue. A user can keep front-running the withdrawTokens transaction, by sending just 1 wei to the contract, thereby increasing the balance and making amount in withdrawTokens( which was sent with the intention of being equal to balance) less than balance, hence causing the function to revert.

A malicious user can easily do this for more than a week and keep DOSing this transaction, which by Sherlock's rules should make this a valid issue (DOS for more than a week). It's easy to DOS, as the cost of doing so is just the gas fees on avalanche + 1 wei of the ERC20 token.

This issue is different from other issues which just mention the incorrect comparison. The issue here talks about front-running and explains why it can have a much bigger impact.

[sherlock-admin3]

Escalate

The admin will only be able to withdraw when amount == balance. Looking at just this might make this issue have a low severity. But, please read the last paragraph:

In addition, the function is also prone to DOS attacks. When a user sees the withdrawTokens transaction in the mempool, where the admin is trying to withdraw amount==balance number of tokens(which is the only possible way to use it), a malicious actor would transfer 1 wei to the contract, such that the amount becomes less than the balance now. Hence, the function reverts. The actor can keep doing these attacks as long as they want (easily more than 1 week), as the cost is negligible (only 1 wei + gas cost on Avalanche), preventing the admin from withdrawing tokens.

I am essentially talking about a front-running issue, which makes this a high-impact issue. A user can keep front-running the withdrawTokens transaction, by sending just 1 wei to the contract, thereby increasing the balance and making amount in withdrawTokens( which was sent with the intention of being equal to balance) less than balance, hence causing the function to revert.

A malicious user can easily do this for more than a week and keep DOSing this transaction, which by Sherlock's rules should make this a valid issue (DOS for more than a week). It's easy to DOS, as the cost of doing so is just the gas fees on avalanche + 1 wei of the ERC20 token.

This issue is different from other issues which just mention the incorrect comparison. The issue here talks about front-running and explains why it can have a much bigger impact.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[debugging3]

183, #297, #432, #444, #485, #500,, #575 #582 is dup of this.

[MohammadArif13]

360 is dup of this

[boringslav]

145 is dup of this

[Parthmandale]

452 is dup of this

[dimi6oni]

571 is dup of this

[mystery0x]

A low finding at best:

1. There's a workaround to circumvent partial withdrawing.
2. If it mainly concerns LINK, just don't withdraw it. The protocol will be grateful for the generous donation by needing to send in less LINK to the contract to pay for CCIP Measages.

[NishithPat]

I request you again. Please go through the issue above and my comments on the escalation. This is different from those issues which simply mention the incorrect comparison.

There's a workaround to circumvent partial withdrawing.

What workaround? I don't see any function other than withdrawTokens that an admin can use to withdraw ERC20 tokens. If the workaround you are talking about is regarding amount == balance for token withdrawal, then this can easily be attacked by front-running as described above.

If it mainly concerns LINK, just don't withdraw it

https://github.com/sherlock-audit/2024-08-winnables-raffles-judging/issues/53 - For this issue, I can make the same argument. If you can't remove the roles, then just don't remove roles. The roles are trusted anyway. If an account with a role gets compromised, then it's the account's/user's mistake. An unreasonable argument, right? The same can be said for yours.

But, seriously. Let's say the protocol decides to not use these raffle contracts anymore. Then they would definitely want to withdraw their LINK tokens right? Plus, what about other tokens that are sent here for whatever reason? You should also be able to withdraw them.

The protocol will be grateful for the generous donation by needing to send in less LINK to the contract to pay for CCIP Measages.

Let's say that the protocol has sent 1 LINK token to WinnablesTicketManager to pay for CCIP and chainlink VRF. As LINK has 18 decimals, this means the WinnablesTicketManagercontract has 1000000000000000000 tokens (expressed in wei). The attacker sends 1 wei to carry this attack. The balance now becomes 1000000000000000001. Hmm... the protocol will be really, really, really grateful for this "generous" donation of "1 wei", right?

Apart from all this, the sponsor has also confirmed the issue. You may check their tag for the dups of this issue.

[mystery0x]

The most straight forward workaround is sending some tokens back to the contract after a full withdrawal atomically to make it a partial withdrawal.

Historically, issue pertaining to this optional function which many other protocols won't even care to include is deemed low. Note that it deals with ERC20 tokens accidentally sent to the contract, which is non-rewarded based on Sherlock rule.

[NishithPat]

This is clearly not how the function is meant to be used. Workarounds? Well, by that logic, there would not be any bugs in upgradeable contracts cause you can simply upgrade them.

[WangSecurity]

In this scenario on instance of the attack leads to a DOS of one block and the admin can withdraw tokens in the very next transaction. For this issue to have a larger impact the attack has to be repeated, which falls under this rule:

Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.

The function is not time-sensitive, therefore, planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• NishithPat: rejected

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/Winnables/public-contracts/pull/6