VTVL contest - Aymen0909's results

QA Report

Summary

Findings

1- numTokensReservedForVesting could overflow for certain ERC20 tokens :

The numTokensReservedForVesting state variable uses uint112 which has a maximum value of 2**112 = 5.1922969e+33, this value could be not sufficient as both cliffAmount and linearVestAmount also uses uint112, so if both those variable get a big value their sum could be greater that 2**112 which will lead to the vesting contract to be unable of creating new claims.

This issue could occur for example if we take an ERC20 token with 18 decimals and doesn't cost a lot in term of USD let say 1 token = 0.000000001 USD (which is possible with new ERC20 tokens) so the values of both cliffAmount and linearVestAmount variable could be very close to the uint112 limit and thus their sum would be greater than 2**112 .

Impact - Low

Proof of Concept

Instances include:

File: contracts/VTVLVesting.sol Line 292-301

uint112 allocatedAmount = _cliffAmount + _linearVestAmount;

numTokensReservedForVesting += allocatedAmount;

Mitigation

Consider using uint256 for the variable numTokensReservedForVesting to avoid any overflow risks, or make sure to pick ERC20 tokens which doesn't have very low prices.

2- Openzeppelin Ownable extension imported but not used :

The Openzeppelin Ownable extension is imported in both the VTVLVesting and AccessProtected contracts but none of them has inherited it, maybe the import statement was just forgotten or left for later use if not it should be removed.

Note that the AccessProtected contract is itself acting as an access control contract, so if Ownable extension is set to be used in the future devs must make sure to correctely manage both those access control extensions.

Impact - NON CRITICAL

Proof of Concept

Instances include:

File: contracts/VTVLVesting.sol

import "@openzeppelin/contracts/access/Ownable.sol";

File: contracts/AccessProtected.sol

import "@openzeppelin/contracts/access/Ownable.sol";

Mitigation

Remove the Openzeppelin Ownable import statement if it's not intended to be used.

3- Redundant ZERO address check inside the mint function :

The _mint function from the Openzeppelin ERC20 contract already contains a check to verify that the reciever account is not the zero address, so it's redundant to add another check in the mint function as it will call the _mint function which will revert if account == address(0).

_mint function from the Openzeppelin ERC20 contract

function _mint(address account, uint256 amount) internal virtual {
    require(account != address(0), "ERC20: mint to the zero address");

    _beforeTokenTransfer(address(0), account, amount);

    _totalSupply += amount;
    unchecked {
        // Overflow not possible: balance + amount is at most totalSupply + amount, which is checked above.
        _balances[account] += amount;
    }
    emit Transfer(address(0), account, amount);

    _afterTokenTransfer(address(0), account, amount);
}

Impact - NON CRITICAL

Proof of Concept

Instances occurs in :

File: contracts/token/VariableSupplyERC20Token.sol

require(account != address(0), "INVALID_ADDRESS");

Mitigation

Remove the zero address check aforementioned as it's redundant.

4- Non-library/interface files should use fixed compiler versions, not floating ones :

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

check this source : https://swcregistry.io/docs/SWC-103

Impact - NON CRITICAL

Proof of Concept

The VariableSupplyERC20Token contract uses a floating solidity version in contrary to others contracts:

pragma solidity ^0.8.14;

Mitigation

