jkoppel - Refunds can be disabled by spamming deposits

[github-actions]

jkoppel

high

Refunds can be disabled by spamming deposits

Summary

By submitting a large number of deposits to a bounty, anyone can make the deposits array grow to a massive size, causing an out-of-gas error any time someone tries to refund.

This works even if all such deposits are refunded, meaning that 2 blocks notice the attack can be carried out for just the cost of gas.

Vulnerability Detail

DepositManager.refundDeposit calls BountyCore.getLockedFunds, which loops over the entire deposits array. This array grows on every call to DepositManager.fundBountyToken, which can be called by anyone. The array never shrinks. An OUT_OF_GAS error thereby makes it impossible to withdraw.

For atomic and tiered bounties, this can be worked around: assuming they have the ability to instruct the oracle to award everything to themselves, the issuer can retrieve deposits using the claim functionality. However, for ongoing bounties, this is not possible, because it is only possible to claim funds from an OngoingBounty if it is still open. A griefer can front-run a call to OngoingBountyV1.closeOngoing() with a large number of deposits of 1e-18 tokens each. All funds will then be locked in the bounty, permanently.

Note that the attack still works if the griefer refunds all their deposits, up until the last deposit that makes deposits impossible.

Note further that spam deposits of any whitelisted token will block refunds for all tokens, including NFTs.

An issuer can also do this themselves, making it impossible for other depositors to retrieve their funds. This is of interest if there would otherwise be some mechanism in the oracle to prevent an issuer from stealing funds from anyone who funds a bounty.

Impact

This can be done on any bounty, making it impossible to refund deposits.

For ongoing bounties, or when combined with other attacks that disable claims such as https://github.com/sherlock-audit/2023-02-openq-jkoppel/issues/6 or https://github.com/sherlock-audit/2023-02-openq-jkoppel/issues/8, the funds may be lost forever.

Code Snippet

Here is BountyCore.getLockedFunds, https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L333

    function getLockedFunds(address _depositId)
        public
        view
        virtual
        returns (uint256)
    {
        uint256 lockedFunds;
        bytes32[] memory depList = this.getDeposits();
        for (uint256 i = 0; i < depList.length; i++) {
            if (
                block.timestamp <
                depositTime[depList[i]] + expiration[depList[i]] &&
                tokenAddress[depList[i]] == _depositId
            ) {                                      
                lockedFunds += volume[depList[i]];
            }
        }

        return lockedFunds;
    }

This is a loop over an array of unbounded length.

Note also that this.getDeposits() performs a full copy of the array into memory, making it easier to hit the gas limit.

The vulnerability was confirmed by modifying the DepositManager test "should transfer refunded deposit volume from bounty contract to funder" to include the following snippet

				for (var i = 0; i < 10000; i++) {
					await depositManager.fundBountyToken(bountyAddress, mockLink.address, 1, 1, Constants.funderUuid);
				}

This caused the refundDeposit function to return an error is getLockedFunds as expected.

Tool used

Manual Review

Recommendation

Either prevent non-issuers from depositing into a bounty, or replace getLockedFunds() with a lockedFunds variable. Consider adding a markExpired(depositId) function to update the lockedFunds variable when a deposit expires, so that it can be kept up-to-date without any need for looping over deposits.

Alternatively, put a cap on the number of deposits, similar to the NFT cap.

Duplicate of #77