cuthalion0x - FootiumPrizeDistributor may "claim" ERC20 without actually disbursing

[sherlock-admin]

cuthalion0x

medium

FootiumPrizeDistributor may "claim" ERC20 without actually disbursing

Summary

In the FootiumPrizeDistributor contract, it is possible for an ERC20 token transfer to fail while the total claim amount is increased. The user will be unable to submit another claim to get the tokens out, even if the transfer might succeed on the second attempt.

Vulnerability Detail

The Footium contest description says that all ERC20 tokens are in scope. Some tokens, notably older tokens, do not revert on failed transfers; they merely return false. Token transfers can fail for myriad reasons, not limited to incorrect parametrization. The token contract could be temporarily paused, for example, so that it is not possible to transfer now but it will be in the future.

In FootiumPrizeDistributor.claimERC20Prize, the return value of the outbound token transfer is unchecked. As a result, it is possible for non-standard ERC20 token transfers to fail without reverting the claim transaction. The totalERC20Claimed will still be increased to reflect a claim that did not actually happen because the user did not receive tokens.

Subsequently, the user will be unable to claim the tokens because the value computation will underflow on _amount - totalERC20Claimed[_token][_to]. If the tokens are to be later retrieved (assuming the second attempted transfer does not fail), the contract's owner will have to set a new merkle root to claim them to a different address.

Impact

Some non-standard ERC20 tokens may be irretrievable from the FootiumPrizeDistributor if the first attempted transfer fails. The owner may still be able to get the tokens out by setting a new merkle root to claim them to a different address.

Code Snippet

https://github.com/sherlock-audit/2023-04-footium/blob/11736f3f7f7efa88cb99ee98b04b85a46621347c/footium-eth-shareable/contracts/FootiumPrizeDistributor.sol#L126-L131

        uint256 value = _amount - totalERC20Claimed[_token][_to];

        if (value > 0) {
            totalERC20Claimed[_token][_to] += value;
            _token.transfer(_to, value); // @audit Return value unchecked.
        }

Tool used

Manual Review

Recommendation

Consider using OpenZeppelin's SafeERC20 library to deal with non-standard tokens.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/8b2ed0f5706fe1a7ab6ade84b27bf875dc611fda/contracts/token/ERC20/utils/SafeERC20.sol#L22-L28

    /**
     * @dev Transfer `value` amount of `token` from the calling contract to `to`. If `token` returns no value,
     * non-reverting calls are assumed to be successful.
     */
    function safeTransfer(IERC20 token, address to, uint256 value) internal {
        _callOptionalReturn(token, abi.encodeWithSelector(token.transfer.selector, to, value));
    }

Duplicate of #86