sherlock-audit / 2024-08-winnables-raffles-judging

6 stars 2 forks source link

Straight Hotpink Wolverine - Incorrect Balance Comparison in withdrawTokens Function #641

Closed sherlock-admin4 closed 3 months ago

sherlock-admin4 commented 3 months ago

Straight Hotpink Wolverine

Low/Info

Incorrect Balance Comparison in withdrawTokens Function

Summary

The withdrawTokens function in the smart contract contains a logic flaw that can prevent the successful withdrawal of ERC20 tokens by the admin. The function is designed to allow the admin to withdraw tokens from the contract, but the flaw in the balance comparison could lead to unintended reverts.

Vulnerability Detail

WinnablesTicketManager.sol contract

The vulnerability lies in the following line:

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);
    }

if (amount < balance) revert InsufficientBalance();

This line incorrectly checks if the amount to be withdrawn is less than the balance of the contract. This is contrary to the intended logic, where the function should check if the amount exceeds the available balance. As a result, the function reverts when the amount is actually less than the balance, preventing legitimate withdrawals.

Impact

Due to this flaw, the admin will be unable to withdraw tokens if the amount specified is less than the available balance, which is an illogical restriction. This could hinder the proper management of the contract's assets and create unnecessary confusion or operational delays.

Code Snippet

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

Tool used

Manual Review

Recommendation

The comparison logic should be corrected by replacing the flawed line with: if (amount > balance) revert InsufficientBalance();

This will ensure that the withdrawTokens function only reverts when the withdrawal amount is greater than the contract’s token balance, aligning with the intended functionality and allowing legitimate withdrawals to proceed.

sherlock-admin2 commented 3 months ago

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