VTVL contest - Rolezn's results

Lines of code

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

Vulnerability details

Description

According to docs for maxSupply_ :

The contract won't allow minting over this amount. Set to 0 for no limit.
This means that if maxSupply_ is set, then no minting beyond maxSupply_ is allowed.
max supply == 0 means mint at will. 

However, according to the logic placed in VariableSupplyERC20Token.sol it is still possible to mint as the state will switch to mint at will when mintableSupply reaches 0

Impact

It is possible to continue minting past the maxSupply that was defined by maxSupply_

Proof of Concept

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

Assume: maxSupply_ = 100; mintableSupply = 100; Admin calls mint(0x1, 100);

This will mint 100 tokens to 0x1 mintableSupply -= 100 -> mintableSupply = 100 - 100 = 0

Since mintableSupply is now equal to 0, admin can mint at will since it won't enter the condition of:

If(mintableSupply > 0)

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

And will skip to calling _mint(account, amount); https://github.com/code-423n4/2022-09-vtvl/tree/main/contracts/token/VariableSupplyERC20Token.sol#L45

Another option is, assume: initialSupply_ = maxSupply_ = 100

The constructor calls mint(msgSender(), inmitialSupply); This will mint 100 tokes to _msgSender() and supposedly reach maxSupply of 100 and mintableSupply -= 100 -> mintableSupply = 100 - 100 = 0

However, it is still possible to mint at will since mintableSupply is now equal to 0, admin can mint at will since it won't enter the condition of:

If(mintableSupply > 0)

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

