VTVL contest - wastewa's results

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L176

Vulnerability details

Impact

If a Claim object (for a user) has a large enough vesting time and/or linear vesting amount, then that object can get into a state such that the calculations in _baseVestedAmount overflow. If the _baseVestedAmount calculations overflow, then the associated tokens are locked into the contract with no way to retrieve them, because revokeClaim and withdraw both rely on this calculation to succeed.

In my PoC below, I show that it is not unreasonable to hit this state. It is very common for employees to vest over 4 years (~1.26*10^(8) seconds) and for token amounts to go into billions.

Proof of Concept

On line 167 of VTVLVesting.sol, the calculation is: uint112 linearVestAmount = _claim.linearVestAmount * truncatedCurrentVestingDurationSecs / finalVestingDurationSecs; This calculation overflows if _claim.linearVestAmount * truncatedCurrentVestingDurationSecs exceeds the max value of uint112, as solidity does the math operations with the largest type used.

This is very easy to do as I show below (with very reasonable values):

  it("create unclaimable vesting schedule", async() => {
    const [ recip1 ] = await ethers.getSigners()

    // token params
    let tokenName = "company Shares"
    let tokenSymbol = "aapl"
    let initialSupplyTokens = 6 * (10**9) // 6 billion shares

    const { tokenContract, vestingContract } = 
        await createPrefundedVestingContract({tokenName, tokenSymbol, initialSupplyTokens});
    

    // claim params
    const vestDuration = 60 * 60 * 24 * 365 // vesting period is a year
    const startTimestamp = await getLastBlockTs() - vestDuration;
    const endTimestamp = startTimestamp + vestDuration;
    const releaseIntervalSecs = 10;
    const cliffAmount = BigNumber.from(0);
    const cliffReleaseTimestamp = 0;
    const linearVestAmount = ethers.constants.WeiPerEther.mul(initialSupplyTokens); // initial supply * 10 ** 18 (decimal amount)
    
    // make claim
    await vestingContract.createClaim(recip1.address, startTimestamp, endTimestamp, cliffReleaseTimestamp, releaseIntervalSecs, linearVestAmount, cliffAmount);

    // withdraw and fail due to overflow exception
    await expect(vestingContract.connect(recip1).withdraw()).to.be.reverted
    // no function that depends on _baseVestedAmount calculation will work for user
    await expect(vestingContract.revokeClaim(recip1.address)).to.be.reverted
  })

Tools Used

Used ts and chai tests already in project (I stuck my PoC code snippet into VTVLVesting.ts to run it)

Recommended Mitigation Steps

A very easy fix is to make a temporary variable that is type uint256 (like making truncatedCurrentVestingDurationSecs uint256 just for the calculation) to use in the multiplication part of the calculation, this makes it possible for multiplication part of the calculation to exceed the max value uint112 (resulting value will definitely fit into a uint112 because the maximum value of this calculation is _claim.linearVestAmount, which is of type uint112).

Would not recommend capping values as a fix, as it is would heavily limit use of code.