VTVL contest - 0xmatt's results

Lines of code

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

Vulnerability details

Impact

As the supplied VariableSupplyERC20Token does not support burning a supply limit can be set on construction via the maxSupply_ parameter. This is copied to a public mintableSupply variable. To enforce this supply limit the mint function has a require check ensuring the amount to be minted does not exceed the mintableSupply value. However, the require test is contained inside an if statement that is only executed if mintableSupply > 0. This means any token using this contract will have an infinite supply once mintableSupply drops to 0, even when otherwise expected.

This issue may be triggered accidentally and may significantly impact token price for VTVL-based project users. Therefore I have reported this issue as High. A project depending on a fixed supply may encounter significant downside when it becomes apparent supply isn't really limited.

Proof of Concept

VariableSupplyERC20Token.sol's constructor allows the token creator to specify an initial and max supply. The maximum token supply value is stored in maxSupply_. Following a check to ensure either initialSupply or maxSupply is greater than zero, the value of maxSupply_ is assigned to the public variable mintableSupply. The initial supply is then minted by calling the mint() function.

The mint() function checks that the destination address is not 0, and that mintableSupply > 0. When mintableSupply > 0 a require statement checks that the amount to be minted is less than the mintable Supply. The mintableSupply value is then reduced by the amount to be minted. The if statement then exits.

If mintableSupply is 0 this means mint at will. The problem is that the mintableSupply value will eventually = 0 as new tokens are minted. Once this happens the if check will never apply, and tokens will simply be minted at will.

Tools Used

VSCode, Remix

Recommended Mitigations

The mintableSupply variable alone is insufficient to track all possible states under which minting should take place. Two new variables should be used to track state, set in the contract:

uint256 public maxSupply; // The total max supply. Variable on unlimitedMint, otherwise fixed.
bool public unlimitedMint; // Determine if unlimitedMint is enabled.

These two allow the contract to correctly enumerate all possible conditions under which a mint is possible:

1. If unlimitedMint is enabled then mint. Otherwise:
2. If totalSupply() + amount < maxSupply then mint. Otherwise:

This also removes the need to check that amount <= mintableSupply, as a greater value would also exceed case 2 above.

To avoid ABI changes (as this may require front-end changes), the following could be added to the constructor:

maxSupply = maxSupply_;
if (maxSupply == 0){
	unlimitedMint = true;
}

The code above enables an unlimitedMint mode if maxSupply == 0 as mentioned in the comments. Two capture cases 1 and 2 above, code such as the following could be used:

function mint(address account, uint256 amount) public onlyAdmin {

uint256 total = totalSupply() + amount;
require(account != address(0), "INVALID_ADDRESS");

// If we're using maxSupply, we need to make sure we respect it
// unless unlimitedMint is enabled.
// mintableSupply = 0 means mint at will

require(total <= maxSupply || unlimitedMint, "INVALID_AMOUNT");

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

if(unlimitedMint) {
	// Increase maxSupply to reflect the change in total Supply.
	maxSupply += amount;
}

_mint(account, amount);

}

Other Examples

None Found.

Vulnerability Details

Low/Non-Critical Findings

L-01: finalVestedAmount() Can report incorrect or ambiguous values.

The finalVestedAmount() function in VTVLVesting.sol asserts to return the "total vested at the end of the schedule". It does this by using a _claim.endTimestamp value to calculate a claim via the _baseVestedAmount() function.

When a user partially withdraws during the vesting period, the claim's amountWithdrawn values are updated. This change is not reflected in the finalVestedAmount() function, which will report the original final value. The ambiguity comes from interpretations of "total" in the comments versus "final" in the function name.

It may be helpful to rename the function to totalVestedAmount, if this is supposed to reflect the sum total vested throughout the schedule's lifetime. If this should reflect the final amount vested at the end of the schedule, replace line 208 with:

return _baseVestedAmount(_claim, _claim.endTimestamp) - _claim.amountWithdrawn;

There is a possibility that people may be led to believe that an amount remaining vested in a VTVL-produced vesting contract is significantly higher than in reality. The impact of this issue is contextual, therefore I have rated this as Low/QA.

L-02: Constructor does not check that supplied address is a valid contract.

The constructor in VTVLVesting.sol checks that the supplied _tokenAddress value is not 0. It doesn't check that it's a valid contract.

Check the supplied address' extcodesize to determine whether or not this is a valid contract.

uint size;
assembly { size := extcodesize(addr) }
require(size != 0, "NOT_CONTRACT");