chainNue - Refund a Deposit will failed because of unbounded deposits array.

[github-actions]

chainNue

high

Refund a Deposit will failed because of unbounded deposits array.

Summary

Refund a deposit will failed because of unbounded deposit array.

Vulnerability Detail

OpenQ is permissionless, anyone can fund a contest. When someone deposit a fund to the bounty, they will call fundBountyToken in DepositManager then calling receiveFunds on the Bounty contract. This call will eventually increase the deposits array length.
(deposits.push(depositId);)

File: BountyCore.sol
21:     function receiveFunds(
22:         address _funder,
23:         address _tokenAddress,
24:         uint256 _volume,
25:         uint256 _expiration
26:     )
27:         external
28:         payable
29:         virtual
30:         onlyDepositManager
31:         nonReentrant
32:         returns (bytes32, uint256)
33:     {
...
38:         bytes32 depositId = _generateDepositId();
...
54:         deposits.push(depositId);
...
58:     }

Any malicious actor can just send a dust amount of valid (whitelisted) ERC20 token to increase this deposits array (for example 0.000001 USDC). Moreover, the attacker can just use the native coin (ETH) (by providing zero as token address) and send the msg.value to 0, (since the msg.value is not being checked if it's > 0), therefore the attack cost is really cheap. (ignoring the transaction fee).

If the deposits array length is large enough, at some point the getLockedFunds function will be reverted because out of gas, because a for loop with length is depList.length (deposits length).

this getLockedFunds is being called by refundDeposit function, so any refund will be failed.

Impact

User can't refund their deposit, locked on the contract, loss asset.

Code Snippet

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

File: DepositManagerV1.sol
152:     function refundDeposit(address _bountyAddress, bytes32 _depositId)
153:         external
154:         onlyProxy
155:     {
...
171:         uint256 availableFunds = bounty.getTokenBalance(depToken) -
172:             bounty.getLockedFunds(depToken);
...
195:     }


File: BountyCore.sol
333:     function getLockedFunds(address _depositId)
334:         public
335:         view
336:         virtual
337:         returns (uint256)
338:     {
339:         uint256 lockedFunds;
340:         bytes32[] memory depList = this.getDeposits();
341:         for (uint256 i = 0; i < depList.length; i++) {
342:             if (
343:                 block.timestamp <
344:                 depositTime[depList[i]] + expiration[depList[i]] &&
345:                 tokenAddress[depList[i]] == _depositId
346:             ) {
347:                 lockedFunds += volume[depList[i]];
348:             }
349:         }
350: 
351:         return lockedFunds;
352:     }
353: }

Tool used

Manual Review

Recommendation

To battle this issue, it's best to limit the deposits array length to some max length, just like NFT deposit but make it larger, for example 100 deposits or 500 deposits.
Moreover, prevent the deposit if the msg.value = 0 or at least set the minimum deposit value so any small / dust token amount should be rejected.

Duplicate of #77