Platform: Code4rena
Start Date: 20/09/2022
Pot Size: $30,000 USDC
Total HM: 12
Participants: 198
Period: 3 days
Judge: 0xean
Total Solo HM: 2
Id: 164
League: ETH
Rank: 61/198
Findings: 2
Award: $47.42
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: AkshaySrivastav
Also found by: 0v3rf10w, 0x040, 0x1f8b, 0x4non, 0x5rings, 0x85102, 0xA5DF, 0xDecorativePineapple, 0xNazgul, 0xSky, 0xSmartContract, 0xbepresent, 0xf15ers, 0xmatt, 2997ms, Aeros, Aymen0909, B2, Bahurum, Bnke0x0, CertoraInc, Chom, ChristianKuri, CodingNameKiki, Deivitto, Diana, Diraco, Dravee, ElKu, Funen, IllIllI, JC, JLevick, JohnSmith, JohnnyTime, KIntern_NA, Lambda, Margaret, MasterCookie, OptimismSec, RaymondFam, Respx, ReyAdmirado, RockingMiles, Rohan16, Rolezn, Ruhum, RustyRabbit, Sm4rty, SooYa, StevenL, TomJ, Tomo, V_B, Waze, Yiko, __141345__, a12jmx, ajtra, ak1, async, ayeslick, aysha, berndartmueller, bin2chen, bobirichman, brgltd, bulej93, c3phas, carrotsmuggler, cccz, ch13fd357r0y3r, chatch, cryptostellar5, cryptphi, csanuragjain, d3e4, datapunk, delfin454000, dic0de, djxploit, durianSausage, eighty, erictee, exd0tpy, fatherOfBlocks, gogo, got_targ, hansfriese, ignacio, ikbkln, indijanc, innertia, joestakey, karanctf, ladboy233, leosathya, lukris02, martin, medikko, millersplanet, nalus, natzuu, neko_nyaa, neumo, obront, oyc_109, pcarranzav, peanuts, pedr02b2, pedroais, peiw, peritoflores, prasantgupta52, rajatbeladiya, rbserver, reassor, ret2basic, rokinot, romand, rotcivegaf, rvierdiiev, sach1r0, seyni, sikorico, slowmoses, sorrynotsorry, supernova, tibthecat, tnevler, ubermensch, yongskiws, zzykxx, zzzitron
20.4189 USDC - $20.42
As some known vulnerabilities exist in the current @openzeppelin/contracts version, consider updating package.json with at least @openzeppelin/contracts@4.7.3 here:
File: /package.json 7: "@openzeppelin/contracts": "^4.5.0",
Contracts are allowed to override their parents' functions and change the visibility from external to public.
File: /contracts/AccessProtected.sol 39: function setAdmin(address admin, bool isEnabled) public onlyAdmin {
File: /contracts/VTVLVesting.sol 398: function withdrawAdmin(uint112 _amountRequested) public onlyAdmin {
Contracts should be deployed with the same compiler version and flags that they have been tested the most with. Locking the pragma helps ensure that contracts do not accidentally get deployed using, for example, the latest compiler which may have higher risks of undiscovered bugs. Contracts may also be deployed by others and the pragma indicates the compiler version intended by the original authors.
File: /contracts/token/VariableSupplyERC20Token.sol 2: pragma solidity ^0.8.14;
File: /contracts/VTVLVesting.sol //@audit: an user --> a user 96: @notice This modifier requires that an user has a claim attached.
File: /contracts/token/VariableSupplyERC20Token.sol //@audit: A ERC20 --> An ERC20 8: @notice A ERC20 token contract that allows minting at will, with limited or unlimited supply. //@audit: A ERC20 --> An ERC20 14: @notice A ERC20 token contract that allows minting at will, with limited or unlimited supply. No burning possible
File: /contracts/token/FullPremintERC20Token.sol 9: // uint constant _initialSupply = 100 * (10**18);
File: /contracts/VTVLVesting.sol 114: // require(_claim.linearVestAmount + _claim.cliffAmount > 0, "INVALID_VESTED_AMOUNT"); 115: // require(_claim.endTimestamp > 0, "NO_END_TIMESTAMP");
File: /contracts/VTVLVesting.sol 133: // require(_claim.isActive == false, "CLAIM_ALREADY_EXISTS"); 136: // require(_claim.endTimestamp == 0, "CLAIM_ALREADY_EXISTS"); 137: // require(_claim.linearVestAmount + _claim.cliffAmount == 0, "CLAIM_ALREADY_EXISTS"); 138: // require(_claim.amountWithdrawn == 0, "CLAIM_ALREADY_EXISTS");
File: /contracts/VTVLVesting.sol 266: // Potential TODO: sanity check, if _linearVestAmount == 0, should we perhaps force that start and end ts are the same?
File: /contracts/VTVLVesting.sol //@audit: Missing @param _recipient 415: @notice Allow an Owner to revoke a claim that is already active. 416: @dev The requirement is that a claim exists and that it's active. 417: */ 418: function revokeClaim(address _recipient) external onlyAdmin hasActiveClaim(_recipient) {
File: /contracts/VTVLVesting.sol //@audit: Missing @param _recipients, @param _startTimestamps,@param _endTimestamps, @param _cliffReleaseTimestamps, @param _releaseIntervalsSecs,@param _linearVestAmounts, //@param _cliffAmounts 333: function createClaimsBatch( 334: address[] memory _recipients, 335: uint40[] memory _startTimestamps, 336: uint40[] memory _endTimestamps, 337: uint40[] memory _cliffReleaseTimestamps, 338: uint40[] memory _releaseIntervalsSecs, 339: uint112[] memory _linearVestAmounts, 340: uint112[] memory _cliffAmounts) 341: external onlyAdmin {
File: /contracts/token/VariableSupplyERC20Token.sol //@audit: Missing @param account, @param amount 36: function mint(address account, uint256 amount) public onlyAdmin {
File: /contracts/AccessProtected.sol //@audit: Missing @param _addressToCheck 29: function isAdmin(address _addressToCheck) external view returns (bool) {
File: /contracts/VTVLVesting.sol //@audit: Should the string be "INVALID_START_TIMESTAMP" instead or probably change it not be similar with the one on the next statement 107: require(_claim.startTimestamp > 0, "NO_ACTIVE_CLAIM");
The revert string used in above matches another one in the same modifier which does the actual check reffered to by the string. see below
File: /contracts/VTVLVesting.sol 111: require(_claim.isActive == true, "NO_ACTIVE_CLAIM");
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.
The following contract has modifiers coming in between functions, external function are mixed with internal/private ones https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol
Similar to how we emit an event when withdrawing unallocated tokens, we should also emit an event for the following
File: /contracts/VTVLVesting.sol 446: function withdrawOtherToken(IERC20 _otherTokenAddress) external onlyAdmin { 447: require(_otherTokenAddress != tokenAddress, "INVALID_TOKEN"); // tokenAddress address is already sure to be nonzero due to constructor 448: uint256 bal = _otherTokenAddress.balanceOf(address(this)); 449: require(bal > 0, "INSUFFICIENT_BALANCE"); 450: _otherTokenAddress.safeTransfer(_msgSender(), bal); }
🌟 Selected for report: IllIllI
Also found by: 0v3rf10w, 0x040, 0x1f8b, 0x4non, 0x85102, 0xA5DF, 0xDanielC, 0xNazgul, 0xSmartContract, 0xbepresent, 0xc0ffEE, 0xsam, 2997ms, AkshaySrivastav, Amithuddar, Atarpara, Aymen0909, B2, Bnke0x0, CertoraInc, Chom, ChristianKuri, CodingNameKiki, Deivitto, Diana, DimitarDimitrov, Diraco, Funen, JC, JLevick, JohnSmith, Junnon, KIntern_NA, Lambda, MasterCookie, Matin, Noah3o6, Ocean_Sky, OptimismSec, RaymondFam, Respx, ReyAdmirado, RockingMiles, Rohan16, Rolezn, Ruhum, Saintcode_, Satyam_Sharma, Sm4rty, SnowMan, SooYa, Sta1400, StevenL, Tadashi, Tagir2003, TomJ, Tomio, Tomo, V_B, Waze, WilliamAmbrozic, Yiko, __141345__, a12jmx, adriro, ajtra, ak1, async, aysha, beardofginger, bobirichman, brgltd, bulej93, c3phas, carrotsmuggler, caventa, ch0bu, cryptostellar5, cryptphi, csanuragjain, d3e4, delfin454000, dharma09, djxploit, durianSausage, eighty, emrekocak, erictee, exd0tpy, fatherOfBlocks, francoHacker, gianganhnguyen, gogo, got_targ, hxzy, ignacio, ikbkln, imare, indijanc, jag, jpserrat, karanctf, ladboy233, leosathya, lucacez, lukris02, m9800, malinariy, martin, medikko, mics, millersplanet, mrpathfindr, nalus, natzuu, neko_nyaa, oyc_109, pauliax, peanuts, pedroais, peiw, pfapostol, prasantgupta52, rbserver, ret2basic, rokinot, rotcivegaf, rvierdiiev, sach1r0, samruna, seyni, slowmoses, subtle77, supernova, tgolding55, tibthecat, tnevler, w0Lfrum, yaemsobak, zishansami
26.9971 USDC - $27.00
NB: Some functions have been truncated where neccessary to just show affected parts of the code The gas estimates are the exact results from running the tests included with an exception of internal/public functions where estimates are based on the known opcodes. The optimizer is set to run with the default runs(200). Throught the report some places might be denoted with audit tags to show the actual place affected.
The code can be optimized by minimizing the number of SLOADs.
SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.
File: /contracts/token/VariableSupplyERC20Token.sol 36: function mint(address account, uint256 amount) public onlyAdmin { 37: require(account != address(0), "INVALID_ADDRESS"); 38: // If we're using maxSupply, we need to make sure we respect it 39: // mintableSupply = 0 means mint at will 40: if(mintableSupply > 0) { //@audit: mintableSupply SLOAD 1 41: require(amount <= mintableSupply, "INVALID_AMOUNT"); //@audit: mintableSupply SLOAD 2 42: // We need to reduce the amount only if we're using the limit, if not just leave it be 43: mintableSupply -= amount; //@audit: mintableSupply SLOAD 3 + 1 SSTORE 44: } 45: _mint(account, amount); 46: }
diff --git a/contracts/token/VariableSupplyERC20Token.sol b/contracts/token/VariableSupplyERC20Token.sol index 1297a14..7a560c3 100644 --- a/contracts/token/VariableSupplyERC20Token.sol +++ b/contracts/token/VariableSupplyERC20Token.sol @@ -37,10 +37,11 @@ contract VariableSupplyERC20Token is ERC20, AccessProtected { 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"); + uint256 temp = mintableSupply; + if(temp > 0) { + require(amount <= temp, "INVALID_AMOUNT"); // We need to reduce the amount only if we're using the limit, if not just leave it be - mintableSupply -= amount; + mintableSupply = temp - amount; } _mint(account, amount); }
Function: createClaim Average gas before: 163686 Average gas after : 163506 Function:createClaimsBatch Average gas before: 274860 Average gas after : 274500
File: /contracts/VTVLVesting.sol 245: function _createClaimUnchecked( 295: require(tokenAddress.balanceOf(address(this)) >= numTokensReservedForVesting + allocatedAmount, "INSUFFICIENT_BALANCE"); //@audit: SLOAD 1 301: numTokensReservedForVesting += allocatedAmount; // track the allocated amount //@audit: SLOAD 2 }
diff --git a/contracts/VTVLVesting.sol b/contracts/VTVLVesting.sol index c564c6f..3f30dee 100644 --- a/contracts/VTVLVesting.sol +++ b/contracts/VTVLVesting.sol @@ -290,15 +290,16 @@ contract VTVLVesting is Context, AccessProtected { // Our total allocation is simply the full sum of the two amounts, _cliffAmount + _linearVestAmount // Not necessary to use the more complex logic from _baseVestedAmount uint112 allocatedAmount = _cliffAmount + _linearVestAmount; + uint112 temp = numTokensReservedForVesting; // Still no effects up to this point (and tokenAddress is selected by contract deployer and is immutable), so no reentrancy risk - require(tokenAddress.balanceOf(address(this)) >= numTokensReservedForVesting + allocatedAmount, "INSUFFICIENT_BALANCE"); + require(tokenAddress.balanceOf(address(this)) >= temp + allocatedAmount, "INSUFFICIENT_BALANCE"); // Done with checks // Effects limited to lines below claims[_recipient] = _claim; // store the claim - numTokensReservedForVesting += allocatedAmount; // track the allocated amount + numTokensReservedForVesting = temp + allocatedAmount; // track the allocated amount vestingRecipients.push(_recipient); // add the vesting recipient to the list emit ClaimCreated(_recipient, _claim); // let everyone know }
Average Gas before: 69082 Average Gas After: 68789
File: /contracts/VTVLVesting.sol 364: function withdraw() hasActiveClaim(_msgSender()) external { 367: Claim storage usrClaim = claims[_msgSender()]; 374: require(allowance > usrClaim.amountWithdrawn, "NOTHING_TO_WITHDRAW"); 377: uint112 amountRemaining = allowance - usrClaim.amountWithdrawn; 381: usrClaim.amountWithdrawn += amountRemaining; }
diff --git a/contracts/VTVLVesting.sol b/contracts/VTVLVesting.sol index c564c6f..5b51596 100644 --- a/contracts/VTVLVesting.sol +++ b/contracts/VTVLVesting.sol @@ -365,20 +365,21 @@ contract VTVLVesting is Context, AccessProtected { // Get the message sender claim - if any Claim storage usrClaim = claims[_msgSender()]; + uint112 temp = usrClaim.amountWithdrawn; // 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)); // Make sure we didn't already withdraw more that we're allowed. - require(allowance > usrClaim.amountWithdrawn, "NOTHING_TO_WITHDRAW"); + require(allowance > temp, "NOTHING_TO_WITHDRAW"); // Calculate how much can we withdraw (equivalent to the above inequality) - uint112 amountRemaining = allowance - usrClaim.amountWithdrawn; + uint112 amountRemaining = allowance - temp; // "Double-entry bookkeeping" // Carry out the withdrawal by noting the withdrawn amount, and by transferring the tokens. - usrClaim.amountWithdrawn += amountRemaining; + usrClaim.amountWithdrawn = temp + amountRemaining; // Reduce the allocated amount since the following transaction pays out so the "debt" gets reduced numTokensReservedForVesting -= amountRemaining;
Average Gas before: 36542 Average Gas After: 36451
File: /contracts/VTVLVesting.sol 418: function revokeClaim(address _recipient) external onlyAdmin hasActiveClaim(_recipient) { 420: Claim storage _claim = claims[_recipient]; 426: require( _claim.amountWithdrawn < finalVestAmt, "NO_UNVESTED_AMOUNT"); 429: uint112 amountRemaining = finalVestAmt - _claim.amountWithdrawn; 437: }
diff --git a/contracts/VTVLVesting.sol b/contracts/VTVLVesting.sol index c564c6f..675a9ab 100644 --- a/contracts/VTVLVesting.sol +++ b/contracts/VTVLVesting.sol @@ -423,10 +423,11 @@ contract VTVLVesting is Context, AccessProtected { - require( _claim.amountWithdrawn < finalVestAmt, "NO_UNVESTED_AMOUNT"); + uint112 temp = _claim.amountWithdrawn; + require( temp < finalVestAmt, "NO_UNVESTED_AMOUNT"); // The amount that is "reclaimed" is equal to the total allocation less what was already withdrawn - uint112 amountRemaining = finalVestAmt - _claim.amountWithdrawn; + uint112 amountRemaining = finalVestAmt - temp;
File: /contracts/VTVLVesting.sol 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");
If _startTimestamp is equal to 0, the whole transaction would revert but this would be after evaluating _linearVestAmount + _cliffAmount > 0 which costs more gas.
If we refactor it, and _startTimestamp happens to be 0, then we would fail without consuming the gas for the expression _linearVestAmount + _cliffAmount > 0
diff --git a/contracts/VTVLVesting.sol b/contracts/VTVLVesting.sol index c564c6f..144c228 100644 --- a/contracts/VTVLVesting.sol +++ b/contracts/VTVLVesting.sol @@ -253,8 +253,8 @@ contract VTVLVesting is Context, AccessProtected { ) private hasNoClaim(_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"); + require(_linearVestAmount + _cliffAmount > 0, "INVALID_VESTED_AMOUNT"); // Actually only one of linearvested/cliff amount must be 0, not necessarily both
diff --git a/contracts/VTVLVesting.sol b/contracts/VTVLVesting.sol index c564c6f..891d902 100644 --- a/contracts/VTVLVesting.sol +++ b/contracts/VTVLVesting.sol @@ -253,14 +253,15 @@ contract VTVLVesting is Context, AccessProtected { ) private hasNoClaim(_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"); + require(_releaseIntervalSecs > 0, "INVALID_RELEASE_INTERVAL"); require(_startTimestamp < _endTimestamp, "INVALID_END_TIMESTAMP"); // _endTimestamp must be after _startTimestamp - require(_releaseIntervalSecs > 0, "INVALID_RELEASE_INTERVAL"); + require(_linearVestAmount + _cliffAmount > 0, "INVALID_VESTED_AMOUNT"); // Actually only one of linearvested/cliff amount must be 0, not necessarily both require((_endTimestamp - _startTimestamp) % _releaseIntervalSecs == 0, "INVALID_INTERVAL_LENGTH");
contract testRequire{ function testReq(address sender, uint40 val,uint40 val2) external pure returns (uint40){ require (sender != address(0),"INVALID_ADDRESS"); require(val + val2 > 5,"INVALID_VESTED_AMOUNT"); require (val > 0, "INVALID_START_TIMESTAMP" ); return (val); } }
When val = 0, the execution cost for the above is : 22411 gas
contract testRequire{ function testReq(address sender, uint40 val,uint40 val2) external pure returns (uint40){ require (sender != address(0),"INVALID_ADDRESS"); require (val > 0, "INVALID_START_TIMESTAMP" ); require(val + val2 > 5,"INVALID_VESTED_AMOUNT"); return (val); } }
When val = 0, the execution cost for the above is : 22293 gas
If we split the require statement that uses && we can further reorder them to save more gas.
Refactor your code to have the require statement that consumes less gas at the top. It saves gas if we fail early as no need to execute complex arithmetics
We can pack the struct together by redeclaring the releaseIntervalSecs as a uint32 instead of uint40
File: /contracts/VTVLVesting.sol struct Claim { 35: uint40 startTimestamp; // When does the vesting start (40 bits is enough for TS) 36: uint40 endTimestamp; // When does the vesting end - the vesting goes linearly between the start and end timestamps 37: uint40 cliffReleaseTimestamp; // At which timestamp is the cliffAmount released. This must be <= startTimestamp 38: uint40 releaseIntervalSecs; // Every how many seconds does the vested amount increase. 42: uint112 linearVestAmount; // total entitlement 43: uint112 cliffAmount; // how much is released at the cliff 44: uint112 amountWithdrawn; // how much was withdrawn thus far - released at the cliffReleaseTimestamp 45: bool isActive; // whether this claim is active (or revoked) 46: }
We can modify as follows(Note, the releaseIntervalSecs uses uint32 after modification)
diff --git a/contracts/VTVLVesting.sol b/contracts/VTVLVesting.sol index c564c6f..d2b7015 100644 --- a/contracts/VTVLVesting.sol +++ b/contracts/VTVLVesting.sol @@ -32,17 +32,17 @@ contract VTVLVesting is Context, AccessProtected { struct Claim { + bool isActive; // whether this claim is active (or revoked) uint40 startTimestamp; // When does the vesting start (40 bits is enough for TS) uint40 endTimestamp; // When does the vesting end - the vesting goes linearly between the start and end timestamps uint40 cliffReleaseTimestamp; // At which timestamp is the cliffAmount released. This must be <= startTimestamp - uint40 releaseIntervalSecs; // Every how many seconds does the vested amount increase. - + uint112 linearVestAmount; // total entitlement + // uint112 range: range 0 – 5,192,296,858,534,827,628,530,496,329,220,095. // uint112 range: range 0 – 5,192,296,858,534,827. - uint112 linearVestAmount; // total entitlement + uint32 releaseIntervalSecs; // Every how many seconds does the vested amount increase. uint112 cliffAmount; // how much is released at the cliff uint112 amountWithdrawn; // how much was withdrawn thus far - released at the cliffReleaseTimestamp - bool isActive; // whether this claim is active (or revoked) }
If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.
File: /contracts/VTVLVesting.sol 333: function createClaimsBatch( 334: address[] memory _recipients, 335: uint40[] memory _startTimestamps, 336: uint40[] memory _endTimestamps, 337: uint40[] memory _cliffReleaseTimestamps, 338: uint40[] memory _releaseIntervalsSecs, 339: uint112[] memory _linearVestAmounts, 340: uint112[] memory _cliffAmounts) 341: external onlyAdmin {
File: /contracts/token/VariableSupplyERC20Token.sol 43: mintableSupply -= amount;
Modify above to:
unchecked{ mintableSupply = mintableSupply - amount; }
It's safe to use unchecked blocks here as our operation cannot underflow
Other instances
File: /contracts/VTVLVesting.sol 301: numTokensReservedForVesting += allocatedAmount; // track the allocated amount
File: /contracts/VTVLVesting.sol 381: usrClaim.amountWithdrawn += amountRemaining;
File: contracts/VTVLVesting.sol 383: numTokensReservedForVesting -= amountRemaining;
File: /contracts/VTVLVesting.sol 433: numTokensReservedForVesting -= amountRemaining; // Reduces the allocation
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.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed
File: /contracts/VTVLVesting.sol //@audit: uint40 _referenceTs 147: function _baseVestedAmount(Claim memory _claim, uint40 _referenceTs) internal pure returns (uint112) { //@audit:uint112 vestAmt 148: uint112 vestAmt = 0; //@audit: uint40 _referenceTs 196: function vestedAmount(address _recipient, uint40 _referenceTs) public view returns (uint112) { //@audit: uint40 _startTimestamp,uint40 _endTimestamp,uint40 _cliffReleaseTimestamp, uint40 _releaseIntervalSecs,uint112 _linearVestAmount, uint112 _cliffAmount 245: function _createClaimUnchecked( 246: address _recipient, 247: uint40 _startTimestamp, 248: uint40 _endTimestamp, 249: uint40 _cliffReleaseTimestamp, 250: uint40 _releaseIntervalSecs, 251: uint112 _linearVestAmount, 252: uint112 _cliffAmount 253: ) private hasNoClaim(_recipient) { //@audit: uint40 _startTimestamp,uint40 _endTimestamp,uint40 _cliffReleaseTimestamp, uint40 _releaseIntervalSecs,uint112 _linearVestAmount, uint112 _cliffAmount 317: function createClaim( 318: address _recipient, 319: uint40 _startTimestamp, 320: uint40 _endTimestamp, 321: uint40 _cliffReleaseTimestamp, 322: uint40 _releaseIntervalSecs, 323: uint112 _linearVestAmount, 324: uint112 _cliffAmount 325: ) external onlyAdmin { @audit:uint112 _amountRequested 398: function withdrawAdmin(uint112 _amountRequested) public onlyAdmin {
If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). If you explicitly initialize it with its default value, you are just wasting gas. It costs more gas to initialize variables to zero than to let the default of zero be applied Declaring uint256 i = 0; means doing an MSTORE of the value 0 Instead you could just declare uint256 i to declare the variable without assigning it’s default value, saving 3 gas per declaration
File: /contracts/VTVLVesting.sol 27: uint112 public numTokensReservedForVesting = 0;
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 see resource https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/token/VariableSupplyERC20Token.sol#L43
File: /contracts/token/VariableSupplyERC20Token.sol 43: mintableSupply -= amount;
The operation mintableSupply -= amount; cannot underflow due to the check on Line 41 that ensures that the value of mintableSupply is greater than amount before perfoming the operation
Modify above to:
unchecked{ mintableSupply = mintableSupply - amount; }
File: /contracts/VTVLVesting.sol 377: uint112 amountRemaining = allowance - usrClaim.amountWithdrawn;
The operation allowance - usrClaim.amountWithdrawn; cannot underflow due to the check on Line 374 which ensures that allowance is greater than usrClaim.amountWithdrawn before performing the arithmetic operation
File: /contracts/VTVLVesting.sol 429: uint112 amountRemaining = finalVestAmt - _claim.amountWithdrawn;
The operation finalVestAmt - _claim.amountWithdrawn cannot underflow due to the check on Line 426 that ensures that finalVestAmt is greater than _claim.amountWithdrawn before performing the arithmetic operation
File: /contracts/VTVLVesting.sol 264: require((_endTimestamp - _startTimestamp) % _releaseIntervalSecs == 0, "INVALID_INTERVAL_LENGTH");
The operation _endTimestamp - _startTimestamp cannot underflow due to the check on Line 262 that ensures that _endTimestamp is greater than _startTimestamp before performing the arithmetic operation.
File: /contracts/VTVLVesting.sol 167: uint40 currentVestingDurationSecs = _referenceTs - _claim.startTimestamp; // How long since the start
The operation _referenceTs - _claim.startTimestamp cannot underflow due to the check on Line 166 that ensures that _referenceTs is greater than _claim.startTimestamp
The majority of Solidity for loops increment a uint256 variable that starts at 0. These increment operations never need to be checked for over/underflow because the variable will never reach the max number of uint256 (will run out of gas long before that happens). The default over/underflow check wastes gas in every iteration of virtually every for loop . eg.
e.g Let's work with a sample loop below.
for(uint256 i; i < 10; i++){ //doSomething }
can be written as shown below.
for(uint256 i; i < 10;) { // loop logic unchecked { i++; } }
We can also write it as an inlined function like below.
function inc(i) internal pure returns (uint256) { unchecked { return i + 1; } } for(uint256 i; i < 10; i = inc(i)) { // doSomething }
Affected code https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L353
File: /contracts/VTVLVesting.sol 353: for (uint256 i = 0; i < length; i++) {
++i costs less gas compared to i++ or i += 1 for unsigned integer, 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
Instances include:
File: /contracts/VTVLVesting.sol 353: for (uint256 i = 0; i < length; i++) {
Instead of using the && operator in a single require statement to check multiple conditions,using multiple require statements with 1 condition per require statement will save 8 GAS per && The gas difference would only be realized if the revert condition is realized(met).
File: /contracts/VTVLVesting.sol 279: require( 280: ( 281: _cliffReleaseTimestamp > 0 && 282: _cliffAmount > 0 && 283: _cliffReleaseTimestamp <= _startTimestamp 284: ) || ( 285: _cliffReleaseTimestamp == 0 && 286: _cliffAmount == 0 287: ), "INVALID_CLIFF");
File: /contracts/VTVLVesting.sol 344: require(_startTimestamps.length == length && 345: _endTimestamps.length == length && 346: _cliffReleaseTimestamps.length == length && 347: _releaseIntervalsSecs.length == length && 348: _linearVestAmounts.length == length && 349: _cliffAmounts.length == length, 350: "ARRAY_LENGTH_MISMATCH" 351: );
Proof The following tests were carried out in remix with both optimization turned on and off
function multiple (uint a) public pure returns (uint){ require ( a > 1 && a < 5, "Initialized"); return a + 2; }
Execution cost 21617 with optimization and using && 21976 without optimization and using &&
After splitting the require statement
function multiple(uint a) public pure returns (uint){ require (a > 1 ,"Initialized"); require (a < 5 , "Initialized"); return a + 2; }
Execution cost 21609 with optimization and split require 21968 without optimization and using split require
Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value. I suggest using if(directValue) instead of if(directValue == true) and if(!directValue) instead of if(directValue == false) here:
File: /contracts/VTVLVesting.sol 111: require(_claim.isActive == true, "NO_ACTIVE_CLAIM");
// 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.
See source Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past
Instances affected include https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/AccessProtected.sol#L12
File: /contracts/AccessProtected.sol 12: mapping(address => bool) private _admins; // user address => admin? mapping
Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) 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
Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries). see Source
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");
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"); 129: require(_claim.startTimestamp == 0, "CLAIM_ALREADY_EXISTS"); 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");