Unsafe cast blocks withdraw of tokens

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-size/blob/59ab86cffde7ba1d2565e97dd7731412a6394183/src/util/CommonTokenMath.sol#L59-L67

Vulnerability details

Unsafe cast blocks withdraw of tokens

Impact

tokensAvailableAtTime is a internal function used by tokensAvailableForWithdrawal and this one being used internally (also externally as it is public) by withdraw method. cliffAmount is a uint256, being able to have much bigger values than a uint128. However, it is used by the return method of tokensAvailableAtTime with a cast to uint128. If the user has more tokens than type(uint128).max, he won't be able to withdraw them as return value would be 0.

PoC

Affected code

    function tokensAvailableAtTime(
        uint32 vestingStart,
        uint32 vestingEnd,
        uint32 currentTime,
        uint128 cliffPercent,
        uint128 baseAmount
    ) internal pure returns (uint128) {
        if (currentTime > vestingEnd) {
            return baseAmount; // If vesting is over, bidder is owed all tokens
        } else if (currentTime <= vestingStart) {
            return 0; // If cliff hasn't been triggered yet, bidder receives no tokens
        } else {
            // Vesting is active and cliff has triggered
            uint256 cliffAmount = FixedPointMathLib.mulDivDown(baseAmount, cliffPercent, 1e18);

            return uint128(
                cliffAmount
                    + FixedPointMathLib.mulDivDown(
                        baseAmount - cliffAmount, currentTime - vestingStart, vestingEnd - vestingStart
                    )
            );
        }
    }

https://github.com/code-423n4/2022-11-size/blob/59ab86cffde7ba1d2565e97dd7731412a6394183/src/util/CommonTokenMath.sol#L59-L67

Test case used in Remix for casts

    // uint128 max value: 340282366920938463463374607431768211455
    uint constant public CASE_128_MAX_VALUE = 340282366920938463463374607431768211455;
    // uint128 max value + 1: 340282366920938463463374607431768211456
    uint constant public CASE_128_MAX_VALUE_PLUS_ONE = 340282366920938463463374607431768211456;

    uint128 saved;
    uint256 base;

    // If used CASE_128_MAX_VALUE_PLUS_ONE value in castThis
    // base will be correctly assigned as expected
    // saved will be assigned to 0

    function castThis(uint a) public returns (uint128){
        base = a;
        saved = uint128(a);
        return saved;
    }

    function getSaved() public view returns (uint128){
        return saved;
    }

    function getBase() public view returns (uint){
        return base;
    }

Mitigation

Even though Solidity 0.8.x is used, type casts do not throw an error. A SafeCast library must be used everywhere a typecast is done. SafeCast References: https://docs.openzeppelin.com/contracts/4.x/api/utils#SafeCast

[trust1995]

baseAmount is uint128 and cliffPercent is checked to be under 1e18. Therefore, cliff amount is bound to uint128.

            uint256 cliffAmount = FixedPointMathLib.mulDivDown(baseAmount, cliffPercent, 1e18);

This makes the issue invalid.

[0xean]

        if (timings.cliffPercent > 1e18) {
            revert InvalidCliffPercent();
        }

[c4-judge]

0xean marked the issue as unsatisfactory: Invalid