sherlock-audit / 2023-04-footium-judging

13 stars 7 forks source link

PokemonAuditSimulator - The transfer of ERC20 prizes may fail without reverting, resulting in the funds becoming locked #386

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

PokemonAuditSimulator

medium

The transfer of ERC20 prizes may fail without reverting, resulting in the funds becoming locked

Summary

Some ERC20 tokens do not revert when a transaction fails, resulting in users' funds being locked and inaccessible to them.

Vulnerability Detail

The claimERC20Prize() function can have unpredictable behavior or cause fund loss because some ERC20 tokens do not revert or return 0 upon transfer failure.

The claimERC20Prize() function does not include a check to ensure that the token transfer was successful. This means that if a transfer fails, the ERC20 tokens will still be marked as claimed from the PrizeDistributor contract, even though the user did not receive them. For more detailed information, please refer to the following material: https://github.com/d-xo/weird-erc20#no-revert-on-failure.

Impact

This causes the funds of user to be locked and made inaccessible to them.

Code Snippet

[FootiumPrizeDistributor/L106-L134]

FootiumPrizeDistributor.sol

function claimERC20Prize(
        address _to,
        IERC20Upgradeable _token,
        uint256 _amount,
        bytes32[] calldata _proof
    ) external whenNotPaused nonReentrant {
        if (_to != msg.sender) {
            revert InvalidAccount();
        }

        if (
            !MerkleProofUpgradeable.verify(
                _proof,
                erc20MerkleRoot,
                keccak256(abi.encodePacked(_token, _to, _amount))
            )
        ) {
            revert InvalidERC20MerkleProof();
        }

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

        if (value > 0) {
            totalERC20Claimed[_token][_to] += value;

        // @audit The following line can fail without reverting | will lead to locked funds!
            _token.transfer(_to, value);
        }

        emit ClaimERC20(_token, _to, value);
    }

Tool used

Manual Review

Recommendation

Consider modifying the specified line to mitigate the potential vulnerability. [FootiumPrizeDistributor/L130] to:

require(_token.transfer(_to, value), "Token transfer failed.");

Implementing this change will effectively eliminate the potential vulnerability.

Duplicate of #86