Quick test done through brownie (since I don't know how to handle hardhat):

from brownie import accounts, config, VariableSupplyERC20Token

def main():
    deployer = accounts[0]
    someUser = accounts[1]
    tokenContract = VariableSupplyERC20Token.deploy("testToken", "T", 0, 100, {'from': deployer})
    print(f'mintableSupply before mint: {tokenContract.mintableSupply()}') # @audit will show mintableSupply = 100
    tokenContract.mint(someUser, 100)
    print(f'mintableSupply after mint: {tokenContract.mintableSupply()}') # @audit will show mintableSupply = 0
    tokenContract.mint(someUser, 100) # @audit still possible to mint because mintableSupply = 0 means mintAtwill even though it's not, since initial mintableSupply = 100

Recommended Mitigation Steps

Add a mintAtWill bool to check whether the state is defined as mint at will.

contract VariableSupplyERC20Token is ERC20, AccessProtected {
	uint256 public mintableSupply;
	bool public mintAtWill; // @audit added mintAtWill bool
	    
	constructor(string memory name_, string memory symbol_, uint256 initialSupply_, uint256 maxSupply_) ERC20(name_, symbol_) {
	        require(initialSupply_ > 0 || maxSupply_ > 0, "INVALID_AMOUNT");
	        mintableSupply = maxSupply_;
	
	        if(initialSupply_ > 0) {
	            if(maxSupply_ == 0) { // @audit max supply == 0 means mint at will. 
	                mintAtWill = true; //@audit change mintAtWill to true
	            }
	            mint(_msgSender(), initialSupply_);
	        }
	    }
	
	 function mint(address account, uint256 amount) public onlyAdmin {
	        require(account != address(0), "INVALID_ADDRESS");
	        if(mintableSupply > 0) {
	            require(amount <= mintableSupply, "INVALID_AMOUNT");
	            mintableSupply -= amount;
	        }
	        else {
	            require(mintAtWill, "Mint at will is disabled"); // @audit add require check for mintAtWill
	        }
	        _mint(account, amount);
	    }
}

Total Low Severity: 3 Total Non-Critical Severity: 7

(1) Update OpenZeppelin Packages to latest v4.7.3

Severity: Low

According to package.json, the OZ packages are currently set to ^4.5.0

Proof of Concept

"@openzeppelin/contracts": "^4.5.0",

Package.json#L7

Recommended Mitigation Steps

Update the versions of @openzeppelin/contracts and @openzeppelin/contracts-upgradeable to be the latest in package.json. I also recommend double checking the versions of other dependencies as a precaution, as they may include important bug fixes.

(2) Missing Checks for Address(0x0)

Severity: Low

Missing zero address check for accidently calling revokeClaim on address(0).

Proof Of Concept

function revokeClaim(address _recipient) external onlyAdmin hasActiveClaim(_recipient) {

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

Recommended Mitigation Steps

Consider adding zero-address checks in the mentioned codebase.

(3) Contracts are not using their OZ Upgradeable counterparts

Severity: Low

The non-upgradeable standard version of OpenZeppelin’s library, such as ERC20, Ownable, Context and SafeERC20 are inherited / used by the contracts.

It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour.

Recommended Mitigation Steps

Where applicable, use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts.

(4) No need to check for true in require

Severity: Non-Critical

Since isActive is bool, there is no need to check for "true" value, can simply call: require(_claim.isActive, "NO_ACTIVE_CLAIM");

Proof Of Concept

require(_claim.isActive == true, "NO_ACTIVE_CLAIM");

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

Recommended Mitigation Steps

require(_claim.isActive, "NO_ACTIVE_CLAIM");

(5) Avoid Floating Pragmas: The Version Should Be Locked

Severity: Non-Critical

Proof Of Concept

Found usage of floating pragmas ^0.8.14 of Solidity

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

(6) Implementation contract may not be initialized

OpenZeppelin recommends that the initializer modifier be applied to constructors. Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits. https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/5

Severity: Non-Critical

Proof Of Concept

contract VTVLVesting is Context, AccessProtected 

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

contract FullPremintERC20Token is ERC20 

https://github.com/code-423n4/2022-09-vtvl/tree/main/contracts/token/FullPremintERC20Token.sol#L8

contract VariableSupplyERC20Token is ERC20, AccessProtected 

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

(7) Public Functions Not Called By The Contract Should Be Declared External Instead

Severity: Non-Critical

Contracts are allowed to override their parents’ functions and change the visibility from external to public.

Proof Of Concept

function setAdmin(

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

function withdrawAdmin(

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

function mint(

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

(8) Commented Code

Severity: Non-Critical

Proof Of Concept

// require(_claim.linearVestAmount + _claim.cliffAmount > 0, "INVALID_VESTED_AMOUNT");
// require(_claim.endTimestamp > 0, "NO_END_TIMESTAMP");

https://github.com/code-423n4/2022-09-vtvl/tree/main/contracts/VTVLVesting.sol#L114-L115

// require(_claim.isActive == false, "CLAIM_ALREADY_EXISTS");

// Further checks aren't necessary (to save gas), as they're done at creation time (createClaim)
// require(_claim.endTimestamp == 0, "CLAIM_ALREADY_EXISTS");
// require(_claim.linearVestAmount + _claim.cliffAmount == 0, "CLAIM_ALREADY_EXISTS");
// require(_claim.amountWithdrawn == 0, "CLAIM_ALREADY_EXISTS");

https://github.com/code-423n4/2022-09-vtvl/tree/main/contracts/VTVLVesting.sol#L133-L138

// uint constant _initialSupply = 100 * (10**18);

https://github.com/code-423n4/2022-09-vtvl/tree/main/contracts/token/FullPremintERC20Token.sol#L9

(9) TODOs

Severity: Non-Critical

Proof Of Concept

// Potential TODO: sanity check, if _linearVestAmount == 0, should we perhaps force that start and end ts are the same?

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

(10) Non-usage of specific imports

Severity: Non-Critical

The current form of relative path import is not recommended for use because it can unpredictably pollute the namespace. Instead, the Solidity docs recommend specifying imported symbols explicitly. https://docs.soliditylang.org/en/v0.8.15/layout-of-source-files.html#importing-other-source-files

Proof of Concept

import "./AccessProtected.sol";

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

import "../AccessProtected.sol";

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

Recommended Mitigation Steps

Use specific imports syntax per solidity docs recommendation.

Total Gas Optimizations: 10

(1) Using Calldata Instead Of Memory For Read-only Arguments In External Functions Saves Gas

Severity: Gas Optimizations

When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Structs have the same overhead as an array of length one

Proof Of Concept

function allVestingRecipients() external view returns (address[] memory) {

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

function createClaimsBatch(
    address[] memory _recipients, 
    uint40[] memory _startTimestamps, 
    uint40[] memory _endTimestamps, 
    uint40[] memory _cliffReleaseTimestamps, 
    uint40[] memory _releaseIntervalsSecs, 
    uint112[] memory _linearVestAmounts, 
    uint112[] memory _cliffAmounts) 

https://github.com/code-423n4/2022-09-vtvl/tree/main/contracts/VTVLVesting.sol#L333-L340

(2) ++i Costs Less Gas Than i++, Especially When It’s Used In For-loops (--i/i-- Too)

Severity: Gas Optimizations

Saves 6 gas per loop

Proof Of Concept

for (uint256 i = 0; i < length; i++) {

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

Recommended Mitigation Steps

For example, use ++i instead of i++

(3) It Costs More Gas To Initialize Variables To Zero Than To Let The Default Of Zero Be Applied

Severity: Gas Optimizations

Proof Of Concept

for (uint256 i = 0; i < length; i++) {

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

(4) Using > 0 Costs More Gas Than != 0 When Used On A Uint In A Require() Statement

Severity: Gas Optimizations

This change saves 6 gas per instance

Proof Of Concept

require(_linearVestAmount + _cliffAmount > 0, "INVALID_VESTED_AMOUNT");

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

require(_startTimestamp > 0, "INVALID_START_TIMESTAMP");

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

require(_releaseIntervalSecs > 0, "INVALID_RELEASE_INTERVAL");

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

require(bal > 0, "INSUFFICIENT_BALANCE");

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

(5) <x> -=/+= <y> Costs More Gas Than <x> = <x> -/+ <y> For State Variables

Severity: Gas Optimizations

Proof Of Concept

vestAmt += _claim.cliffAmount;

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

vestAmt += linearVestAmount;

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

numTokensReservedForVesting += allocatedAmount;

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

usrClaim.amountWithdrawn += amountRemaining;

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

numTokensReservedForVesting -= amountRemaining;

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

numTokensReservedForVesting -= amountRemaining; // Reduces the allocation

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

mintableSupply -= amount;

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

(6) Public Functions To External

Severity: Gas Optimizations

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

Proof Of Concept

function setAdmin(address admin, bool isEnabled) public onlyAdmin {

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

function withdrawAdmin(uint112 _amountRequested) public onlyAdmin {    

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

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

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

(7) Use of non-uint64 state variable is less efficient than uint256

Severity: Gas Optimizations

Lower than uint256 size storage variables are less gas efficient. Using uint64 does not give any efficiency, actually, it is the opposite as EVM operates on default of 256-bit values so uint64 is more expensive in this case as it needs a conversion. It only gives improvements in cases where you can pack variables together, e.g. structs.

Proof Of Concept

uint112 public numTokensReservedForVesting = 0;

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

uint40 startTimestamp;

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

uint40 endTimestamp;

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

uint40 cliffReleaseTimestamp;

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

uint40 releaseIntervalSecs;

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

uint112 linearVestAmount;

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

uint112 cliffAmount;

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

uint112 amountWithdrawn;

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

uint112 vestAmt = 0;

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

uint40 currentVestingDurationSecs = _referenceTs - _claim.startTimestamp;

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

uint40 truncatedCurrentVestingDurationSecs = (currentVestingDurationSecs / _claim.releaseIntervalSecs) * _claim.releaseIntervalSecs;

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

uint40 finalVestingDurationSecs = _claim.endTimestamp - _claim.startTimestamp;

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

uint112 linearVestAmount = _claim.linearVestAmount * truncatedCurrentVestingDurationSecs / finalVestingDurationSecs;

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

uint112 allocatedAmount = _cliffAmount + _linearVestAmount;

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

uint112 allowance = vestedAmount(_msgSender(), uint40(block.timestamp));

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

uint112 amountRemaining = allowance - usrClaim.amountWithdrawn;

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

uint112 finalVestAmt = finalVestedAmount(_recipient);

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

uint112 amountRemaining = finalVestAmt - _claim.amountWithdrawn;

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

(8) ++i/i++ Should Be Unchecked{++i}/unchecked{i++} When It Is Not Possible For Them To Overflow, As Is The Case When Used In For- And While-loops

Severity: Gas Optimizations

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas PER LOOP

Proof Of Concept

for (uint256 i = 0; i < length; i++) {

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

(9) Using Bools For Storage Incurs Overhead

Severity: Gas Optimizations

Booleans are more expensive than uint256 or any type that takes up a full word because each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled.

Proof Of Concept

mapping(address => bool) private _admins;

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

(10) No need to check for true in require

Severity: Gas Optimizations

Since isActive is bool, there is no need to check for "true" value, can simply call: require(_claim.isActive, "NO_ACTIVE_CLAIM");

Proof Of Concept

require(_claim.isActive == true, "NO_ACTIVE_CLAIM");

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

Recommended Mitigation Steps

require(_claim.isActive, "NO_ACTIVE_CLAIM");