unforgiven - attacker can cause claims of the atomicBounty and TieredPercentageBounty to revert if one of the whitelisted tokens revert when transferring 0 amount

[github-actions]

unforgiven

high

attacker can cause claims of the atomicBounty and TieredPercentageBounty to revert if one of the whitelisted tokens revert when transferring 0 amount

Summary

Some ERC20 tokens revert when transfer called with 0 amount. function claimBalance() in AtomicBountyV1 and claimTiered() in TieredPercentageBountyV1 would try to transfer 0 amount to winner if token balance of the contract was zero. attacker can use this and cause any bounty of those types to be unclaimable by making claim calls to revert because in claim code would loop through deposit tokens and try to transfer win amount which would revert on the 0 transfer of that specific token.

Vulnerability Detail

Function claimBounty() in ClaimManagerV1 is used for claiming winner prize from Bounty contract. it calls appropriate claim method based on bounty type. the specific type bounty claim function would loop through bounty deposit tokens and call claim function of the bounty contract for that deposit token. for bounty types atomic and percentage tiered it calls claimBalance() and claimTiered().
This is _claimAtomicBounty() code which is called by claimBounty():

    function _claimAtomicBounty(
        IAtomicBounty _bounty,
        address _closer,
        bytes calldata _closerData
    ) internal {
        _eligibleToClaimAtomicBounty(_bounty, _closer);

        for (uint256 i = 0; i < _bounty.getTokenAddresses().length; i++) {
            uint256 volume = _bounty.claimBalance(
                _closer,
                _bounty.getTokenAddresses()[i]
            );
            ................
        }

        for (uint256 i = 0; i < _bounty.getNftDeposits().length; i++) {
            _bounty.claimNft(_closer, _bounty.nftDeposits(i));
            ................
        }
    }

As you can see it loops through bounty deposit tokens and calls bounty.claimBalance(). This is claimBalance() code in AtomicBountyV1 contract:

    function claimBalance(address _payoutAddress, address _tokenAddress)
        external
        onlyClaimManager
        nonReentrant
        returns (uint256)
    {
        uint256 claimedBalance = getTokenBalance(_tokenAddress);
        _transferToken(_tokenAddress, claimedBalance, _payoutAddress);
        return claimedBalance;
    }

As you can see it calls _transferToken() with contract current balance in that token and if contract token balance was 0 code still calls transfer method. This is _transferToken() and _transferERC20() code:

    function _transferToken(
        address _tokenAddress,
        uint256 _volume,
        address _payoutAddress
    ) internal virtual {
        if (_tokenAddress == address(0)) {
            _transferProtocolToken(_payoutAddress, _volume);
        } else {
            _transferERC20(_tokenAddress, _payoutAddress, _volume);
        }
    }

    function _transferERC20(
        address _tokenAddress,
        address _payoutAddress,
        uint256 _volume
    ) internal virtual {
        IERC20Upgradeable token = IERC20Upgradeable(_tokenAddress);
        token.safeTransfer(_payoutAddress, _volume);
    }

As you can see there is no check that transfer volume is not 0. the function claimTiered() in TieredPercentageBountyV1 contract is similar and it would try to transfer 0 amount too when contract token balance is zero. there are some ERC20 tokens that reverts when transfer called with 0 amount. if one of those tokens were in whitelists then attacker can use that token to prevent any claim in every bounty by depositing small amount and then withdrawing that revert-on-zero-transfer token and then in any claim call to that bounty, code would loop through bounty deposit tokens and try to transfer win amount but when it reaches the attacker deposit token, because balance is zero, code would try to transfer 0 amount and the whole transaction would revert.
these are the steps attacker need to perform:

1. Token1 is in the whitelist and also it reverts when transferring 0 amounts.
2. User1 creates a Bounty1 (atomicBounty) and deposits 100K USDC for 6 month.
3. after 3 month attacker deposits 1 amount of Token1 for 1 day to Bounty1. code would add Token1 to Bounty1 token list.
4. after 1 day attacker would refund his deposit and the balance of the Token1 in the contract would be zero.
5. after 2 month(5 month from bounty start time) User1 would merge PR and the winner of the bounty would be announced.
6. off-chain oracle would call claim function to transfer winner funds for Bounty1 but claim function would try to loop through deposit tokens and when it reaches the Token1 it would try to transfer 0 amount and the transfer call would revert and whole oracle's transaction would revert.
7. even so winner performed his job he couldn't receive his funds.

the issue for percentage tiered bounty is similar.

Impact

attacker can cause winners to not receive their prizes as claim function would always revert.

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/AtomicBountyV1.sol#L89-L98

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L181-L191

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L221-L228

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/TieredPercentageBountyV1.sol#L104-L120

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/ClaimManager/Implementations/ClaimManagerV1.sol#L130-L148

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/ClaimManager/Implementations/ClaimManagerV1.sol#L230-L236

Tool used

Manual Review

Recommendation

don't call token transfer when the amount is 0. this check can be in claim function or lover level functions like _transferERC20()

Duplicate of #267

[0xunforgiven]

Escalate for 51 USDC

This should be duplicate of #267

both reports(mine #249 and #267) are about whitelisted legit ERC20 tokens that revert when transferring 0 amounts. adding those ERC20 tokens to whitelist can give attacker opportunity to perform DOS on AtomicBounty and TieredPercentageBounty. code should not try to transfer 0 amount.

issue #62 is about malicious ERC20 tokens and my report is marked duplicate of #62 by mistake.

[sherlock-admin]

Escalate for 51 USDC

This should be duplicate of #267

both reports(mine #249 and #267) are about whitelisted legit ERC20 tokens that revert when transferring 0 amounts. adding those ERC20 tokens to whitelist can give attacker opportunity to perform DOS on AtomicBounty and TieredPercentageBounty. code should not try to transfer 0 amount.

issue #62 is about malicious ERC20 tokens and my report is marked duplicate of #62 by mistake.

You've created a valid escalation for 51 USDC!

To remove the escalation from consideration: Delete your comment.
To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

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

[hrishibhat]

Escalation accepted

Valid duplicate of #267

[sherlock-admin]

Escalation accepted

Valid duplicate of #267

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.