yixxas - refundDeposit() can reach the out-of-gas state

[github-actions]

yixxas

high

refundDeposit() can reach the out-of-gas state

Summary

refundDeposit() can be DOSed. It calls getLockedFunds() which loops through all deposits that are made to the bounty. However, because depositing is permissionless and number of deposit is unbounded, refunds cannot be made if too many deposits are made.

Vulnerability Detail

refundDeposit() calls getLockedFunds() to calculate availableFunds for max amount of deposits that can be withdrawn.

function refundDeposit(address _bountyAddress, bytes32 _depositId)
	external
	onlyProxy
{
	IBounty bounty = IBounty(payable(_bountyAddress));

	require(
		bounty.funder(_depositId) == msg.sender,
		Errors.CALLER_NOT_FUNDER
	);

	require(
		block.timestamp >=
			bounty.depositTime(_depositId) + bounty.expiration(_depositId),
		Errors.PREMATURE_REFUND_REQUEST
	);

	address depToken = bounty.tokenAddress(_depositId);

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

	uint256 volume;
	if (bounty.volume(_depositId) <= availableFunds) {
		volume = bounty.volume(_depositId);
	} else {
		volume = availableFunds;
	}

	bounty.refundDeposit(_depositId, msg.sender, volume);

	emit DepositRefunded(
		_depositId,
		bounty.bountyId(),
		_bountyAddress,
		bounty.organization(),
		block.timestamp,
		bounty.tokenAddress(_depositId),
		volume,
		0,
		new bytes(0),
		VERSION_1
	);
}

In getLockedFunds(), we go through depList, which is all the deposits that has been made to the bounty. This list is unbounded as there is no limit on the number of deposits that can be funded. Having a list that is too large will cause refunds to revert, even if expiration has been 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;
}

Impact

Depositors may not be able to refund deposits even if expiration has been made. In fact, a malicious user can deposit dust amounts to force the DOS state.

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L333-L352
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/DepositManager/Implementations/DepositManagerV1.sol#L152-L195

Tool used

Manual Review

Recommendation

Consider adding a maximum limit on number of deposits that can be made for bounty, as well as enforcing a minimum amount that bounty accepts as deposit. Currently, we accept any amount > 0.

Duplicate of #77