eyexploit - Unbounded loop in getLockedFunds could lead to griefing/DOS attack when user try claiming refunds via refundDeposit

[github-actions]

eyexploit

high

Unbounded loop in getLockedFunds could lead to griefing/DOS attack when user try claiming refunds via refundDeposit

Summary

Griefing/DOS attack is possible when a malcious actor makes multiple fake deposits through fundBountyToken. As for, each new deposit its generating a new depositId which later it appends to the deposits for the record. So whenever a legitmate user try claiming his deposit via refundDeposit, it could cause excessive gas usage in getLockedFunds and transaction reverted due to block gas limit.

Vulnerability Detail

1. Consider the attacker make a deposits of 1 Wei of token 5000 times (say) through fundBountyToken(), which increase the deposits count by 5000.
2. A legitmate user try claiming for his deposits by calling refundDeposit in the same bounty contract above, where it first calculate the availableFunds using the total tokenBalance and locked tokenBalance.

uint256 availableFunds = bounty.getTokenBalance(depToken) - bounty.getLockedFunds(depToken);

3. getLockedFunds will revert due to step 1, as block gas limit reached.

    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;
    }

Hence, user funds got freezed into the contract and also no claims are refundable for any new deposits in future.

Impact

Permanent freezing of user funds and no claim on any future deposits.

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/DepositManager/Implementations/DepositManagerV1.sol#L54-L56

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

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/DepositManager/Implementations/DepositManagerV1.sol#L171-L172

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

Tool used

Manual Review

Recommendation

Currently attacker can deposit with same account address multiple times and for every deposit a new depositId is generated to kept that deposit records.

In BountyCore.sol for receiveFunds, We can make sure if funder has previously deposit, then there is no need to generate new depositId, just update it for the previous depositId and accordingly the expiration can be extended.

Duplicate of #77