VTVL contest - brgltd's results

[NC-01] Use the latest version of solidity and OpenZeppelin and avoid floating the pragma

The contract VariableSupplyERC20Token.sol is floating the pragma version. If the contract is not intended to be used as library, floating the pragma should be avoided to ensure that contracts do not accidentally get deployed using an outdated compiler version.

Also, the project is using OpenZeppelin v4.5.0. The latest stable version of OpenZeppelin (at 23 Sep. 2022) is 4.7.3.

Furthermore, the following contracts are using solidity v0.8.14.

FullPremintERC20Token.sol,
AccessProtected.sol,
VTVLVesting.sol

Consider updating solidity to 0.8.15 or 0.8.16 and also updating OpenZeppelin to ensure the latest security fixes at the compiler and library level are included in the contracts.

[NC-02] Emit an event for critical functions

The function withdrawOtherToken could emit an event when a succesful transfer occurs to facilitade off chain monitoring.

function withdrawOtherToken(IERC20 _otherTokenAddress) external onlyAdmin {
    require(_otherTokenAddress != tokenAddress, "INVALID_TOKEN"); // tokenAddress address is already sure to be nonzero due to constructor
    uint256 bal = _otherTokenAddress.balanceOf(address(this));
    require(bal > 0, "INSUFFICIENT_BALANCE");
    _otherTokenAddress.safeTransfer(_msgSender(), bal);
}

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

[NC-03] Replace memory with calldata for read-only, non-primitve function arguments

function _baseVestedAmount(Claim memory _claim, uint40 _referenceTs) internal pure returns (uint112) {

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

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

Calldata is recommended for arrays and structs when the inputs are not modified. This can also yield gas savings.

[NC-04] Missing NATSPEC

function mint(address account, uint256 amount) public onlyAdmin {
    require(account != address(0), "INVALID_ADDRESS");
    // If we're using maxSupply, we need to make sure we respect it
    // mintableSupply = 0 means mint at will
    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;
    }
    _mint(account, amount);
}

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

[NC-05] TODOs should be resolved before deployment

// 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/blob/main/contracts/VTVLVesting.sol

[NC-06] Replace public with external for functions not called by the contract

