An attacker can fill the deposits array with a very large number of tiny deposits and DoS the DepositManagerV1.refundDeposit function which will always revert.

The refundDeposit function of the rely on the getLockedFunds function to calculate the availableFunds for a given token to refund: DepositManagerV1.sol#L171-L172

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

In BountyCore, getLockedFunds loops through all of the deposits ever made (this.getDeposits() returns deposits):

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

An attacker could fill the deposits array by calling fundBountyToken multiples time since there is no limit to how much deposit can be made. He could even calculate approximatively how much deposit he has to make before the refundDeposit function will DoS and get all his deposit back right before it is the case (assuming low expiration time). Therefore, he could fake being a legitimate participant of the protocol.

The refundDeposit function can be DoS by a malicious user at a potentially very low cost (multiple transactions cost) depending on the chain.

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

Manual Review

I'm not sure how to solve this problem perfectly. Adding a limit to how much deposits can be made could partially mitigate the problem but on the other hand would allow someone to DoS the ability to fund tokens or NFTs. A minimum amount of token for funding could be added to make the attack very expensive. Unfortunately, it would need to be added for each token and thus would only work for ongoing or tiered fixed bounties which have a fixed token address to fund.

PS: The argument of the getLockedFunds function is _depositId while it should be called _depToken since it's the deposit address.

Duplicate of #77