VTVL contest - 0x1f8b's results

• Low
  • 1. Outdated packages
  • 2. Mixing and Outdated compiler
  • 3. Admin can mint any amount of tokens to an arbitrary address
  • 4. Lack of ACK during owner change
  • 5. Poor governance design
• Non critical
  • 6. Integer overflow by unsafe casting
  • 7. Use tokens with ERC20 permit
  • 8. Inheritance issues
  • 9. Open TODO

Low

1. Outdated packages

Some used packages are out of date, it is good practice to use the latest version of these packages:

    "@openzeppelin/contracts": "^4.5.0"

Affected source code:

• package.json:7

2. Mixing and Outdated compiler

The pragma version used are:

pragma solidity 0.8.14;
pragma solidity ^0.8.14;

Note that mixing pragma is not recommended. Because different compiler versions have different meanings and behaviors, it also significantly raises maintenance costs. As a result, depending on the compiler version selected for any given file, deployed contracts may have security issues.

The minimum required version must be 0.8.16; otherwise, contracts will be affected by the following important bug fixes:

0.8.15

• Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays.
• Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

0.8.16

• Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

Apart from these, there are several minor bug fixes and improvements.

3. Admin can mint any amount of tokens to an arbitrary address

The owner can mint an arbitrary amount of tokens to an arbitrary address.

This is needless and poses a severe risk of centralization. A malicious or compromised owner can take advantage of this.

Affected source code:

• VariableSupplyERC20Token.sol:36

4. Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because of human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

• AccessProtected.sol:39

5. Poor governance design

Instead of using a TimeLock or a multisignature wallet, an unlimited number of owners are allowed which can lead to governance issues.

Affected source code:

• AccessProtected.sol

---

Non critical

6. Integer overflow by unsafe casting

Keep in mind that the version of solidity used, despite being greater than 0.8, does not prevent integer overflows during casting, it only does so in mathematical operations.

It is necessary to safely convert between the different numeric types.

Recommendation:

Use a safeCast from Open Zeppelin.

uint40(block.timestamp)

Affected source code:

• VTVLVesting.sol:217
• VTVLVesting.sol:371
• VTVLVesting.sol:436

7. Use tokens with ERC20 permit

The erc20 signature approval extension is a major usability enhancement that helps users and projects reap its benefits in the future.

The use of this feature is fully recommended.

Reference:

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/draft-ERC20Permit.sol

Affected source code:

• FullPremintERC20Token.sol:8
• VariableSupplyERC20Token.sol:10

8. Inheritance issues

You can see that the VTVLVesting contract inherits from Context and AccessProtected, however, AccessProtected already inherits from Context so the double definition is unnecessary and redundant.

Affected source code:

• VTVLVesting.sol:11
• AccessProtected.sol:11

9. Open TODO

The code that contains "open todos" reflects that the development is not finished and that the code can change a posteriori, prior release, with or without audit.

Affected source code:

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

• VTVLVesting.sol:266

• Gas
  • 1. Reduce boolean comparison
    • Total gas saved: 18 * 1 = 18
  • 2. Avoid compound assignment operator in state variables
    • Total gas saved: 13 * 5 = 65
  • 3. ++i costs less gas compared to i++ or i += 1
  • 4. There's no need to set default values for variables
    • Total gas saved: 8 * 3 = 24
  • 5. Use Custom Errors instead of Revert Strings to save Gas
    • Total gas saved: 9 * 24 = 216
  • 6. Change bool to uint256 can save gas
  • 7. Reduce Inheritance
  • 8. Remove unnecessary variables
  • 9. Use the unchecked keyword
  • 10. Optimize hasActiveClaim

Gas

1. Reduce boolean comparison

Compare a boolean value using == true or == false instead of using the boolean value is more expensive.

NOT opcode it's cheaper than using EQUAL or NOTEQUAL when the value it's false, or just the value without == true when it's true, because it will use less opcodes inside the VM.

Proof of concept (without optimizations):

pragma solidity 0.8.16;

contract TesterA {
function testEqual(bool a) public view returns (bool) { return a == true; }
}

contract TesterB {
function testNot(bool a) public view returns (bool) { return a; }
}

Gas saving executing: 18 per entry for == true

TesterA.testEqual:   21814
TesterB.testNot:     21796   

pragma solidity 0.8.16;

contract TesterA {
function testEqual(bool a) public view returns (bool) { return a == false; }
}

contract TesterB {
function testNot(bool a) public view returns (bool) { return !a; }
}

Gas saving executing: 15 per entry for == false

TesterA.testEqual:   21814
TesterB.testNot:     21799

Affected source code:

Use the value instead of == true:

• VTVLVesting.sol:111

Total gas saved: 18 * 1 = 18

2. Avoid compound assignment operator in state variables

Using compound assignment operators for state variables (like State += X or State -= X ...) it's more expensive than using operator assignment (like State = State + X or State = State - X ...).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
uint private _a;
function testShort() public {
_a += 1;
}
}

contract TesterB {
uint private _a;
function testLong() public {
_a = _a + 1;
}
}

