VTVL contest - seyni's results

Lines of code

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

Vulnerability details

Impact

The check if(mintableSupply > 0) is supposed to assess whether or not the token supply is capped. But mintableSupply is used to perform this check, which can be modified in the following logic mintableSupply -= amount. Eventually, when mintableSupply == 0 the check if(mintableSupply > 0) will not pass anymore and the administrator will be able to mint at will.

An administrator can mint at will even if the supply of the token is supposed to be capped.

An administrator could use this to trick the vesting recipients into thinking the mintableSupply is going to be fixed, and then proceed to rug the vesting recipients by diluting the token supply and withdrawing what is not vested.

Even accepting centralization in the design, I don't think this is a behaviour that is intended.

Proof of Concept

Link : https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/token/VariableSupplyERC20Token.sol#L40-L45

1st call to mint :

• mintableSupply == 1000
• amount == 1000

        if(1000 > 0) {
            require(1000 <= 1000, "INVALID_AMOUNT");
            // We need to reduce the amount only if we're using the limit, if not just leave it be
            mintableSupply -= amount; // @audit mintableSupply is set to 0

2nd call to mint :

• mintableSupply == 0
• amount == 10000

                if(0 > 0) { // @audit condition is false but amount > mintableSupply
                    require(amount <= mintableSupply, "INVALID_AMOUNT"); // @audit the check is never done
                    // We need to reduce the amount only if we're using the limit, if not just leave it be
                    mintableSupply -= amount;
                }
        _mint(account, amount); // @audit the administrator can mint even if the mintableSupply was originally capped.

Tools Used

Manual review.

Recommended Mitigation Steps

The original maxSupply_ need to be stored in a state variable in order to ensure that the if condition will always be true if the maximum mintable supply is capped. This ensure that the check require(amount <= mintableSupply, "INVALID_AMOUNT"); would cause a revert if the admistrator want to mint when the maximum mintable supply is reached.

The result would look like this :

        contract VariableSupplyERC20Token is ERC20, AccessProtected {
            uint256 public mintableSupply;
            uint256 public maxMintableSupply; // @audit state variable to store maxSupply_ in constructor


            /**
            @notice A ERC20 token contract that allows minting at will, with limited or unlimited supply. No burning possible
            @dev
            @param name_ - Name of the token
            @param symbol_ - Symbol of the token
            @param initialSupply_ - How much to immediately mint (saves another transaction). If 0, no mint at the beginning.
            @param maxSupply_ - What's the maximum supply. The contract won't allow minting over this amount. Set to 0 for no limit.
            */
            constructor(string memory name_, string memory symbol_, uint256 initialSupply_, uint256 maxSupply_) ERC20(name_, symbol_) {
                // max supply == 0 means mint at will.
                // initialSupply_ == 0 means nothing preminted
                // Therefore, we have valid scenarios if either of them is 0
                // However, if both are 0 we might have a valid scenario as well - user just wants to create a token but doesn't want to mint anything
                // Should we allow this?
                require(initialSupply_ > 0 || maxSupply_ > 0, "INVALID_AMOUNT");
                mintableSupply = maxSupply_;
                maxMintableSupply = maxSupply_; // @audit keep the information if whether or not maxSupply_ is capped

                // Note: the check whether initial supply is less than or equal than mintableSupply will happen in mint fn.
                if(initialSupply_ > 0) {
                    mint(_msgSender(), initialSupply_);
                }
            }


            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(maxMintableSupply > 0) { // @audit this will always be true if the maximum mintable supply is capped.
                    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);
            }


            // We can't really have burn, because that could make our vesting contract not work.
            // Example: if the user can burn tokens already assigned to vesting schedules, it could be unable to pay its obligations.
    }

Also, an additionnal check could be added to prevent all the logic (if logic and _mint) to execute when amount == 0.

QA

Low Severity

[L-01] Typos in comments and wrong comments

Files links :

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

Findings :

token/VariableSupplyERC20Token.sol:28:        // max supply == 0 means mint at will.

Instead of :

token/VariableSupplyERC20Token.sol:28:        // maxSupply_ == 0 means mint at will.

token/VariableSupplyERC20Token.sol:49:        // mintableSupply = 0 means mint at will

Instead of :

token/VariableSupplyERC20Token.sol:49:        // mintableSupply == 0 means mint at will

2022-09-vtvl/contracts/VTVLVesting.sol:373:        // Make sure we didn't already withdraw more that we're allowed.

Instead of :

2022-09-vtvl/contracts/VTVLVesting.sol:373:        // Make sure we don't withdraw more that we're allowed.

