ctf_sec - Developer cannot claim the bounty if the token revert in 0 amount transfer after the user get the bounty refund after funding the bounty contract

[github-actions]

ctf_sec

high

Developer cannot claim the bounty if the token revert in 0 amount transfer after the user get the bounty refund after funding the bounty contract

Summary

Developer cannot claim the bounty if the token revert in 0 amount transfer after the user get the bounty refund after funding the bounty contract

Vulnerability Detail

If user A fund the bounty contract using 50 token A, user B fund the bounty contract using 100 token B.

User A then refund the deposit and remove the 50 token A, the developer cannot claim the bounty if the token A reverts in 0 amount transfer.

According to https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers

• Some tokens (e.g. LEND
  ) revert when transfering a zero value amount

When fund the token via DepositManager.sol, the function below is called:

(bytes32 depositId, uint256 volumeReceived) = bounty.receiveFunds{
    value: msg.value
}(msg.sender, _tokenAddress, _volume, _expiration);

which calls:

function receiveFunds(
      address _funder,
      address _tokenAddress,
      uint256 _volume,
      uint256 _expiration
  )
      external
      payable
      virtual
      onlyDepositManager
      nonReentrant
      returns (bytes32, uint256)
  {
      require(_volume != 0, Errors.ZERO_VOLUME_SENT);
      require(_expiration > 0, Errors.EXPIRATION_NOT_GREATER_THAN_ZERO);
      require(status == OpenQDefinitions.OPEN, Errors.CONTRACT_IS_CLOSED);

      bytes32 depositId = _generateDepositId();

      uint256 volumeReceived;
      if (_tokenAddress == address(0)) {
          volumeReceived = msg.value;
      } else {
          volumeReceived = _receiveERC20(_tokenAddress, _funder, _volume);
      }

      funder[depositId] = _funder;
      tokenAddress[depositId] = _tokenAddress;
      volume[depositId] = volumeReceived;
      depositTime[depositId] = block.timestamp;
      expiration[depositId] = _expiration;
      isNFT[depositId] = false;

      deposits.push(depositId);
      tokenAddresses.add(_tokenAddress);

      return (depositId, volumeReceived);
  }

When the bounty is funded, we push the depositId and tokenAddress into the deposits and tokenAddresses.

deposits.push(depositId);
tokenAddresses.add(_tokenAddress)

However, when the user call refundDeposit, the deposits array and tokenAddresses map is not cleaned up.

In Atomic bounty, when the claim function is caleld via the claimManager.sol

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]
      );

which calls:

uint256 volume = _bounty.claimBalance(
    _closer,
    _bounty.getTokenAddresses()[i]
);

which calls:

/// @notice Transfers full balance of _tokenAddress from bounty to _payoutAddress
/// @param _tokenAddress ERC20 token address or Zero Address for protocol token
/// @param _payoutAddress The destination address for the funds
function claimBalance(address _payoutAddress, address _tokenAddress)
    external
    onlyClaimManager
    nonReentrant
    returns (uint256)
{
    uint256 claimedBalance = getTokenBalance(_tokenAddress);
    _transferToken(_tokenAddress, claimedBalance, _payoutAddress);
    return claimedBalance;
}

If the claimedBalance is 0, calling _transferToken would revert in 0 amount transfer.

First we need to edit the MockLink Contract:

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Mocks/MockLink.sol#L9

contract MockLink is ERC20 {

    address public admin;

    constructor() ERC20('Mock Link', 'mLINK') {
        _mint(msg.sender, 10000 * 10**18);
        admin = msg.sender;
    }

    function mint(uint256 amount) external {
        _mint(msg.sender, amount * 10 ** 18);
    }

    function _beforeTokenTransfer(address from, address to, uint256 amount)
        internal
        override
    { 
        require(amount > 0, "zero amount transfer not allowed!");
        super._beforeTokenTransfer(from, to, amount);
    }

}

Then we add the POC test below:

https://github.com/sherlock-audit/2023-02-openq/blob/main/test/ClaimManager.test.js#L253

describe.only('ATOMIC POC', () => {
	it('if token A does not allow 0 amount transfer, claim bounty failed if user get refund after supplying bounty token', async () => {
		// ARRANGE

		const [ user1, user2, user3 ] = await ethers.getSigners();
		
		await openQProxy.mintBounty(Constants.bountyId, Constants.organization, atomicBountyInitOperation);
		const bountyAddress = await openQProxy.bountyIdToAddress(Constants.bountyId);

		const volume = 100;
		const tokenDepositId = generateDepositId(Constants.bountyId, 0);
		
		await mockLink.connect(user1).approve(bountyAddress, 10000000);
		await depositManager.connect(user1).fundBountyToken(bountyAddress, mockLink.address, volume, 1, Constants.funderUuid);

		const expectedTimestamp = await setNextBlockTimestamp(2764900);
		await depositManager.connect(user1).refundDeposit(bountyAddress, tokenDepositId)

		// ASSUME
		let bountyIsClaimable = await claimManager.bountyIsClaimable(bountyAddress);
		expect(bountyIsClaimable).to.equal(true);

		await claimManager.connect(oracle).claimBounty(bountyAddress, user3.address, abiEncodedSingleCloserData);

		// ASSERT
		bountyIsClaimable = await claimManager.bountyIsClaimable(bountyAddress);
		expect(bountyIsClaimable).to.equal(false);
	});
});

We run the test using

yarn test

the output is:

  ClaimManager.sol
    bountyIsClaimable
      ATOMIC POC
        1) if token A does not allow 0 amount transfer, claim bounty failed if user get refund after supplying bounty token


  0 passing (2s)
  1 failing

  1) ClaimManager.sol
       bountyIsClaimable
         ATOMIC POC
           if token A does not allow 0 amount transfer, claim bounty failed if user get refund after supplying bounty token:
     Error: VM Exception while processing transaction: reverted with reason string 'zero amount transfer not allowed!'  

Impact

Developer cannot claim the bounty if the token revert in 0 amount transfer

Code Snippet

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

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

Tool used

Manual Review

Recommendation

We recommend the protocol clean up the deposits and check that if the token amount is larger than 0 before transfer the token.

Duplicate of #267