VTVL contest - __141345__'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 maxSupply_ and mintableSupply are used to set the maximum supply. But the totalSupply can still be increased when mintableSupply is 0 even a maximum supply has been set. The token supply accounting will be break. The malicious admin or if the contract is compromised, unlimited token can be minted to dilute the value of all the users, effectively steal their funds.

Proof of Concept

When maxSupply_ > 0 and all maxSupply_ has been minted already, mintableSupply will be 0, the if (mintableSupply > 0) check will be skipped, but the next line _mint() can still execute, making the maxSupply_ and mintableSupply useless.

// contracts/token/VariableSupplyERC20Token.sol
    function mint(address account, uint256 amount) public onlyAdmin {
        // ...
        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);
    }

Tools Used

Manual analysis.

Recommended Mitigation Steps

Add another state variable to record the maximum supply. And add more if blocks in mint() to determine the actions accordingly.

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L295 https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L388

Vulnerability details

Impact

Some ERC20 token's balance could change, one example is stETH. The balance could become insufficient at the time of withdraw(). User's fund will be locked due to DoS. The way to take the fund out is to send more token into the contract, causing fund loss to the protocol. And there is no guarantee that until the end time the balance would stay above the needed amount, the lock and loss issue persist.

Proof of Concept

For stETH like tokens, the balanceOf() value might go up or down, even without transfer.

// stETH
    function balanceOf(address who) external override view returns (uint256) {
        return _shareBalances[who].div(_sharesPerToken);
    }

In VTVLVesting, the require check for the spot balanceOf() value will pass, but it is possible that as time goes on, the value become smaller and fail the transfer. As a result, the withdraw() call will revert, causing DoS, and lock user's fund.

// contracts/VTVLVesting.sol
    function _createClaimUnchecked() private  hasNoClaim(_recipient) {
        // ...
        require(tokenAddress.balanceOf(address(this)) >= numTokensReservedForVesting + allocatedAmount, "INSUFFICIENT_BALANCE");
        // ...
    }

    function withdraw() hasActiveClaim(_msgSender()) external {
        // ...
        tokenAddress.safeTransfer(_msgSender(), amountRemaining);
        // ...
    }

Reference

https://etherscan.io/address/0x312ca0592a39a5fa5c87bb4f1da7b77544a91b87#code

Tools Used

Manual analysis.

Recommended Mitigation Steps

Disallow such kind of variable balance token.

uint256 balBefore = ERC20.balanceOf(address(this));
ERC20.transferFrom(...);
uint256 balAfter= ERC20.balanceOf(address(this));
uint256 actualBalChange = balAfter - balBefore;

Avoid floating pragmas: the version should be locked

The pragma declared across the contracts is ^0.8.0 and 0.8.14. Locking the pragma (for e.g. by not using ^ in pragma solidity 0.8.0) ensures that contracts do not accidentally get deployed using an older compiler version with unfixed bugs. (see here)

Description: 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.

Recommendation: Consider locking the pragma version.

Event is missing indexed fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

Instances number of this issue: 1

// contracts/VTVLVesting.sol
69:     event ClaimRevoked(address indexed _recipient, uint112 _numTokensWithheld, uint256 revocationTimestamp, Claim _claim);

Missing Time locks

When critical parameters of systems need to be changed, it is required to broadcast the change via event emission and recommended to enforce the changes after a time-delay. This is to allow system users to be aware of such critical changes and give them an opportunity to exit or adjust their engagement with the system accordingly. None of the onlyOwner functions that change critical protocol addresses/parameters have a timelock for a time-delayed change to alert: (1) users and give them a chance to engage/exit protocol if they are not agreeable to the changes (2) team in case of compromised owner(s) and give them a chance to perform incident response.

Instances number of this issue: 1

// contracts/AccessProtected.sol
    function setAdmin(address admin, bool isEnabled) public onlyAdmin {
        require(admin != address(0), "INVALID_ADDRESS");
        _admins[admin] = isEnabled;
        emit AdminAccessSet(admin, isEnabled);
    }

Recommendation: Users may be surprised when critical parameters are changed. Furthermore, it can erode users' trust since they can’t be sure the protocol rules won’t be changed later on. Compromised owner keys may be used to change protocol addresses/parameters to benefit attackers. Without a time-delay, authorized owners have no time for any planned incident response. (May be medium since owner can change critical parameters at anytime that can affect the users poorly).

Unused import

