clems4ever - Number of deposits can increase infinitely and prevent any refund

[github-actions]

clems4ever

high

Number of deposits can increase infinitely and prevent any refund

Summary

Every time a deposit is being made, the deposit is appended to an array and this array is iterated over when computing the locked funds for eventually computing the refundable volume. This could lead to a revert of any refund transaction if the number of deposits is too high because the array would have too many item to iterate over leading to too high gas costs.

Vulnerability Detail

In order to compute the volume to be refunded in the following snippet, the contract computes the locked funds by checking the expiry date of each deposit.

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

And the iteration is done here

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

However, anyone could just call fundBoutyToken as many times as needed to grow the array enough for the iteration over the deposits to revert

function fundBountyToken(
        address _bountyAddress,
        address _tokenAddress,
        uint256 _volume,
        uint256 _expiration,
        string memory funderUuid
    ) external payable onlyProxy {
        IBounty bounty = IBounty(payable(_bountyAddress));

        if (!isWhitelisted(_tokenAddress)) {
            require(
                !tokenAddressLimitReached(_bountyAddress),
                Errors.TOO_MANY_TOKEN_ADDRESSES
            );
        }

        require(bountyIsOpen(_bountyAddress), Errors.CONTRACT_ALREADY_CLOSED);

        (bytes32 depositId, uint256 volumeReceived) = bounty.receiveFunds{  <===================== this call will append a new element to the array
            value: msg.value
        }(msg.sender, _tokenAddress, _volume, _expiration);

        bytes memory funderUuidBytes = abi.encode(funderUuid);

        emit TokenDepositReceived(
            depositId,
            _bountyAddress,
            bounty.bountyId(),
            bounty.organization(),
            _tokenAddress,
            block.timestamp,
            msg.sender,
            _expiration,
            volumeReceived,
            0,
            funderUuidBytes,
            VERSION_1
        );
    }

function receiveFunds(
        address _funder,
        address _tokenAddress,
        uint256 _volume,
        uint256 _expiration
    )
        external
        payable
        virtual
        onlyDepositManager
        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);

        bytes32 depositId = _generateDepositId();

        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); <=============================================== a new element is appended here.
        tokenAddresses.add(_tokenAddress);

        return (depositId, volumeReceived);
    }

So a malicious user could just send many transaction with the smallest possible amount of a token in order to create many entries in the deposits and make the refunds revert. The amount required to perform the attack would probably be in the order of a few wei.

The issue does not apply to NFT deposits because they are limited to 5.

Impact

I set high because all users would be unable to get a refund. This can really degrade the image of the project if it happens even once because the funders would definitely be pissed off. They could ask the developer to refund afterwards but this would be at his or her good will...

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/DepositManager/Implementations/DepositManagerV1.sol#L54
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L21
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/DepositManager/Implementations/DepositManagerV1.sol#L172
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L341

Tool used

Manual Review

Recommendation

One of those solutions with a preference for the first one

• Change the way the refund volume is computed to avoid the iteration altogether.
• Limit the max number of deposits
• Set a minimum price for a deposit to disincentivize such behavior (which is not ideal if you want to support any token).

Duplicate of #77