VTVL contest - wagmi's results

Lines of code

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

Vulnerability details

Impact

In VariableSupplyERC20Token, if token are using maxSupply, it will assigned that value to mintableSupply and whenever some tokens are minted, mintableSupply is updated accordingly. And in case mintableSupply = 0, it means there is no max supply limit.

require(initialSupply_ > 0 || maxSupply_ > 0, "INVALID_AMOUNT");
mintableSupply = maxSupply_;

In case token has maxSupply, since mintableSupply is reduced accordingly after every mint(), its value can become mintableSupply = 0 and the token is no longer has max supply limit.

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;
}

Proof of Concept

Consider the scenario

1. Admin created a token with maxSupply = 1000, which means mintableSupply = 1000
2. Admin called mint() with amount = 1000, it’s okay and mintableSupply is updated

mintableSupply -= amount;
mintableSupply = 0;

3. Admin can mint any amount of token at will since mintableSupply == 0 now.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider store maxSupply value and change the check to

require(totalSupply + _amount <= maxSupply);

Lines of code

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

Vulnerability details

Impact

In VTVLVesting contract, vestingRecipients list is declared as internal which means we cannot read it values directly (like from Etherscan). We can only get its values from allVestingRecipients() view function.

This function fetched all elements of the list from storage, which is really gas consuming and even can break the block gas limit in case the list is too large. Even though users don’t need to pay gas for view function, this function is still failed if its gas cost larger than block gas limit.

So in general, if there are too many vesting recipients, allVestingRecipients() may be unabled to called and since vestingRecipients is internal, it’s cannot be read directly either.

Proof of Concept

Function allVestingRecipients() returns the whole vestingRecipients list

function allVestingRecipients() external view returns (address[] memory) {
    return vestingRecipients; // @audit out of gas
}

And admin can add any number of vesting recipient he wants through createClaim() or createClaimsBatch()

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding another view function to read vestingRecipients list, for example in range

function allVestingRecipients(uint startIdx, uint endIdx) external view returns (address[] memory)