Ruhum - Deposits can be refunded after a bounty closed

[github-actions]

Ruhum

medium

Deposits can be refunded after a bounty closed

Summary

Bounties can only be funded while they are still open. But, deposits can be refunded at any time. If a depositor refunds after the bounty is closed, the reward distribution will be broken.

Vulnerability Detail

This issue affects the TieredFixedBounty and TieredPercentageBounty. Before the first tier is claimed, the bounty is set to closed. After that, you're not able to deposit new funds through the DepositManager. But, depositors are still able to refund their deposits given that they expired.

If a deposit is refunded after the contest has ended, the bounty won't have enough funds to cover all the upcoming claims. At least one of the users won't be able to claim their rewards.

Impact

Refunds after a competition is closed will break the reward distribution.

Code Snippet

Given that we use a TieredPercentageBounty:

1. Alice, Bob, and Charlie deposit 1000 tokens each with Alice's expiration date set to 1 week
2. The bounty issuer sets the payout schedule through setPayoutSchedule()
3. First user claims their reward and the ClaimManager closes the bounty using closeCompetition():

        if (_bounty.status() == 0) {
            _bounty.closeCompetition();

            emit BountyClosed(
                _bounty.bountyId(),
                address(_bounty),
                _bounty.organization(),
                address(0),
                block.timestamp,
                _bounty.bountyType(),
                new bytes(0),
                VERSION_1
            );
        }

4. The bounty calculates the total funding which is used to distribute each tier's rewards:

    /// @notice Similar to close() for single priced bounties. closeCompetition() freezes the current funds for the competition.
    function closeCompetition() external onlyClaimManager {
        require(
            status == OpenQDefinitions.OPEN,
            Errors.CONTRACT_ALREADY_CLOSED
        );

        status = OpenQDefinitions.CLOSED;
        bountyClosedTime = block.timestamp;

        for (uint256 i = 0; i < getTokenAddresses().length; i++) {
            address _tokenAddress = getTokenAddresses()[i];
            fundingTotals[_tokenAddress] = getTokenBalance(_tokenAddress);
        }
    }

As described in the function's comment, it assumes the funds to be frozen.
5. Alice's deposit expires and she refunds her deposit using refundDeposit():

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

This leaves the bounty with a total of 2000 tokens instead of the original 3000. But, the fundingTotals state variable still uses the original 3000 value. Subsequent claims will try to distribute more tokens than the bounty holds.

Tool used

Manual Review

Recommendation

Refunds should be disabled after the competition closes.

Duplicate of #266