MyFDsYours - Possible DOS can lock user's funds

[github-actions]

MyFDsYours

high

Possible DOS can lock user's funds

Summary

Array state variables deposits is growing indefinitely, this might lead to expensive transactions and effectively denial of service for the user when calling refundDeposit() function, because this one is calling getLockedFunds() that requires iterations over the whole deposits array.

Vulnerability Detail

Attacker can call DepositManagerV1.fundBountyToken() with :

• _tokenAddress as 0x0 address
• set _volume to any value > 0
• send nothing (msg.value = 0)

This scenario will create a valid deposit with volume[depositId] = 0

Attackers can repeat this scenario indefinitely until deposits array length will be big enough to make reverting getLockedFunds (and by the way refundDeposit function) due to gas limit.

Impact

User will not be able to get refunded because refundDeposit function is not working proprely.

Code Snippet

receiveFunds (called by fundBountyToken) in BountyCore.sol#L40-L58

    uint256 volumeReceived;
    if (_tokenAddress == address(0)) {
        volumeReceived = msg.value;
    } else {
        volumeReceived = _receiveERC20(_tokenAddress, _funder, _volume);
    }

    funder[depositId] = _funder;
    tokenAddress[depositId] = _tokenAddress;
    volume[depositId] = volumeReceived;
    depositTime[depositId] = block.timestamp;
    expiration[depositId] = _expiration;
    isNFT[depositId] = false;

    deposits.push(depositId);
    tokenAddresses.add(_tokenAddress);

    return (depositId, volumeReceived);
}

getLockedFunds in BountyCore.sol#L333-L352

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

Tool used

Manual Review

Recommendation

• First : instead of checking volume in receiveFunds prefer checking volumeReceived.

diff --git a/contracts/Bounty/Implementations/BountyCore.sol b/contracts/Bounty/Implementations/BountyCore.sol
index 7de0deb..b1ab5f2 100755
--- a/contracts/Bounty/Implementations/BountyCore.sol
+++ b/contracts/Bounty/Implementations/BountyCore.sol
@@ -31,7 +31,6 @@ abstract contract BountyCore is BountyStorageCore {
         nonReentrant
         returns (bytes32, uint256)
     {
-        require(_volume != 0, Errors.ZERO_VOLUME_SENT);
         require(_expiration > 0, Errors.EXPIRATION_NOT_GREATER_THAN_ZERO);
         require(status == OpenQDefinitions.OPEN, Errors.CONTRACT_IS_CLOSED);
 
@@ -44,6 +43,8 @@ abstract contract BountyCore is BountyStorageCore {
             volumeReceived = _receiveERC20(_tokenAddress, _funder, _volume);
         }
 
+        require(volumeReceived != 0, Errors.ZERO_VOLUME_SENT);
+
         funder[depositId] = _funder;
         tokenAddress[depositId] = _tokenAddress;
         volume[depositId] = volumeReceived;

• Consider adding a whitelist of users that can add deposit, or in general simply permit to add a whitelist of users that can interact with DepositManagerV1.sol contract

Duplicate of #77