Platform: Code4rena
Start Date: 21/12/2021
Pot Size: $30,000 USDC
Total HM: 20
Participants: 20
Period: 5 days
Judge: Jack the Pug
Total Solo HM: 13
Id: 70
League: ETH
Rank: 11/20
Findings: 2
Award: $241.19
🌟 Selected for report: 7
🚀 Solo Findings: 0
🌟 Selected for report: Dravee
57.4139 USDC - $57.41
Dravee
Increased gas cost
The StakingRewards contract uses Solidity version 0.8.9 which already implements overflow checks by default. At the same time, it uses the SafeMath library from OpenZeppelin which is more gas expensive than the 0.8.* overflow checks.
It should just use the built-in checks and remove SafeMath from the dependencies.
./staking-rewards/StakingRewards.sol:4:import "@openzeppelin/contracts/utils/math/SafeMath.sol"; ./staking-rewards/StakingRewards.sol:19: using SafeMath for uint256;
VS Code
Use the built-in checks here https://github.com/code-423n4/2021-12-vader/blob/main/contracts/staking-rewards/StakingRewards.sol#L19 and remove SafeMath from the dependencies here https://github.com/code-423n4/2021-12-vader/blob/main/contracts/staking-rewards/StakingRewards.sol#L4
15.5017 USDC - $15.50
Dravee
SLOADs cost 2100 gas for first time reads of state variables and then 100 gas for repeated reads in the context of a transaction (post Berlin fork). MLOADs cost 3 gas units. Therefore, caching state variable in local variables for repeated reads saves gas.
If you look in this for-loop, you'll notice that the twapData and usdvPairs storage variables are accessed at each iteration and the resulting extracted value is then written in uint256[] memory pastLiquidityWeights, a memory variable. After that, pastLiquidityWeights is passed as a memory variable to the _calculateUSDVPrice method.
Here, there aren't any reason to use SLOADs instead of MLOADs.
Notice that here and here, the SLOAD is indeed necessary.
VS Code
Read the storage variable only once and store it in a local memory variable. Then use this memory variable instead.
#0 - jack-the-pug
2022-03-12T14:39:38Z
Dup #15
🌟 Selected for report: pauliax
Also found by: Dravee, Jujic, Meta0xNull, robee
7.5338 USDC - $7.53
Dravee
Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.
Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.
Revert strings > 32 bytes are here:
Migrations.sol: 11: "This function is restricted to the contract's owner" dex-v2\pool\BasePoolV2.sol: 96: "BasePoolV2::constructor: Incorrect Arguments" 210: "BasePoolV2::burn: Incorrect Ownership" 228: "BasePoolV2::burn: Insufficient Liquidity Burned" 290: "BasePoolV2::doubleSwap: Insufficient Tokens Provided" 301: "BasePoolV2::doubleSwap: Swap Impossible" 332: "BasePoolV2::doubleSwap: Swap Impossible" 396: "BasePoolV2::swap: Only One-Sided Swaps Supported" 409: "BasePoolV2::swap: Invalid Receiver" 420: "BasePoolV2::swap: Swap Impossible" 431: "BasePoolV2::swap: Swap Impossible" 507: "BasePoolV2::mint: Insufficient Liquidity Provided" 555: "BasePoolV2::_update: Balance Overflow" 590: "BasePoolV2::_supportedToken: Unsupported Token" 600: "BasePoolV2::_onlyRouter: Only Router is allowed to call" dex-v2\pool\VaderPoolV2.sol: 123: "VaderPoolV2::initialize: Already initialized" 127: "VaderPoolV2::initialize: Incorrect Wrapper Specified" 131: "VaderPoolV2::initialize: Incorrect SynthFactory Specified" 135: "VaderPoolV2::initialize: Incorrect Router Specified" 215: "VaderPoolV2::burnSynth: Inexistent Synth" 220: "VaderPoolV2::burnSynth: Insufficient Synth Amount" 322: "VaderPoolV2::mintFungible: Unsupported Token" 346: "VaderPoolV2::mintFungible: Insufficient Liquidity Provided" 389: "VaderPoolV2::burnFungible: Unsupported Token" 406: "VaderPoolV2::burnFungible: Insufficient Liquidity Burned" 430: "VaderPoolV2::setQueue: Already At Desired State" 452: "VaderPoolV2::supportToken: Already At Desired State" 459: "VaderPoolV2::supportToken: Cannot Unsupport Token w/ Liquidity" 464: "VaderPoolV2::supportToken: Improper First-Time Liquidity Provision" 486: "VaderPoolV2::setGasThrottle: Already At Desired State" lbt\LiquidityBasedTWAP.sol: 41: "LBTWAP::construction: Zero Address" 93: "LBTWAP::getChainlinkPrice: Stale Chainlink Price" 96: require(price > 0, "LBTWAP::getChainlinkPrice: Chainlink Malfunction"); 233: "LBTWAP::setupVader: Already Initialized" 249: "LBTWAP::addVaderPair: Vader Uninitialized" 262: "LBTWAP::addVaderPair: Incorrect Update Period" 265: require(oracle.decimals() == 8, "LBTWAP::addVaderPair: Non-USD Oracle"); 277: require(nativeAsset == vader, "LBTWAP::addVaderPair: Unsupported Pair"); 429: "LBTWAP::setupUSDV: Already Initialized" 445: "LBTWAP::addUSDVPair: USDV Uninitialized" 458: "LBTWAP::addUSDVPair: Incorrect Update Period" 461: require(oracle.decimals() == 8, "LBTWAP::addUSDVPair: Non-USD Oracle"); staking-rewards\Owned.sol: 20: require(msg.sender == nominatedOwner, "You must be nominated before you can accept ownership"); 32: require(msg.sender == owner, "Only the contract owner may perform this action"); staking-rewards\StakingRewards.sol: 170: "Cannot withdraw the staking token" 179: "Previous rewards period must be complete before changing the duration for the new period" tokens\USDV.sol: 57: "USDV::constructor: Improper Configuration" 85: "USDV::mint: 24 Hour Limit Reached" 175: "USDV::initialize: Improper Configuration" 202: "USDV::setLock: Insufficient Privileges" tokens\Vader.sol: 105: "Vader::setComponents: Incorrect Arguments" 109: "Vader::setComponents: Already Set" 137: require(_usdv != IUSDV(_ZERO_ADDRESS), "Vader::setUSDV: Invalid USDV address"); 138: require(usdv == IUSDV(_ZERO_ADDRESS), "Vader::setUSDV: USDV already set"); 157: require(amount != 0, "Vader::claimGrant: Non-Zero Amount Required"); 176: "Vader::adjustMaxSupply: Max supply cannot subcede current supply" 225: "Vader::_onlyUSDV: Insufficient Privileges" tokens\converter\Converter.sol: 68: "Converter::constructor: Misconfiguration" 92: "Converter::setVesting: Vesting is already set" 96: "Converter::setVesting: Cannot Set Zero Vesting Address" 126: "Converter::convert: Non-Zero Conversion Amount Required" 133: "Converter::convert: Vesting is not set" 141: "Converter::convert: Incorrect Proof Provided" tokens\vesting\LinearVesting.sol: 60: "LinearVesting::constructor: Misconfiguration" 112: "LinearVesting::claim: Incorrect Vesting Type" 117: "LinearVesting::claim: Not Started Yet" 127: require(vestedAmount != 0, "LinearVesting::claim: Nothing to claim"); 158: "LinearVesting::begin: Vesters and Amounts lengths do not match" 172: "LinearVesting::begin: Incorrect Amount Specified" 176: "LinearVesting::begin: Zero Vester Address Specified" 180: "LinearVesting::begin: Duplicate Vester Entry Specified" 192: "LinearVesting::begin: Invalid Vest Amounts Specified" 197: "LinearVesting::begin: Vader is less than TEAM_ALLOCATION" 219: "LinearVesting::vestFor: Amount Overflows uint192" 223: "LinearVesting::vestFor: Already a vester" 256: "LinearVesting::_hasStarted: Vesting hasn't started yet" 266: "LinearVesting::_onlyConverter: Only converter is allowed to call"
Visual Studio Code
Shorten the revert strings to fit in 32 bytes.
Or consider using Custom Errors (solc >=0.8.4). I can see that the versions of Solidity used on the contracts are recent enough but the Custom Errors are never used. Here's some documentation on how to do it: https://solidity-by-example.org/error/
#0 - jack-the-pug
2022-03-12T14:44:00Z
Dup #188
15.5017 USDC - $15.50
Dravee
Public functions that are never called by the contract should be declared external to save gas.
This function is never called by the contract : https://github.com/code-423n4/2021-12-vader/blob/main/contracts/Migrations.sol#L16-L18
VS Code
Change function visibility from public to external
#0 - jack-the-pug
2022-03-12T14:46:35Z
Dup #175
15.5017 USDC - $15.50
Dravee
++i costs less gass compared to i++ for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration)
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:
./tokens/USDV.sol:152: for (uint256 i = 0; i < userLocks.length; i++) {
VS Code
Use ++i instead of i++ to increment the value of an uint variable.
It's done everywhere else in the project
25.8362 USDC - $25.84
Dravee
private functions are cheaper than internal functions.
Several internal functions are in contracts that are never inherited.
Their internal keywords are there:
lbt\LiquidityBasedTWAP.sol: 154: ) internal returns (uint256 currentLiquidityEvaluation) { 194: ) internal view returns (uint256) { 255: ) internal { 357: ) internal returns (uint256 currentLiquidityEvaluation) { 388: ) internal view returns (uint256) { 449: ) internal { tokens\converter\Converter.sol: 166: function getChainId() internal view returns (uint256 chainId) {
Therefore, their visibility should be reduced to private.
VS Code
Define these functions as private.
🌟 Selected for report: Dravee
57.4139 USDC - $57.41
Dravee
uint a = b++; is an error-prone syntax that is often misunderstood by developers.
5 gas can be saved with a pre-increment after the assignment.
The uint256 id = positionId++; code is here https://github.com/code-423n4/2021-12-vader/blob/main/contracts/dex-v2/pool/BasePoolV2.sol#L510:
It's an often misunderstood syntax as after this line, id == positionId - 1 (or id + 1 == positionId) is the true statement.
In short, the value of positionId is first stored in uint256 id, and then the variable positionId is incremented by 1.
Manual Review
The existing short-syntax is not worth the cost on code clarity (more so than the 5 gas saved). I'd advise you divide the statement over 2 lines:
// Before uint256 id = positionId++; // After uint256 id = positionId; ++positionId; // pre-increment costs 5 gas less than post-increment
🌟 Selected for report: Dravee
Also found by: Meta0xNull, robee
15.5017 USDC - $15.50
Dravee
Explicit initialization with zero is not required for variable declaration because uints are 0 by default. Removing this will reduce contract size and save a bit of gas.
Instances include:
./staking-rewards/StakingRewards.sol:26: uint256 public periodFinish = 0; ./staking-rewards/StakingRewards.sol:27: uint256 public rewardRate = 0; ./tokens/USDV.sol:152: for (uint256 i = 0; i < userLocks.length; i++) { ./tokens/vesting/LinearVesting.sol:169: for (uint256 i = 0; i < vesters.length; ++i) {
Manual Analysis
Remove explicit initialization with zero. It's already done at several places and in most for-loops, which means that this optimization would be consistent with the overall code style.
15.5017 USDC - $15.50
Dravee
Redundant arithmetic underflow/overflow checks can be avoided when an underflow/overflow cannot happen.
The "unchecked" keyword can be applied here since there is an "if" statement before to ensure the arithmetic operations, would not cause an integer underflow or overflow.
Instances include: https://github.com/code-423n4/2021-12-vader/blob/main/contracts/dex-v2/pool/VaderPoolV2.sol#L78-L79
Add the "unchecked" keyword.
if(a < b){ unchecked { b - a } }
Or move the line inside the unchecked statement right below it
15.5017 USDC - $15.50
Dravee
Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.
Caching the array length in the stack saves around 3 gas per iteration.
./tokens/USDV.sol:152: for (uint256 i = 0; i < userLocks.length; i++) { ./tokens/vesting/LinearVesting.sol:169: for (uint256 i = 0; i < vesters.length; ++i) {
VS Code
Store the array's length in a variable before the for-loop, and use it instead. In the project, it's done everywhere else other than the places mentionned.