VTVL contest - neumo's results

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L176 https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L147 https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L196-L199 https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L206-L209 https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L215-L218 https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L371 https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L422

Vulnerability details

Impact

In contract VTVLVesting.sol, the multiplication in function _baseVestedAmount can overflow for big enough values of truncatedCurrentVestingDurationSecs and linearVestAmount. This means the claim could be successfully created by the admin, but could NEVER be revoked or fully withdrawn (it could only be withdrawn as far as truncatedCurrentVestingDurationSecs is small enough to not make the multiplication overflow. In fact, ALL calls to function _baseVestedAmount passing in a value of _referenceTs greater than or equal to the claim's endTimestamp would result in a revert. This affects functions vestedAmount, finalVestedAmount, claimableAmount, withdraw and revokeClaim.

Proof of Concept

The test below, shows how if we use a release interval of 1 second and a linear vesting span of 10 years, we can create a claim of 16,500,000 tokens, but if we then call function revokeClaim, the call reverts because the multiplication would overflow.

/* eslint-disable prettier/prettier */
import { expect } from "chai";
import { ethers } from "hardhat";
import Chance  from "chance";
// eslint-disable-next-line node/no-missing-import
import { VTVLVesting } from "../typechain";
import { BigNumber } from "ethers";
import { PANIC_CODES } from "@nomicfoundation/hardhat-chai-matchers/panic";
import "@nomicfoundation/hardhat-chai-matchers";

const chance = new Chance(43153); // Make sure we have a predictable seed for repeatability

const randomAddress = async () => {
  return await ethers.Wallet.createRandom().getAddress();
}

const createContractFactory = async () => await ethers.getContractFactory("VTVLVesting");
const deployVestingContract = async (tokenAddress?: string) => {
  const factory = await createContractFactory();
  // TODO: check if we need any checks that the token be valid, etc
  const contract = await factory.deploy(tokenAddress ?? await randomAddress());
  return contract;
}

const dateToTs = (date: Date|string) => ethers.BigNumber.from(Math.floor((date instanceof Date ? date : new Date(date)).getTime() / 1000));

const createPrefundedVestingContractBN = async (props: {tokenName: string, tokenSymbol: string, initialSupply: BigNumber}) => {
  const {
      tokenName, 
      tokenSymbol, 
      initialSupply, 
  } = props;

  // Create an example token
  const tokenContractFactory = await ethers.getContractFactory('TestERC20Token');
  // const initialSupply = ethers.utils.parseUnits(initialSupplyTokens.toString(), decimals);
  const tokenContract = await tokenContractFactory.deploy(tokenName, tokenSymbol, initialSupply);

  // Create an example vesting contract
  const vestingContract = await deployVestingContract(tokenContract.address);
  await vestingContract.deployed();

  expect(await vestingContract.tokenAddress()).to.be.equal(tokenContract.address);

  // Fund the vesting contract - transfer everything to the vesting contract (from the user)
  await tokenContract.transfer(vestingContract.address, await tokenContract.totalSupply());

  return {tokenContract, vestingContract};
}

type VestingContractType = VTVLVesting;

describe("My tests", async function () {

    it('overflow test', async () => {
        const recipientAddress = await randomAddress();
        const initialSupply = BigNumber.from("5100000000000000000000000000000000");
        const tokenName = "TOKEN";
        const tokenSymbol = "TK";
        const {vestingContract} = await createPrefundedVestingContractBN({tokenName, tokenSymbol, initialSupply});
        // Create and immediately revoke the claim
        const myLinearVestAmount = BigNumber.from("16500000000000000000000000");
        const myStartTimestamp = dateToTs('2012-02-01');
        const myEndTimestamp = dateToTs('2022-02-01');
        const myCliffReleaseTimestamp = dateToTs(new Date('2010-02-01'));
        const cliffAmount = BigNumber.from("16500000000000000000000000");
        const myReleaseIntervalSecs = 1;
        await vestingContract.createClaim(recipientAddress, myStartTimestamp, myEndTimestamp, myCliffReleaseTimestamp, myReleaseIntervalSecs, myLinearVestAmount, cliffAmount);
        await expect(vestingContract.vestedAmount(recipientAddress, myEndTimestamp)).to.be.revertedWithPanic(PANIC_CODES.ARITHMETIC_UNDER_OR_OVERFLOW);
        await expect(vestingContract.revokeClaim(recipientAddress)).to.be.revertedWithPanic(PANIC_CODES.ARITHMETIC_UNDER_OR_OVERFLOW);
      });
      
});

Tools Used

Hardhat

Recommended Mitigation Steps

Devs are using uint112 to store the claim amounts thinking this is enough, and this certainly is. But to overcome the overflow caused by the multiplication in line 176, one solution would be casting. The line could be changed like this:

uint112 linearVestAmount = uint112(uint152(_claim.linearVestAmount) * truncatedCurrentVestingDurationSecs / finalVestingDurationSecs);

This way the multiplication would fit in a uint152 and after division by finalVestingDurationSecs we would downcast back to uint112. Note here that finalVestingDurationSecs is always greater than or equal to truncatedCurrentVestingDurationSecs, so the downcast will not give us any trouble.

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/token/VariableSupplyERC20Token.sol#L40

Vulnerability details

Impact

Contract VariableSupplyERC20Token.sol can be initialized at creation with a value that specifies the maxSupply for the token. If 0 is passed to maxSupply in the constructor, it means there is no mint limit. But if a value greater than zero is passed in, the contract should never allow to mint tokens if the totalSupply after minting surpasses that maxSupply value.

Proof of Concept

In the test below, I deploy a VariableSupplyERC20Token contract with maxSupply 2M tokens and an initial mint of 1M. After deployment, the total supply is 1M (because of the initial mint) and the mintable tokens remaining are 1M of the 2M max supply. We mint the remaining 1M tokens and then the mintableSupply is equal to zero. Because of this, it passes the check at line 40 and does not check if we are surpassing the max supply.

import { expect } from "chai";
import { ethers } from "hardhat";

const createContractFactory = async () => await ethers.getContractFactory("VariableSupplyERC20Token");
const deployTokenContract = async () => {
    const factory = await createContractFactory();
    const contract = await factory.deploy("TOKEN", "TK", 1_000_000, 2_000_000);
    return contract;
}

describe("VariableSupplyToken", async function () {

    it("can mint beyond maxSupply", async () => {

        const contract = await deployTokenContract();
        await contract.deployed();
    
        expect(await contract.totalSupply()).to.equal(1_000_000);

        expect(await contract.mintableSupply()).to.equal(1_000_000);

        await contract.mint(contract.address, 1_000_000);
        expect(await contract.totalSupply()).to.equal(2_000_000);

        await contract.mint(contract.address, 2_000_000);
        expect(await contract.totalSupply()).to.equal(4_000_000);

    });
    
});

Tools Used

Hardhat

Recommended Mitigation Steps

Don't get track of the mintable supply. Instead of this, record the maxSupply in an immutable variable at contract creation:

uint256 public immutable maxSupply;

And change function mint to check if the amount to mint is less than or equal than the tokens left to get to the maxSupply:

function mint(address account, uint256 amount) public onlyAdmin {
    require(account != address(0), "INVALID_ADDRESS");
    if(maxSupply > 0) {
        require(amount <= (maxSupply - totalSupply), "INVALID_AMOUNT");
    }
    _mint(account, amount);
}

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L53 https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L418-L437 https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L223-L225 https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L230-L232

Vulnerability details

Impact

If an admin revokes a claim of a recipient that has not withdrawn anything yet, the vestingRecipients array should not consider it as a valid recipient because it has not (and will not) received anything.

Proof of Concept

The contract VTVLVesting.sol has the array vestingRecipients to keep track of the addresses that have a claim. When the admin revokes the claim for one given recipient in function revokeClaim, in the case that the recipient has not withdrawn any amount (claim.amountWithdrawn == 0), the function should remove the address from the vestingRecipients array. Otherwise, functions allVestingRecipients and numVestingRecipients would return wrong values.

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove the address of the recipient from the vestingRecipients array in the call to revokeClaim. Add this code snippet before the event emission in function revokeClaim:

// If recipient has not withdrawn any amount, remove the address from the vestingRecipients array
if(_claim.amountWithdrawn == 0){
    address lastRecipient = vestingRecipients[vestingRecipients.length - 1];
    for(uint i = 0;i<vestingRecipients.length;i++){
        if(vestingRecipients[i] == _recipient){
            vestingRecipients[i] = lastRecipient;
            vestingRecipients.pop();
            break;
        }
    }
}