The OZ Ownable.sol is not used. Instances number of this issue: 2

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

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

Consider inherit the contract if needed.

Code Structure Deviates From Best-Practice

Description: The best-practice layout for a contract should follow the following order: state variables, events, modifiers, constructor and functions. Function ordering helps readers identify which functions they can call and find constructor and fallback functions easier. Functions should be grouped according to their visibility and ordered as: constructor, receive function (if exists), fallback function (if exists), external, public, internal, private. Some constructs deviate from this recommended best-practice: Modifiers are in the middle of contracts. External/public functions are mixed with internal/private ones. Few events are declared in contracts while most others are in corresponding interfaces.

Recommendation: Consider adopting recommended best-practice for code structure and layout.

contracts/VTVLVesting.sol

Missing or Incomplete NatSpec

Some functions are missing @notice/@dev NatSpec comments for the function, @param for all/some of their parameters and @return for return values. Given that NatSpec is an important part of code documentation, this affects code comprehension, auditability and usability.

Recommendation: Consider adding in full NatSpec comments for all functions to have complete code documentation for future use.

contracts/VTVLVesting.sol

Open TODO

Suggestion:

Implement the open TODOs or delete it.

Instances number of this issue: 1

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

Boolean comparison

Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value.

The demo of the gas comparison can be seen here.

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

Splitting require() statements that use &&

REQUIRE() statements with multiple conditions can be split.

See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper.

The demo of the gas comparison can be seen here.

// contracts/VTVLVesting.sol
270-278:
        require( 
            (
                _cliffReleaseTimestamp > 0 && 
                _cliffAmount > 0 && 
                _cliffReleaseTimestamp <= _startTimestamp
            ) || (
                _cliffReleaseTimestamp == 0 && 
                _cliffAmount == 0
        ), "INVALID_CLIFF");

344-351:
        require(_startTimestamps.length == length &&
                _endTimestamps.length == length &&
                _cliffReleaseTimestamps.length == length &&
                _releaseIntervalsSecs.length == length &&
                _linearVestAmounts.length == length &&
                _cliffAmounts.length == length, 
                "ARRAY_LENGTH_MISMATCH"
        );

Use custom errors rather than require() strings

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas.

Instances number of this issue: 24

// 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-278:
        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");

        require(_startTimestamp < _endTimestamp, "INVALID_END_TIMESTAMP"); // _endTimestamp must be after _startTimestamp
        require(_releaseIntervalSecs > 0, "INVALID_RELEASE_INTERVAL");
        require((_endTimestamp - _startTimestamp) % _releaseIntervalSecs == 0, "INVALID_INTERVAL_LENGTH");

        require( 
            (
                _cliffReleaseTimestamp > 0 && 
                _cliffAmount > 0 && 
                _cliffReleaseTimestamp <= _startTimestamp
            ) || (
                _cliffReleaseTimestamp == 0 && 
                _cliffAmount == 0
        ), "INVALID_CLIFF");

295:    require(tokenAddress.balanceOf(address(this)) >= numTokensReservedForVesting + allocatedAmount, "INSUFFICIENT_BALANCE");

344-351:
        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");
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
450:    require(bal > 0, "INSUFFICIENT_BALANCE");
    _otherTokenAddress.safeTransfer(_msgSender(), bal);

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

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

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

++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow

This saves 30-40 gas per loop

Instances number of this issue: 1

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

No need to explicitly initialize variables with default values

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

Instances number of this issue: 3

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

The demo of the loop gas comparison can be seen here.

Using calldata instead of memory for read-only arguments in external functions

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.

Instances number of this issue: 1

