VTVL contest - JLevick's results

Lines of code

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

Vulnerability details

Impact

Comment on line 19 suggests that contract wont allow minting over maxSupply (unless specified with maxSupply = 0) however the check in mint() doesn't prevent this and an unlimited number of tokens can be minted even if mintableSupply isn't initially set to 0.

Proof of Concept

1. Contract is deployed with maxSupply = 100 & initialSupply = 0
2. Admin mints 100 tokens
3. mintableSupply will now equal 0 and any future calls will bypass the if statement allowing the admin to mint as many tokens as they like.

Recommended Mitigation Steps

Add another variable in the constructor:

unlimitedSupply = maxSupply == 0 ? true : false;

And change mint() to:

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

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L253 https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L129 https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L418-L437

Vulnerability details

Description

If a user has their claim revoked & then the admin wants to recreate a new claim for them it will fail. This is due to the hasNoClaim() modifier on _createClaimUnchecked() as _claim.startTimestamp != 0.

Proof of Concept

1. Bob creates a claim for Dave
2. Bob is unhappy with Dave for whatever reason and revokes the claim.
3. A couple of different scenarios could be either Dave successfully disputes this and Bob wants to reinstate the claim, or sometime in the future Bob just wants to start an entirely new claim for Dave. Both involve Bob trying to create a new claim for Dave and it failing.

Recommended Mitigation Steps

In revokeClaim() add the line:

_claim.startTimestamp = 0;

Low

1. There is a comment on line 203 suggesting that the function finalVestedAmount() is unneccessary and may be removed, to prevent revokeClaim() from reverting if this happens I recommend updating line 422 to:

uint112 finalVestAmt = vestedAmount(_recipient, _claim.endTimestamp);

VTVLVesting.sol#L203 VTVLVesting.sol#L422

2. Recommend locking pragma to the version used in testing. VariableSupplyERC20Token.sol#L2

Non Critical

1. Recommend removing open todos before deployment. VTVLVesting.sol#L266

2. Natspec is incomplete in the following places. VTVLVesting.sol#L416 - add @param _recipient

Gas Report

1. Comment states finalVestedAmount() is superfluous, deployment gas can be saved by removing it and updating line 422 to:

uint112 finalVestAmt = vestedAmount(_recipient, _claim.endTimestamp);

VTVLVesting.sol#L201-L209 VTVLVesting.sol#L422

2. It is unneccessary to assign state variables to their default value

VTVLVesting.sol#L27 - can update to:

uint112 public numTokensReservedForVesting;

3. Using x = x +/- y is cheaper than x +/-= y for state variables

VTVLVesting.sol#L383 VTVLVesting.sol#L433 VTVLVesting.sol#L301 VTVLVesting.sol#L381

4. Custom errors are cheaper than require statements.

Gas savings above are from updating 1 require statement to a custom error, there are 24 require statements throughout the VTVL contracts, so updating them all will give gas savings of ~48,000. FullPremintERC20Token.sol#L11 AccessProtected.sol#L25 AccessProtected.sol#L40 VariableSupplyERC20Token.sol#L27 VariableSupplyERC20Token.sol#L37 VariableSupplyERC20Token.sol#L41 VTVLVesting.sol#L82 VTVLVesting.sol#L107 VTVLVesting.sol#L111 VTVLVesting.sol#L129 VTVLVesting.sol#L255-L257 VTVLVesting.sol#L262-L264 VTVLVesting.sol#L270-L278 VTVLVesting.sol#L295 VTVLVesting.sol#L344-L351 VTVLVesting.sol#L374 VTVLVesting.sol#L402 VTVLVesting.sol#L426 VTVLVesting.sol#L447-L449