token/VariableSupplyERC20Token.sol:31:        // However, if both are 0 we might have a valid scenario as well - user just wants to create a token but doesn't want to mint anything

• if both are 0 : maxSupply_ == 0 and initialSupply_ == 0 => mint at will and nothing preminted, which is fine.

Which means the comment above 4. is wrong because they can mint at will when maxSupply_ == 0.

If maxSupply_ == 0 means mint at will as mentionned in the following comment :

token/VariableSupplyERC20Token.sol:28:        // max supply == 0 means mint at will.

It has for consequence that there is no scenarios in which the tokens are not mintable.

To answer the question in comment :

token/VariableSupplyERC20Token.sol:36:        // Should we allow this?

All the others scenarios :

• if both are != 0 : maxSupply_ != 0 and initialSupply_ != 0 => mint capped by maxSupply_ and preminted amount, which is fine if maxSupply_ >= initialSupply_.

• if maxSupply_ != 0 and initialSupply_ == 0 => mint capped by maxSupply_ and no preminted amount, which is fine.

• if maxSupply_ == 0 and initialSupply_ != 0 => mint at will and preminted amount, which is fine.

Non-critical

[N-01] Floating pragma compiler version

Contracts should not use a floating pragma in order to ensure that the code has been tested and deployed with the same version.

Files links :

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

Findings :

2022-09-vtvl/contracts/token/VariableSupplyERC20Token.sol::2 	 pragma solidity ^0.8.14;

[N-02] Functions should be declared after modifiers

Files links :

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

Findings :

2022-09-vtvl/contracts/VTVLVesting.sol:91:    function getClaim(address _recipient) external view returns (Claim memory) {

[N-03] Missing address(0) check

An address(0) check could be added in the function VTVLVesting.withdrawOtherToken to the input address _otherTokenAddress.

Files links :

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

Findings :

2022-09-vtvl/contracts/VTVLVesting.sol:446:     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);
                                                }

Gas Optimizations

[G-01] Useless explicit initialization of variable to default value

Explicitly initializing a variable with its default value wastes gas.

Files links :

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

Findings :

