GimelSec - Invalid expiration blocks users from refunding, and also causes abuses of bounty contracts

[github-actions]

GimelSec

high

Invalid expiration blocks users from refunding, and also causes abuses of bounty contracts

Summary

In the document, the expiration of the deposit should be a number of days, but the _expiration could be a number of seconds. The invalid _expiration could block users from refunding, and also causes abuses of bounty contracts.

Vulnerability Detail

In document Deposit Period Days:

Deposit Period Days is how long your funds will be locked in the smart contract.
After this number of days, you will be able to refund your deposit if it hasn't already been claimed by successful work completion.

It defines a number of days, but the _expiration parameter is a number of seconds. Any user could deposit their funds and refund them only after a second.

An example of an exploit scenario is that an attacker could maliciously increase the length of deposits array to become a large length (by repeatedly funding and refunding due to the cheap gas fee on Polygon), causing anyone being unable to refund.
Because refundDeposit() calls bounty.getLockedFunds(depToken) to calculate availableFunds, but a large length of deposits will trigger the gas limit DoS of the for loop, the transaction will be reverted in the for loop of getLockedFunds().

Impact

There are some scenario examples in this issue.

• Users could change their mind after a second to refund, which is different from the document that users should wait a number of days.
• An attacker could maliciously increase the length of deposits array to become a large length (by repeatedly funding and refunding due to the cheap gas fee on Polygon), causing anyone being unable to refund due to the gas limit DoS of bounty.getLockedFunds(depToken) in refundDeposit().
• Users could make a fake donation, and (frontrunning) refund their money back before someone wants to claim the fund.

Code Snippet

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

Tool used

Manual Review

Recommendation

Use _expiration * 86400 in fundBountyToken():

        (bytes32 depositId, uint256 volumeReceived) = bounty.receiveFunds{
            value: msg.value
        }(msg.sender, _tokenAddress, _volume, _expiration * 86400);

The same fix in extendDeposit() and fundBountyNFT().

Duplicate of #77

[FlacoJones]

We convert from days to seconds ahead of time anyways so that part is invalid.

It is true however that we need an upper bound on how many deposits people can make.

[IAm0x52]

Dupe of #77

[FlacoJones]

OpenQDev/OpenQ-Contracts#116