VTVL contest - zzykxx's results

Lines of code

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

Vulnerability details

Impact

In VariableSupplyERC20Token.sol an admin can mint an infinite amount of tokens even if a maxSupply_ > 0 is specified. Users do not expected this to be possible and can lead to their token share diluted and loss of value.

Proof of Concept

Copy the test code inside VariableSupplyERC20Token.ts:

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

const createContractFactory = async () => await ethers.getContractFactory("VariableSupplyERC20Token");

const deployVestingContract = async (initialSupply: BigInt, maxSupply: BigInt) => {
    const factory = await createContractFactory();
    // TODO: check if we need any checks that the token be valid, etc
    const contract = await factory.deploy("TEST", "TST", 0, BigInt(10e18));
    return contract;
}

describe("VariableSupplyERC20Token", async function () {
    it("Can't mint more than maxSupply_", async () => {
        const MAX_SUPPLY = BigInt(10e18);
        const INITIAL_SUPPLY = BigInt(0);

        const [owner] = await ethers.getSigners();
        const contract = await deployVestingContract(INITIAL_SUPPLY, MAX_SUPPLY);
        await contract.deployed();

        //Owner mints the whole supply
        await contract.mint(owner.address, MAX_SUPPLY);
        const balance = await contract.balanceOf(owner.address);

        //Minting works
        expect(BigInt(balance.toString()) == MAX_SUPPLY);
        
        //And then mints one more
        const result = contract.mint(owner.address, 1);

        //Should fail but doesn't
        await expect(result).to.be.revertedWith("INVALID_AMOUNT")
    });
});

Recommended Mitigation Steps

The issue arises maninly because of the choice of considering a mintableSupply = 0 as "the admin can mint inifite tokens".

It is probably a better design choice to set mintableSupply = type(uint256).max for an indefinetely mintable token.

In addition to this, currently, the code prevents an indefinitely mintable token without initial supply to be deployed (initialSupply_ = 0 and maxSupply_ = 0), which is a restriction that adds unnecessary friction.

As such, I would consider changing the constructor to:

constructor(
    string memory name_,
    string memory symbol_,
    uint256 initialSupply_,
    uint256 maxSupply_
    ) ERC20(name_, symbol_) {
    
    //A token with a maxSupply_ of 0 doesn't make sense
    //instead of reverting, we consider it as indefinetely mintable
    mintableSupply = maxSupply_ > 0 ? maxSupply_ : type(uint256).max;

    if (initialSupply_ > 0) {
        mint(_msgSender(), initialSupply_);
    }
}

and the mint function to:

function mint(address account, uint256 amount) public onlyAdmin {
    require(account != address(0), "INVALID_ADDRESS");
    require(amount <= mintableSupply, "INVALID_AMOUNT");
    require(mintableSupply > 0, "INVALID_AMOUNT"); //Probably unnecessary

    mintableSupply -= amount;

    _mint(account, amount);
}

Lines of code

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

Vulnerability details

Impact

VTVLV.sol is supposed to be used with any ERC20 token, but the code doesn't take into consideration the amount of decimals of the token to vest. When choosing the values of _linearVestAmount and _cliffAmount (when calling createClaim) the founders have to know the number of decimals that the token they are vesting uses otherwise if the token has less decimals than expected it can lead to the admin over-allocating, if the token has more decimals than expected it can lead to the admin under-allocating. It can cause loss of funds from both the admin and users point of view.

Proof of Concept

Let's suppose VTVLV.sol is managing the vesting for some tokens with 17 decimals. When calling createClaim an admin might assume that the token he needs to vest has 18 decimals and set _linearVestAmount and _cliffAmount as the amount to vest scaled by 1e18. Let's suppose he sets:

• _linearVestAmount = 10_000 * 1e18
• _cliffAmount = 5_000 * 1e18

Some time passes by and an user calls withdraw(), which at some points calls tokenAddress.safeTransfer(_msgSender(), amountRemaining).

Here amountRemaining is scaled by 1e18 but the tokenAddress expects a value scaled by 1e17, which means the withdrew amount will be bigger than the expected one by a factor of 1e18/1e17 = 10 which the user is able to withdraw assuming the contract is funded with enough tokens.

The contract and the docs don't state anywhere how many decimals are to be assumed by the admin, which can lead to ambiguous situations.

Recommended Mitigation Steps

Since decimals are not supposed to change the number can be cached in an immutable variable which is processed in the constructor, let's call it DECIMALS.

DECIMALS should be equal to tokenAddress.decimals() unless the function reverts/throws/doesn't work, in which case the token can be either rejected or it can be assumed it has 18 decimals.

At this point we have a couple of options:

• An idea is to have the founders input _linearVestAmount and _cliffAmount as values scaled by 1e18 and then adjust the numbers by scaling them (at L285) based on 10^DECIMALS (allows for 1e18 precision at most).

• Another idea is to have the founders input non-scaled values and then scale the values of _linearVestAmount and _cliffAmount by 10^DECIMALS (doesn't allow for precision):

    Claim memory _claim = Claim({
        startTimestamp: _startTimestamp,
        endTimestamp: _endTimestamp,
        cliffReleaseTimestamp: _cliffReleaseTimestamp,
        releaseIntervalSecs: _releaseIntervalSecs,
        cliffAmount: _cliffAmount * (10 ** DECIMALS),
        linearVestAmount: _linearVestAmount * (10 ** DECIMALS),
        amountWithdrawn: 0,
        isActive: true
    });