2022-09-vtvl/contracts/VTVLVesting.sol::27 	 uint112 public numTokensReservedForVesting = 0;
2022-09-vtvl/contracts/VTVLVesting.sol::148 	 uint112 vestAmt = 0;
2022-09-vtvl/contracts/VTVLVesting.sol::353 	 for (uint256 i = 0; i < length; i++) {

[G-02] i++ instead of ++i

i++ costs 5 more gas per loop than ++i.

Files links :

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

Findings :

2022-09-vtvl/contracts/VTVLVesting.sol::353 	 for (uint256 i = 0; i < length; i++) {

[G-03] Inline unchecked{++i;} could be used

The increment of a loop can be inlined and unchecked since there is no risk of overflow/underflow, which costs less gas.

Files links :

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

Findings :

2022-09-vtvl/contracts/VTVLVesting.sol::353 	 for (uint256 i = 0; i < length; i++) {

[G-04] Boolean comparison

Using if(bool) or if(!bool) instead of if(bool == true) or if(bool == false) costs less gas.

Files links :

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

Findings :

2022-09-vtvl/contracts/VTVLVesting.sol::111 	 require(_claim.isActive == true, "NO_ACTIVE_CLAIM");

[G-05] Use custom errors instead of revert string

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met).

Files links :

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

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

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

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

Findings :

2022-09-vtvl/contracts/AccessProtected.sol::25 	 require(_admins[_msgSender()], "ADMIN_ACCESS_REQUIRED");
2022-09-vtvl/contracts/AccessProtected.sol::40 	 require(admin != address(0), "INVALID_ADDRESS");
2022-09-vtvl/contracts/VTVLVesting.sol::82 	 require(address(_tokenAddress) != address(0), "INVALID_ADDRESS");
2022-09-vtvl/contracts/VTVLVesting.sol::107 	 require(_claim.startTimestamp > 0, "NO_ACTIVE_CLAIM");
2022-09-vtvl/contracts/VTVLVesting.sol::111 	 require(_claim.isActive == true, "NO_ACTIVE_CLAIM");
2022-09-vtvl/contracts/VTVLVesting.sol::129 	 require(_claim.startTimestamp == 0, "CLAIM_ALREADY_EXISTS");
2022-09-vtvl/contracts/VTVLVesting.sol::255 	 require(_recipient != address(0), "INVALID_ADDRESS");
2022-09-vtvl/contracts/VTVLVesting.sol::256 	 require(_linearVestAmount + _cliffAmount > 0, "INVALID_VESTED_AMOUNT"); // Actually only one of linearvested/cliff amount must be 0, not necessarily both
2022-09-vtvl/contracts/VTVLVesting.sol::257 	 require(_startTimestamp > 0, "INVALID_START_TIMESTAMP");
2022-09-vtvl/contracts/VTVLVesting.sol::262 	 require(_startTimestamp < _endTimestamp, "INVALID_END_TIMESTAMP"); // _endTimestamp must be after _startTimestamp
2022-09-vtvl/contracts/VTVLVesting.sol::263 	 require(_releaseIntervalSecs > 0, "INVALID_RELEASE_INTERVAL");
2022-09-vtvl/contracts/VTVLVesting.sol::264 	 require((_endTimestamp - _startTimestamp) % _releaseIntervalSecs == 0, "INVALID_INTERVAL_LENGTH");
2022-09-vtvl/contracts/VTVLVesting.sol::270 	 require(
                                                    (
                                                        _cliffReleaseTimestamp > 0 &&
                                                        _cliffAmount > 0 &&
                                                        _cliffReleaseTimestamp <= _startTimestamp
                                                    ) || (
                                                        _cliffReleaseTimestamp == 0 &&
                                                        _cliffAmount == 0
                                                ), "INVALID_CLIFF");
2022-09-vtvl/contracts/VTVLVesting.sol::295 	 require(tokenAddress.balanceOf(address(this)) >= numTokensReservedForVesting + allocatedAmount, "INSUFFICIENT_BALANCE");
2022-09-vtvl/contracts/VTVLVesting.sol::344 	 require(_startTimestamps.length == length &&
2022-09-vtvl/contracts/VTVLVesting.sol::374 	 require(allowance > usrClaim.amountWithdrawn, "NOTHING_TO_WITHDRAW");
2022-09-vtvl/contracts/VTVLVesting.sol::402 	 require(amountRemaining >= _amountRequested, "INSUFFICIENT_BALANCE");
2022-09-vtvl/contracts/VTVLVesting.sol::426 	 require( _claim.amountWithdrawn < finalVestAmt, "NO_UNVESTED_AMOUNT");
2022-09-vtvl/contracts/VTVLVesting.sol::447 	 require(_otherTokenAddress != tokenAddress, "INVALID_TOKEN"); // tokenAddress address is already sure to be nonzero due to constructor
2022-09-vtvl/contracts/VTVLVesting.sol::449 	 require(bal > 0, "INSUFFICIENT_BALANCE");
2022-09-vtvl/contracts/token/FullPremintERC20Token.sol::11 	 require(supply_ > 0, "NO_ZERO_MINT");
2022-09-vtvl/contracts/token/VariableSupplyERC20Token.sol::27 	 require(initialSupply_ > 0 || maxSupply_ > 0, "INVALID_AMOUNT");
2022-09-vtvl/contracts/token/VariableSupplyERC20Token.sol::37 	 require(account != address(0), "INVALID_ADDRESS");
2022-09-vtvl/contracts/token/VariableSupplyERC20Token.sol::41 	 require(amount <= mintableSupply, "INVALID_AMOUNT");

[G-06] Operations could be unchecked

The following operations could be unchecked to save gas, because if(_referenceTs > _claim.startTimestamp) (in _baseVestedAmount) and require(_startTimestamp < _endTimestamp, "INVALID_END_TIMESTAMP") (when creating a claim) checks are done.

Files links :

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

Findings :

2022-09-vtvl/contracts/VTVLVesting.sol:167:      uint40 currentVestingDurationSecs = _referenceTs - _claim.startTimestamp; // How long since the start
2022-09-vtvl/contracts/VTVLVesting.sol:170:      uint40 finalVestingDurationSecs = _claim.endTimestamp - _claim.startTimestamp; // length of the interval

[G-07] _baseVestedAmount(_claim, _claim.endTimestamp) could be called instead of finalVestedAmount(_recipient)

Calling _baseVestedAmount(_claim, _claim.endTimestamp) instead of finalVestedAmount(_recipient) cost significantly less gas because an additional read from storage is done in finalVestedAmount.

I agree with the comment that says that the function finalVestedAmountshould be removed.

Files links :

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

Findings :

2022-09-vtvl/contracts/VTVLVesting.sol:422:     uint112 finalVestAmt = finalVestedAmount(_recipient);

2022-09-vtvl/contracts/VTVLVesting.sol:206:     function finalVestedAmount(address _recipient) public view returns (uint112) {
                                                    Claim storage _claim = claims[_recipient];
                                                    return _baseVestedAmount(_claim, _claim.endTimestamp);
                                                }