Lock the pragma version to the same version as used in the other contracts and also consider known bugs (https://github.com/ethereum/solidity/releases) for the compiler version that is chosen.

Pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or EthPM package. Otherwise, the developer would need to manually update the pragma in order to compile it locally.

Gas Optimizations

Findings Summary

Savings Summary

• Total gas saved : 3168 gas

Findings

1- Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead (saves ~1127 gas):

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size as you can check here.

So use uint256/int256 for state variables and then downcast to lower sizes where needed.

There is 1 instance of this issue:

File: contracts/VTVLVesting.sol Line 27

uint112 public numTokensReservedForVesting = 0;

• Gas savings : in total 1127 gas saved

2- Use unchecked blocks to save gas (saves ~1164 gas) :

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

There are 5 instances of this issue:

File: contracts/VTVLVesting.sol Line 167

uint40 currentVestingDurationSecs = _referenceTs - _claim.startTimestamp;

The above operation cannot underflow due to the check _referenceTs > _claim.startTimestamp and should be modified as follows :

uint40 currentVestingDurationSecs;
unchecked {
    currentVestingDurationSecs = _referenceTs - _claim.startTimestamp;
}

File: contracts/VTVLVesting.sol Line 170

uint40 finalVestingDurationSecs = _claim.endTimestamp - _claim.startTimestamp;

The above operation cannot underflow due to the check require(_startTimestamp < _endTimestamp, "INVALID_END_TIMESTAMP"); and should be modified as follows :

uint40 finalVestingDurationSecs;
unchecked {
    finalVestingDurationSecs = _claim.endTimestamp - _claim.startTimestamp;
}

File: contracts/VTVLVesting.sol Line 377

uint112 amountRemaining = allowance - usrClaim.amountWithdrawn;

The above operation cannot underflow due to the check require(allowance > usrClaim.amountWithdrawn, "NOTHING_TO_WITHDRAW"); and should be modified as follows :

uint112 amountRemaining;
unchecked {
    amountRemaining = allowance - usrClaim.amountWithdrawn;
}

File: contracts/VTVLVesting.sol Line 429

uint112 amountRemaining = finalVestAmt - _claim.amountWithdrawn;

The above operation cannot underflow due to the check require(_claim.amountWithdrawn < finalVestAmt, "NO_UNVESTED_AMOUNT"); and should be modified as follows :

uint112 amountRemaining;
unchecked {
    amountRemaining = finalVestAmt - _claim.amountWithdrawn;
}

File: contracts/token/VariableSupplyERC20Token.sol Line 43

mintableSupply -= amount;

The above operation cannot underflow due to the check require(amount <= mintableSupply, "INVALID_AMOUNT"); and should be modified as follows :

unchecked {
    mintableSupply -= amount;
}

• Gas savings : in total 1164 gas saved

3- Cache storage values in memory to minimize SLOADs (saves ~250 gas):

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive 100 gas compared to MLOADs/MSTOREs(3 gas) Storage value should get cached in memory

There are 2 instances of this issue:

File: contracts/VTVLVesting.sol function withdraw

withdraw(): usrClaim.amountWithdrawn should be cached (Saves ~ 129 gas) because it's value is read twice meaning 2 SLOAD

The withdraw function shoud be modified as follow :

    function withdraw() hasActiveClaim(_msgSender()) external {
        // Get the message sender claim - if any

        Claim storage usrClaim = claims[_msgSender()];

        // we can use block.timestamp directly here as reference TS, as the function itself will make sure to cap it to endTimestamp
        // Conversion of timestamp to uint40 should be safe since 48 bit allows for a lot of years.
        uint112 allowance = vestedAmount(_msgSender(), uint40(block.timestamp));

        // @audit : cach usrClaim.amountWithdrawn into memory variable
        uint112 _amountWithdrawn = usrClaim.amountWithdrawn;

        // Make sure we didn't already withdraw more that we're allowed.
        require(allowance > _amountWithdrawn, "NOTHING_TO_WITHDRAW");

        // Calculate how much can we withdraw (equivalent to the above inequality)
        uint112 amountRemaining = allowance - _amountWithdrawn;

        // "Double-entry bookkeeping"
        // Carry out the withdrawal by noting the withdrawn amount, and by transferring the tokens.
        usrClaim.amountWithdrawn += amountRemaining;
        // Reduce the allocated amount since the following transaction pays out so the "debt" gets reduced
        numTokensReservedForVesting -= amountRemaining;
        
        // After the "books" are set, transfer the tokens
        // Reentrancy note - internal vars have been changed by now
        // Also following Checks-effects-interactions pattern
        tokenAddress.safeTransfer(_msgSender(), amountRemaining);

        // Let withdrawal known to everyone.
        emit Claimed(_msgSender(), amountRemaining);
    }

File: contracts/VTVLVesting.sol function revokeClaim

revokeClaim(address _recipient): _claim.amountWithdrawn should be cached (Saves ~ 121 gas) because it's value is read twice meaning 2 SLOAD

The revokeClaim function shoud be modified as follow :

    function revokeClaim(address _recipient) external onlyAdmin hasActiveClaim(_recipient) {
        // Fetch the claim
        Claim storage _claim = claims[_recipient];
        // Calculate what the claim should finally vest to
        uint112 finalVestAmt = finalVestedAmount(_recipient);
        
        // @audit : cach _claim.amountWithdrawn into memory variable
        uint112 _amountWithdrawn = _claim.amountWithdrawn;

        // No point in revoking something that has been fully consumed
        // so require that there be unconsumed amount
        require( _amountWithdrawn < finalVestAmt, "NO_UNVESTED_AMOUNT");

        // The amount that is "reclaimed" is equal to the total allocation less what was already withdrawn
        uint112 amountRemaining = finalVestAmt - _amountWithdrawn;

        // Deactivate the claim, and release the appropriate amount of tokens
        _claim.isActive = false;    // This effectively reduces the liability by amountRemaining, so we can reduce the liability numTokensReservedForVesting by that much
        numTokensReservedForVesting -= amountRemaining; // Reduces the allocation

        // Tell everyone a claim has been revoked.
        emit ClaimRevoked(_recipient, amountRemaining, uint40(block.timestamp), _claim);
    }

• Gas savings : in total 250 gas saved

4- x += y/x -= y costs more gas than x = x + y/x = x - y for state variables (saves ~45 gas) :

There are 5 instances of this issue:

File: contracts/VTVLVesting.sol Line 301

numTokensReservedForVesting += allocatedAmount;

File: contracts/VTVLVesting.sol Line 381

usrClaim.amountWithdrawn += amountRemaining;

File: contracts/VTVLVesting.sol Line 383

numTokensReservedForVesting -= amountRemaining;

File: contracts/VTVLVesting.sol Line 433

numTokensReservedForVesting -= amountRemaining;

File: contracts/token/VariableSupplyERC20Token.sol Line 43

mintableSupply -= amount;

• Gas savings : in total 45 gas saved

5- Use custom errors rather than require()/revert() strings to save deployments gas :

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information.

There are 24 instances of this issue :

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");
129      require(_claim.startTimestamp == 0, "CLAIM_ALREADY_EXISTS");
255      require(_recipient != address(0), "INVALID_ADDRESS");
256      require(_linearVestAmount + _cliffAmount > 0, "INVALID_VESTED_AMOUNT"); 
257      require(_startTimestamp > 0, "INVALID_START_TIMESTAMP");
262      require(_startTimestamp < _endTimestamp, "INVALID_END_TIMESTAMP");
263      require(_releaseIntervalSecs > 0, "INVALID_RELEASE_INTERVAL"); 
264      require((_endTimestamp - _startTimestamp) % _releaseIntervalSecs == 0, "INVALID_INTERVAL_LENGTH");
270      require( 
            (
                _cliffReleaseTimestamp > 0 && 
                _cliffAmount > 0 && 
                _cliffReleaseTimestamp <= _startTimestamp
            ) || (
                _cliffReleaseTimestamp == 0 && 
                _cliffAmount == 0
        ), "INVALID_CLIFF");
295      require(tokenAddress.balanceOf(address(this)) >= numTokensReservedForVesting + allocatedAmount, "INSUFFICIENT_BALANCE");
344      require(_startTimestamps.length == length &&
                _endTimestamps.length == length &&
                _cliffReleaseTimestamps.length == length &&
                _releaseIntervalsSecs.length == length &&
                _linearVestAmounts.length == length &&
                _cliffAmounts.length == length, 
                "ARRAY_LENGTH_MISMATCH"
        );
374      require(allowance > usrClaim.amountWithdrawn, "NOTHING_TO_WITHDRAW");
402      require(amountRemaining >= _amountRequested, "INSUFFICIENT_BALANCE"); 
429      require( _claim.amountWithdrawn < finalVestAmt, "NO_UNVESTED_AMOUNT"); 
447      require(_otherTokenAddress != tokenAddress, "INVALID_TOKEN"); 
449      require(bal > 0, "INSUFFICIENT_BALANCE");

File: contracts/AccessProtected.sol

25       require(_admins[_msgSender()], "ADMIN_ACCESS_REQUIRED"); 
40       require(admin != address(0), "INVALID_ADDRESS"); 

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"); 

6- Splitting require() statements that uses && saves gas (saves 8 gas per &&) :

There are 2 instances of this issue :

File: contracts/VTVLVesting.sol

270      require( 
            (
                _cliffReleaseTimestamp > 0 && 
                _cliffAmount > 0 && 
                _cliffReleaseTimestamp <= _startTimestamp
            ) || (
                _cliffReleaseTimestamp == 0 && 
                _cliffAmount == 0
        ), "INVALID_CLIFF");
344      require(_startTimestamps.length == length &&
                _endTimestamps.length == length &&
                _cliffReleaseTimestamps.length == length &&
                _releaseIntervalsSecs.length == length &&
                _linearVestAmounts.length == length &&
                _cliffAmounts.length == length, 
                "ARRAY_LENGTH_MISMATCH"
        ); 

7- It costs more gas to initialize variables to zero than to let the default of zero be applied (saves ~3 gas per instance) :

If a variable is not set/initialized, it is assumed to have the default value (0 for uint or int, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

There are 3 instances of this issue:

File: contracts/VTVLVesting.sol Line 27

uint112 public numTokensReservedForVesting = 0;

File: contracts/VTVLVesting.sol Line 148

uint112 vestAmt = 0;

File: contracts/VTVLVesting.sol Line 353

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

8- Use of ++i cost less gas than i++/i=i+1 in for loops (saves ~10 gas) :

There is 1 instance of this issue:

File: contracts/VTVLVesting.sol Line 353

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

• Gas savings : in total 1164 gas saved

9- ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as in the case when used in for & while loops (saves ~240 gas) :

There is 1 instance of this issue:

File: contracts/VTVLVesting.sol Line 353

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

• Gas savings : in total 240 gas saved

10- Functions guaranteed to revert when called by normal users can be marked payable (saves ~393 gas) :

If a function modifier such as onlyAdmin is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for the owner because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are :

CALLVALUE(gas=2), DUP1(gas=3), ISZERO(gas=3), PUSH2(gas=3), JUMPI(gas=10), PUSH1(gas=3), DUP1(gas=3), REVERT(gas=0), JUMPDEST(gas=1), POP(gas=2). Which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

There are 6 instances of this issue:

File: contracts/VTVLVesting.sol

317     function createClaim(
            address _recipient, 
            uint40 _startTimestamp, 
            uint40 _endTimestamp, 
            uint40 _cliffReleaseTimestamp, 
            uint40 _releaseIntervalSecs, 
            uint112 _linearVestAmount, 
            uint112 _cliffAmount
                ) external onlyAdmin
333     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
398     function withdrawAdmin(uint112 _amountRequested) public onlyAdmin
418     function revokeClaim(address _recipient) external onlyAdmin hasActiveClaim(_recipient)
446     function withdrawOtherToken(IERC20 _otherTokenAddress) external onlyAdmin

File: contracts/AccessProtected.sol

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

• Gas savings : in total 393 gas saved