XKET - An attacker can prevent claimers from claiming for atomic and tiered percentage bounties

[github-actions]

XKET

high

An attacker can prevent claimers from claiming for atomic and tiered percentage bounties

Summary

An attacker can prevent claimers from claiming for atomic and tiered percentage bounties so claimers can't claim their payouts.

Vulnerability Detail

For atomic bounty and tiered percentage bounty, the claim manager claims payouts for all deposited tokens at the same time.

Atomic bounty (ClaimManagerV1.sol#L130-L134):

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

Tiered percentage bounty (ClaimManagerV1.sol#L230-L235):

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

So if one of those claims reverts, it will prevent the whole claim process. Some ERC20 tokens revert when they transfer 0 amount, and one of these tokens can be whitelisted.
Even if none of these tokens is whitelisted, an attacker can use any of them if tokenAddressLimitReached is false. (DepositManagerV1.sol#L45-L50)

        if (!isWhitelisted(_tokenAddress)) {
            require(
                !tokenAddressLimitReached(_bountyAddress),
                Errors.TOO_MANY_TOKEN_ADDRESSES
            );
        }

Let us call the ERC20 token tokenR0 for simplicity.

The attacker deposits some amount of the tokenR0 with tiny expiration, tokenR0 will added to tokenAddresses of the bounty. (BountyCore.sol#L21-L58)

21    function receiveFunds(
        ...
55        tokenAddresses.add(_tokenAddress);

If he refunds the same amount of tokenR0 after expiration, tokenR0 still exists in _bounty.getTokenAddresses() because tokenAddresses is only added and there is no removal logic in BountyCore.refundDeposit. And tokenR0 balance of the bounty becomes 0.

When claimers want to claim their payouts after that, _bounty.claimBalance or _bounty.claimTiered will revert because it tries to transfer 0 amount of tokenR0. (AtomicBountyV1.sol#L96-L97)

        uint256 claimedBalance = getTokenBalance(_tokenAddress);
        _transferToken(_tokenAddress, claimedBalance, _payoutAddress);

This will prevent the whole claiming process and the claimers can't claim their payouts.

Impact

Claimers can't claim their payouts due to an attack.

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/ClaimManager/Implementations/ClaimManagerV1.sol#L130-L134
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/ClaimManager/Implementations/ClaimManagerV1.sol#L230-L235
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/DepositManager/Implementations/DepositManagerV1.sol#L45-L50
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L21-L58
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/AtomicBountyV1.sol#L96-L97
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L227

Tool used

Manual Review

Recommendation

Transfer tokens only when volume > 0. (BountyCore.sol#L227)

-           token.safeTransfer(_payoutAddress, _volume);
+       if(_volume > 0) {
+           token.safeTransfer(_payoutAddress, _volume);
+       }

Duplicate of #62