Gas saving executing: 13 per entry

TesterA.testShort: 43507
TesterB.testLong:  43494

Affected source code:

• VTVLVesting.sol:301
• VTVLVesting.sol:381
• VTVLVesting.sol:383
• VTVLVesting.sol:433
• VariableSupplyERC20Token.sol:43

Total gas saved: 13 * 5 = 65

3. ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integers, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Keep in mind that this change can only be made when we are not interested in the value returned by the operation, since the result is different, you only have to apply it when you only want to increase a counter.

Affected source code:

• VTVLVesting.sol:353

4. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testInit() public view returns (uint) { uint a = 0; return a; }
}

contract TesterB {
function testNoInit() public view returns (uint) { uint a; return a; }
}

Gas saving executing: 8 per entry

TesterA.testInit:   21392
TesterB.testNoInit: 21384   

Affected source code:

• VTVLVesting.sol:27
• VTVLVesting.sol:148
• VTVLVesting.sol:353

Total gas saved: 8 * 3 = 24

5. Use Custom Errors instead of Revert Strings to save 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)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testRevert(bool path) public view {
 require(path, "test error");
}
}

contract TesterB {
error MyError(string msg);
function testError(bool path) public view {
 if(path) revert MyError("test error");
}
}

Gas saving executing: 9 per entry

TesterA.testRevert: 21611
TesterB.testError:  21602     

Affected source code:

• AccessProtected.sol:25
• AccessProtected.sol:40
• VTVLVesting.sol:82
• VTVLVesting.sol:107
• VTVLVesting.sol:111
• VTVLVesting.sol:129
• VTVLVesting.sol:255
• VTVLVesting.sol:256
• VTVLVesting.sol:257
• VTVLVesting.sol:262
• VTVLVesting.sol:263
• VTVLVesting.sol:264
• VTVLVesting.sol:270-278
• VTVLVesting.sol:295
• VTVLVesting.sol:344-351
• VTVLVesting.sol:374
• VTVLVesting.sol:402
• VTVLVesting.sol:426
• VTVLVesting.sol:447
• VTVLVesting.sol:449
• FullPremintERC20Token.sol:11
• VariableSupplyERC20Token.sol:27
• VariableSupplyERC20Token.sol:37
• VariableSupplyERC20Token.sol:41

Total gas saved: 9 * 24 = 216

6. Change bool to uint256 can save gas

Because each write operation requires an additional SLOAD to read the slot's contents, replace the bits occupied by the boolean, and then write back, booleans are more expensive than uint256 or any other type that uses a complete word. This cannot be turned off because it is the compiler's defense against pointer aliasing and contract upgrades.

Reference:

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol#L23-L27

Also, this is applicable to integer types different from uint256 or int256.

Affected source code for booleans:

• AccessProtected.sol:12

7. Reduce Inheritance

You can see that the VTVLVesting contract inherits from Context and AccessProtected, however, AccessProtected already inherits from Context so the double definition is unnecessary and redundant. This could result in a larger script size.

Affected source code:

• VTVLVesting.sol:11
• AccessProtected.sol:11

8. Remove unnecessary variables

The following state variables can be removed without affecting the logic of the contract since they are not used and/or are redundant.

Affected source code:

linearVestAmount can be inline:

• VTVLVesting.sol:176

9. Use the unchecked keyword

When an underflow or overflow cannot occur, one might conserve gas by using the unchecked keyword to prevent unnecessary arithmetic underflow/overflow tests.

    function withdraw() hasActiveClaim(_msgSender()) external {
        ...

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

        // Calculate how much can we withdraw (equivalent to the above inequality)
+       uint112 amountRemaining;
+       unchecked {
+           amountRemaining = allowance - usrClaim.amountWithdrawn;
+       }
-       uint112 amountRemaining = allowance - usrClaim.amountWithdrawn;

        ...
    }

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

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

        // The amount that is "reclaimed" is equal to the total allocation less what was already withdrawn
+       uint112 amountRemaining;
+       unchecked {
-           amountRemaining = finalVestAmt - _claim.amountWithdrawn;
+       }
-       uint112 amountRemaining = finalVestAmt - _claim.amountWithdrawn;

        ...
    }

Affected source code:

• VTVLVesting.sol:377
• VTVLVesting.sol:429

10. Optimize hasActiveClaim

There is no need to check the startTimestamp in hasActiveClaim because if isActive is true, startTimestamp always will be different than 0.

    modifier hasActiveClaim(address _recipient) {
        Claim storage _claim = claims[_recipient];
-       require(_claim.startTimestamp > 0, "NO_ACTIVE_CLAIM");

        // We however still need the active check, since (due to the name of the function)
        // we want to only allow active claims
        require(_claim.isActive == true, "NO_ACTIVE_CLAIM");

        // Save gas, omit further checks
        // require(_claim.linearVestAmount + _claim.cliffAmount > 0, "INVALID_VESTED_AMOUNT");
        // require(_claim.endTimestamp > 0, "NO_END_TIMESTAMP");
        _;
    }

Affected source code:

• VTVLVesting.sol:107