function withdrawAdmin(uint112 _amountRequested) public onlyAdmin {    

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

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

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

[NC-07] Repeated validation can be refactored into a single modifier

The checks for validating an input address against address zero can be refactored into a single modifier to improve code reusability.

$ git diff --no-index VTVLVesting.sol.orig VTVLVesting.sol
diff --git a/VTVLVesting.sol.orig b/VTVLVesting.sol
index 3e10653..42e95b8 100644
--- a/VTVLVesting.sol.orig
+++ b/VTVLVesting.sol
@@ -73,13 +73,17 @@ contract VTVLVesting is Context, AccessProtected {

+    modifier addrNotZero(address _addr) {
+        require(_addr != address(0), "zero address");
+        _;
+    }
+
     //
     /**
     @notice Construct the contract, taking the ERC20 token to be vested as the parameter.
     @dev The owner can set the contract in question when creating the contract.
      */
-    constructor(IERC20 _tokenAddress) {
-        require(address(_tokenAddress) != address(0), "INVALID_ADDRESS");
+    constructor(IERC20 _tokenAddress) addrNotZero(_tokenAddress) {
         tokenAddress = _tokenAddress;
     }

@@ -250,9 +254,8 @@ contract VTVLVesting is Context, AccessProtected {
             uint40 _releaseIntervalSecs,
             uint112 _linearVestAmount,
             uint112 _cliffAmount
-                ) private  hasNoClaim(_recipient) {
+                ) private  hasNoClaim(_recipient) addrNotZero(_recipient) {

-        require(_recipient != address(0), "INVALID_ADDRESS");
         require(_linearVestAmount + _cliffAmount > 0, "INVALID_VESTED_AMOUNT"); // Actually only one of linearvested/cliff amount must be 0, not necessarily both
         require(_startTimestamp > 0, "INVALID_START_TIMESTAMP");
         // Do we need to check whether _startTimestamp is greater than the current block.timestamp?

[NC-08] Limit the number of maximum characters per line

Some lines have 120+ characters. Setting a maximum width per line would improve code readbility.

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

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

event ClaimRevoked(address indexed _recipient, uint112 _numTokensWithheld, uint256 revocationTimestamp, Claim _claim);

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

[NC-09] Normalize the spacing for functions with multiple arguments

function createclaim(
        address _recipient, 
        uint40 _starttimestamp, 
        uint40 _endtimestamp, 
        uint40 _cliffreleasetimestamp, 
        uint40 _releaseintervalsecs, 
        uint112 _linearvestamount, 
        uint112 _cliffamount
            ) external onlyadmin {

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

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

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

Note that createClaim has twice as many spaces than createClaimBatch for it's function arguments. Also note that the ending parenthesis is not following the same pattern.

Consider using only one coding style, ideally with the usage of a formatter and linter (e.g. prettier and solhint).

[G-01] Prefix increment costs less gas than postfix increment

There's 1 instance of this issue.

File: contracts/VTVLVesting.sol
353: for (uint256 i = 0; i < length; i++) {

[G-02] The increment in for loop post condition can be made unchecked to save gas

There's 1 instance of this issue.

File: contracts/VTVLVesting.sol
353: for (uint256 i = 0; i < length; i++) {

[G-03] Initializing a variable with the default value wastes gas

There are 2 instances of this issue.

File: contracts/VTVLVesting.sol
148: uint112 vestAmt = 0;
353: for (uint256 i = 0; i < length; i++) {

[G-04] Use != 0 instead of > 0 to save gas

Replace > 0 with != 0 for unsigned integers.

There are 11 instances of this issue.

File: contracts/VTVLVesting.sol
107: require(_claim.startTimestamp > 0, "NO_ACTIVE_CLAIM");
256: require(_linearVestAmount + _cliffAmount > 0, "INVALID_VESTED_AMOUNT"); // Actually only one of linearvested/cliff amount must be 0, not necessarily both
257: require(_startTimestamp > 0, "INVALID_START_TIMESTAMP");
263: require(_releaseIntervalSecs > 0, "INVALID_RELEASE_INTERVAL");
272: _cliffReleaseTimestamp > 0 &&
273: _cliffAmount > 0 &&
449: require(bal > 0, "INSUFFICIENT_BALANCE");

File: contracts/token/FullPremintERC20Token.sol
11: require(supply_ > 0, "NO_ZERO_MINT");

File: contracts/token/VariableSupplyERC20Token.sol
27: require(initialSupply_ > 0 || maxSupply_ > 0, "INVALID_AMOUNT");
31: if(initialSupply_ > 0) {
40: if(mintableSupply > 0) {

[G-05] Use custom errors rather than require/revert strings to save gas

There are 21 instances of this issue.

File: contracts/AccessProtected.sol
25: require(_admins[_msgSender()], "ADMIN_ACCESS_REQUIRED");
40: require(admin != address(0), "INVALID_ADDRESS");

File: contracts/VTVLVesting.sol
82: require(address(_tokenAddress) != address(0), "INVALID_ADDRESS");
107: require(_claim.startTimestamp > 0, "NO_ACTIVE_CLAIM");
111: require(_claim.isActive == true, "NO_ACTIVE_CLAIM");
255: require(_recipient != address(0), "INVALID_ADDRESS");
256: require(_linearVestAmount + _cliffAmount > 0, "INVALID_VESTED_AMOUNT"); // Actually only one of linearvested/cliff amount must be 0, not necessarily both
257: require(_startTimestamp > 0, "INVALID_START_TIMESTAMP");
262: require(_startTimestamp < _endTimestamp, "INVALID_END_TIMESTAMP"); // _endTimestamp must be after _startTimestamp
263: require(_releaseIntervalSecs > 0, "INVALID_RELEASE_INTERVAL");
264: require((_endTimestamp - _startTimestamp) % _releaseIntervalSecs == 0, "INVALID_INTERVAL_LENGTH");
295: require(tokenAddress.balanceOf(address(this)) >= numTokensReservedForVesting + allocatedAmount, "INSUFFICIENT_BALANCE");
374: require(allowance > usrClaim.amountWithdrawn, "NOTHING_TO_WITHDRAW");
402: require(amountRemaining >= _amountRequested, "INSUFFICIENT_BALANCE");
426: require( _claim.amountWithdrawn < finalVestAmt, "NO_UNVESTED_AMOUNT");
447: require(_otherTokenAddress != tokenAddress, "INVALID_TOKEN"); // tokenAddress address is already sure to be nonzero due to constructor
449: require(bal > 0, "INSUFFICIENT_BALANCE");

File: contracts/token/FullPremintERC20Token.sol
11: require(supply_ > 0, "NO_ZERO_MINT");

File: contracts/token/VariableSupplyERC20Token.sol
27: require(initialSupply_ > 0 || maxSupply_ > 0, "INVALID_AMOUNT");
37: require(account != address(0), "INVALID_ADDRESS");
41: require(amount <= mintableSupply, "INVALID_AMOUNT");

[G-06] x += y costs more gas than x = x + y for state variables

There are 5 instances of this issue.

File: contracts/VTVLVesting.sol
301: numTokensReservedForVesting += allocatedAmount; // track the allocated amount
381: usrClaim.amountWithdrawn += amountRemaining;
383: numTokensReservedForVesting -= amountRemaining;
433: numTokensReservedForVesting -= amountRemaining; // Reduces the allocation

File: contracts/token/VariableSupplyERC20Token.sol
43: mintableSupply -= amount;

[G-07] Don’t compare boolean expressions to boolean literals

There's 1 instance of this issue.

File: contracts/VTVLVesting.sol
111: require(_claim.isActive == true, "NO_ACTIVE_CLAIM");