Missing validation for a parameter passed to an external function

In Solidity, for an integer type X, you can use type(X).min and type(X).max to access the minimum and maximum value representable by the type.

Code with vulnerability

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

    ...

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

    (bytes32 depositId, uint256 volumeReceived) = bounty.receiveFunds{
        value: msg.value
    }(msg.sender, _tokenAddress, _volume, _expiration);

    ...
}

Vulnerability description

DepositMangerV1 allows the caller to specify _expiration which specifies how long the deposit is locked. An attacker can specify a deposit with _expiration = type(uint256).max which will cause an overflow in the BountyCore#getLockedFunds sub-call and permanently break refunds. DepositManagerV1’s function fundBountyToken allows the depositor to specify an _expiration which is passed directly to BountyCore’s function receiveFunds(). BountyCore stores the _expiration in the expiration mapping.

expiration[depositId] = _expiration;

When requesting a refund, getLockedFunds returns the amount of funds currently locked. The line to focus on is depositTime[depList[i]] + expiration[depList[i]]

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

An attacker can cause getLockedFunds to always revert by making a deposit in which

depositTime[depList[i]] + expiration[depList[i]] > type(uint256).max

causing an overflow.

To exploit this the user would make a deposit with _expiration = type(uint256).max which will cause a guaranteed overflow. This causes DepositMangerV1’s function refundDeposit to always revert breaking all refunds.

Mitigation

Add the following check to DepositMangerV1’s function fundBountyToken():

require(_expiration <= type(uint128).max)

Sources

https://medium.com/coinmonks/learn-attack-vectors-and-explore-h-m-severity-issues-over-underflow-e331aa41d97b