// contracts/VTVLVesting.sol
333-341:
    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 {

Functions only called outside of the contract can be declared as external

External call cost is less expensive than of public functions.

The difference is because in public functions, Solidity immediately copies array arguments to memory, while external functions can read directly from calldata. Memory allocation is expensive, whereas reading from calldata is cheap.

Instances number of this issue: 2

// contracts/AccessProtected.sol
39:     function setAdmin(address admin, bool isEnabled) public onlyAdmin {

// contracts/VTVLVesting.sol
398:    function withdrawAdmin(uint112 _amountRequested) public onlyAdmin {    

X = X + Y / X = X - Y IS CHEAPER THAN X += Y / X -= Y

The demo of the gas comparison can be seen here.

Consider use X = X + Y / X = X - Y to save gas.

Instances number of this issue: 7

// contracts/VTVLVesting.sol
161:        vestAmt += _claim.cliffAmount;
179:        vestAmt += linearVestAmount;
301:        numTokensReservedForVesting += allocatedAmount; // track the allocated amount
381:        usrClaim.amountWithdrawn += amountRemaining;
383:        numTokensReservedForVesting -= amountRemaining;
433:        numTokensReservedForVesting -= amountRemaining; // Reduces the allocation

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

Functions guaranteed to revert when called by normal users can be marked payable

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 legitimate callers because the compiler will not include checks for whether a payment was provided.

The extra opcodes avoided are

CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2)

which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.

Instances number of this issue: 9

// contracts/AccessProtected.sol
39:     function setAdmin(address admin, bool isEnabled) public onlyAdmin {

// contracts/VTVLVesting.sol
245-253:
    function _createClaimUnchecked() private  hasNoClaim(_recipient) {
317-325:
        function createClaim() external onlyAdmin {
333-341:
        function createClaimsBatch() external onlyAdmin {
364:    function withdraw() hasActiveClaim(_msgSender()) external {
398:    function withdrawAdmin(uint112 _amountRequested) public onlyAdmin {    
418:    function revokeClaim(address _recipient) external onlyAdmin hasActiveClaim(_recipient) {
446:    function withdrawOtherToken(IERC20 _otherTokenAddress) external onlyAdmin {

// contracts/token/VariableSupplyERC20Token.sol
36:     function mint(address account, uint256 amount) public onlyAdmin {

Usage of uint/int smaller than 32 bytes (256 bits) incurs overhead

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.

Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed

Reference: Layout of State Variables in Storage

Instances number of this issue: 2

// contracts/VTVLVesting.sol
27:     uint112 public numTokensReservedForVesting = 0;
148:    uint112 vestAmt = 0;

Using bool for storage incurs overhead

// 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.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past

Instances number of this issue: 1

// contracts/VTVLVesting.sol
45:       bool isActive; // whether this claim is active (or revoked)

Duplicate calculation

Instances number of this issue: 2 numTokensReservedForVesting + allocatedAmount can be saved to a local memory variable.

// contracts/VTVLVesting.sol
295-301:
        require(tokenAddress.balanceOf(address(this)) >= numTokensReservedForVesting + allocatedAmount, "INSUFFICIENT_BALANCE");

        // ...

        numTokensReservedForVesting += allocatedAmount; // track the allocated amount

_cliffAmount + _linearVestAmount can be saved to a local memory variable allocatedAmount first.

// contracts/VTVLVesting.sol
256:    require(_linearVestAmount + _cliffAmount > 0, "INVALID_VESTED_AMOUNT"); // Actually only one of linearvested/cliff amount must be 0, not necessarily both

292:    uint112 allocatedAmount = _cliffAmount + _linearVestAmount;

No need for extra variable

// contracts/VTVLVesting.sol
    modifier hasNoClaim(address _recipient) {
        Claim storage _claim = claims[_recipient];
        // ...
        require(_claim.startTimestamp == 0, "CLAIM_ALREADY_EXISTS");
        _;
    }

Can be simplified as:

// contracts/VTVLVesting.sol
    modifier hasNoClaim(address _recipient) {
        require(claims[_recipient].startTimestamp == 0, "CLAIM_ALREADY_EXISTS");
        _;
    }

Unnecessary check

The check for _releaseIntervalSecs > 0 is not necessary, since if the value is 0, the next line will revert.

// contracts/VTVLVesting.sol
263:    require(_releaseIntervalSecs > 0, "INVALID_RELEASE_INTERVAL");
264:    require((_endTimestamp - _startTimestamp) % _releaseIntervalSecs == 0, "INVALID_INTERVAL_LENGTH");

Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement

// contracts/VTVLVesting.sol
374-377:
        require(allowance > usrClaim.amountWithdrawn, "NOTHING_TO_WITHDRAW");

        uint112 amountRemaining = allowance - usrClaim.amountWithdrawn;

allowance - usrClaim.amountWithdrawn can be unchecked.

Caching storage variables in memory

Caching will replace each Gwarmaccess (100 gas) with a much cheaper stack read.

usrClaim.amountWithdrawn can be saved in local memory.

// contracts/VTVLVesting.sol
374-377:
        require(allowance > usrClaim.amountWithdrawn, "NOTHING_TO_WITHDRAW");

        uint112 amountRemaining = allowance - usrClaim.amountWithdrawn;