VTVL contest - neko_nyaa'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#L41

Vulnerability details

Impact

Line 41 of VTVLVesting.sol implies the assumption that a vesting can reach up to 5,192,296,858,534,827 tokens of 18 decimal. However, line 176 can potentially overflow, effective locking funds in the contract.

Proof of Concept

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

Specifically, _claim.linearVestAmount is of uint112 data type, and truncatedCurrentVestingDurationSecs is of uint40, and thus the data type when multiplied together will be uint112.

For a realistic demonstration, let us assume a vesting schedule of 2 years, that is $86400 * 365 * 2 = 63072000$ seconds. This means that any vesting of more than $82323326$ tokens (assuming 18 decimals), after enough time has passed, will result in _claim.linearVestAmount * truncatedCurrentVestingDurationSecs value exceeding uint112 data type, causing the transaction to revert.

As a result, because the only possible paths to withdraw funds in an active claim are either through withdraw() or through admin revoking a claim, then withdrawing the amount, both of which must go through _baseVestedAmount(), the funds are permanently locked in the contract.

This is identified as a vulnerability because the comment on line 41 implies the assumption of a much larger vesting capability by the contract. In practice, there are numerous 18-decimal tokens with a total supply of 10 or 11 figures, with large vestings amount on a year-long schedule.

Reverse engineering the affected code shows that test() indeed reverts. Screenshot.:

pragma solidity ^0.8.14;

contract Test{
    uint40 constant ONE_YEAR = 31536000;
    uint112 constant LINEAR_VEST_AMOUNT = 85_000_000*1e18;

    constructor() {}

    function test() public view returns (uint112) {
        uint40 currentVestingDurationSecs = ONE_YEAR*2;
        uint40 truncatedCurrentVestingDurationSecs = (currentVestingDurationSecs / 1) * 1;
        uint40 finalVestingDurationSecs = ONE_YEAR*2; 
        
        uint112 linearVestAmount = LINEAR_VEST_AMOUNT * truncatedCurrentVestingDurationSecs / finalVestingDurationSecs;
        return linearVestAmount;
    }
}

Tools Used

Remix IDE

Recommended Mitigation Steps

Appropriate typecasting before multiplication will prevent the issue:

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

[N-01] hasActiveClaim modifier has an unnecessary check

The unnecessary check is here:

require(_claim.startTimestamp > 0, "NO_ACTIVE_CLAIM");

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

The condition _claim.isActive == true is enough, as a storage slot will be all set to 0 if the claim was never created (i.e. a claim that was not created cannot be active).

[N-02] withdrawAdmin() should be made external rather than public

For it is not called anywhere within the contract. Also on the docs it is external

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L398 https://github.com/code-423n4/2022-09-vtvl/blob/main/docs/VTVLVesting.md#withdrawadmin

[N-03] Event ClaimRevoked's revocationTimestamp is redundant

The revocationTimestamp emitted is nothing more than block.timestamp of the operation, which information is readily available when querying or subscribing to events.

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

[G-01] mintableSupply can be made immutable by looking at the token's total supply.

Variable mintableSupply from VariableSupplyERC20Token.sol can be made immutable instead of storage.

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

The mint function from said contract can be modified to only mint if the condition totalSupply() + amount <= mintableSupply is satisfied.

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

The gas cost of the addition operation can be further eliminated by placing the check after the mint function:

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

Saves one Gsset (20000 gas) on construction, as well as an SLOAD (2100 slot) read into the original mintableSupply slot each time the mint function is called.

[G-02] Using != 0 for non-zero uint comparison takes less gas than >0

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

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L256 https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L257 https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L263 https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L449

[G-03] _admins: Using bool data type for storage incurs extra gas overhead

mapping(address => bool) private _admins;

Use uint256 for less gas cost

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

[G-04] numTokensReservedForVesting: Uses smaller-than-32-bytes data type, as well as initializing default value

Both of which costs extra gas

uint112 public numTokensReservedForVesting = 0;

Can be changed to the following

uint256 public numTokensReservedForVesting;

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

[G-05] ++i costs less gas than i++

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

[G-06] require statements with && can be split into multiple requires to save gas

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