VTVL contest - Trust's results

Lines of code

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

Vulnerability details

Description

The _baseVestedAmount() function calculates vested amount for some (claim, timestamp) pair. It is wrapped by several functions, like vestedAmount, which is used in withdraw() to calculate how much a user can retrieve from their claim. Therefore, it is critical that this function will calculate correctly for users to receive their funds.

Below is the calculation of the linear vest amount:

uint112 linearVestAmount = _claim.linearVestAmount * truncatedCurrentVestingDurationSecs / finalVestingDurationSecs;

Importantly, _claim.linearVestAmount is of type uint112 and truncatedCurrentVestingDurationSecs is of type uint40. Using compiler >= 0.8.0, the product cannot exceed uint112 or else the function reverts due to overflow. In fact, we can show that uint112 is an inadequate size for this calculation.

The max value for uint112 is 5192296858534827628530496329220096. Seconds in year = 3600 * 24 * 365 = 31536000 Tokens that inherit from ERC20 like the ones used in VTVL have 18 decimal places -> 1000000000000000000 This means the maximum number of tokens that are safe to vest for one year is 2**112 / 10e18 / (3600 * 24 * 365) = just 16,464,665 tokens. This is definitely not a very large amount and it is expected that some projects will mint a similar or larger amount for vesting for founders / early employees. For 4 year vesting, the safe amount drops to 4,116,166. Projects that are not forewarned about this size limit are likely to suffer from freeze of funds of employees, which will require very patchy manual revocation and restructuring of the vesting to not overflow.

Impact

Employees/founders do not have access to their vested tokens.

Proof of Concept

Below is a test that demonstrates the overflow issue, 1 year after 17,000,000 tokens have matured.

describe('Long vest fail', async () => {
  let vestingContract: VestingContractType;
  // Default params
  // linearly Vest 10000, every 1s, between TS 1000 and 2000
  // additionally, cliff vests another 5000, at TS = 900
  const recipientAddress = await randomAddress();
  const startTimestamp = BigNumber.from(1000);
  const endTimestamp = BigNumber.from(1000 + 3600 * 24 * 365);
  const midTimestamp = BigNumber.from(1000 + (3600 * 24 * 365) / 2);
  const cliffReleaseTimestamp = BigNumber.from(0);
  const linearVestAmount = BigNumber.from('170000000000000000000000000');
  const cliffAmount = BigNumber.from(0);
  const releaseIntervalSecs = BigNumber.from(5);

  before(async () => {
    const {vestingContract: _vc} = await createPrefundedVestingContract({tokenName, tokenSymbol, initialSupplyTokens});
    vestingContract = _vc;
    await vestingContract.createClaim(recipientAddress, startTimestamp, endTimestamp, cliffReleaseTimestamp, releaseIntervalSecs, linearVestAmount, cliffAmount);
  });

  it('half term works', async() => {
    expect(await vestingContract.vestedAmount(recipientAddress, midTimestamp)).to.be.equal('85000000000000000000000000');
  });

  it('full term fails', async() => {
    // Note: at exactly the cliff time, linear vested amount won't yet come in play as we're only at second 0
    await expect(vestingContract.vestedAmount(recipientAddress, endTimestamp)).to.be.revertedWithPanic(0x11
    );
  });
});

Tools Used

Manual audit, hardhat / chai.

Recommended Mitigation Steps

Perform the intermediate calculation of linearVestAmount using the uint256 type.

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

Lines of code

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

Vulnerability details

Description

VariableSupplyERC20Token is defined as a token that has a certain initial supply, and grows to some future max supply. If chosen maxSupply is not 0, it should never be possible to mint more than maxSupply tokens. This is verified in the following code from mint():

        if(mintableSupply > 0) {
            require(amount <= mintableSupply, "INVALID_AMOUNT");
            // We need to reduce the amount only if we're using the limit, if not just leave it be
            mintableSupply -= amount;
        }

mintableSupply starts out as maxSupply. The check that desired mint amount is under mintableSupply is only performed if mintableSupply > 0. However, if owner mints exactly mintableSupply, the check is passed and then mintableSupply will become 0. Now that it is 0, any future mint() call will not perform validation and will freely mint any amount. Additionally, mint() does not log any Event, so users are not alerted to the fact new tokens are being printed. Since this presents an instant rugging opportunity users could not have taken into account, I believe this attack qualifies as critical even though only the deployer can perform it.

Impact

Project owner can mint infinite tokens, hyperinflating the currency and making others' tokens worthless.

Proof of Concept

The code below demonstrates how it is possible to mint 300,000 tokens although max supply is set to 100,000.

const createContractFactory = async () => await ethers.getContractFactory("VariableSupplyERC20Token");
const deployTestToken = async (init_supply: number, max_supply: number) => {
    const factory = await createContractFactory();
    // TODO: check if we need any checks that the token be valid, etc
    const contract = await factory.deploy('Test token', 'TEST', init_supply, max_supply);
    return contract;
}

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

it("VariableSupply token has unbound max supply", async function () {
    let tokenAddress: string;
    let test_token = await deployTestToken(1000, 100_000);
    test_token.mint( await randomAddress(), 99_000);
    test_token.mint( await randomAddress(), 200_000);
    let total_supply = await test_token.totalSupply()
    expect(total_supply.toNumber()).to.be.equal(300_000);
});

Tools Used

Manual audit. Hardhat / chai.

Recommended Mitigation Steps

An additional state variable isCapped has to store whether the token has max supply or not. Using the same variable to store remaining mint amount and the isCapped will likely cause incorrect logic.