sherlock-audit / 2023-02-openq-judging

4 stars 0 forks source link

joestakey - `refundDeposit` can be DOS #486

Closed github-actions[bot] closed 1 year ago

github-actions[bot] commented 1 year ago

joestakey

medium

refundDeposit can be DOS

Summary

Attackers can fill up the deposits array to DOS refunds

Vulnerability Detail

DepositManager.refundDeposit() computes the funds available by calling bounty.getLockedFunds

File:contracts/DepositManager/Implementations/DepositManagerV1.sol
171:         uint256 availableFunds = bounty.getTokenBalance(depToken) -
172:             bounty.getLockedFunds(depToken);

This function loops through the deposits array:

File: contracts/Bounty/Implementations/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++) { 

deposits grows on every deposit:

File: contracts/Bounty/Implementations/BountyCore.sol
54: deposits.push(depositId);

If attackers call DepositManager.fundBountyToken() enough times, the deposits array will be large enough to make getLockedFunds revert with an out-of-gas error

Impact

Funders cannot get refunds. The attack is cheap (low gas on Polygon + minimum deposit is 1 wei), making it extremely likely to happen.

Code Snippet

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

Tool used

Manual Review

Recommendation

You can either add an upper limit to deposits, or add a minimum deposit amount to make the attack too expensive.

Duplicate of #77