veToken Finance contest - _Adam's results

QA01 Add 2 step Changes for Critical Changes

Critical changes such as ownership changes should be a 2 step process to protect against human error. While the errors are unlikely important parts of the contract would become unusable if they occured. Consider changing the following functions to 2 step procedures.

VeAssetDepositor.sol#L53 Booster.sol#L123 Booster.sol#L130 VoterProxy.sol#L62

QA02 Replace safeApprove() when updating balances

As per openZeppelin when updating balances safeApprove has been deprecated in favor of using safeIncreaseAllowance.

VeAssetDepositor.sol#L162 Booster.sol#L374 VE3DRewardPool.sol#L288-L291 VE3DRewardPool.sol#L305-L307 VoterProxy.sol#L102 VoterProxy.sol#L153 VoterProxy.sol#L161

QA03 Booster.sol Fee Ranges Comment

There is a comment in Booster.sol stating values must be within certain ranges but there no checks to ensure that they are. I recommend either removing the comment or adding checks ensuring they fall within the desired ranges.

Booster.sol#L233

Example: add the following checks with lowerNum/upperNum replaced with the desired values. require(_lockFees >= lowerNum && _lockFees <= upperNum, "invalid lockFees"); require(_stakerFees >= lowerNum && _stakerFees <= upperNum, "invalid stakerFees"); require(_stakerLockIncentiveFee >= lowerNum && _stakerLockIncentiveFee <= upperNum, "invalid stakerLockIncentiveFees"); require(_callerFees >= lowerNum && _callerFees <= upperNum, "invalid callerFees"); require(_platform >= lowerNum && _platform <= upperNum, "invalid _platform fee");

G01 Initialising to Default Values

When initialising a variable to its default variable, it is cheaper to leave blank. I ran a test in remix that initialises a single variable and got a saving of 2,246 gas. contract test { uint256 public variable = 0; (69,312 gas) vs uint256 public variable; (67,066 gas) }

BaseRewardPool.sol#L66 BaseRewardPool.sol#L67 BaseRewardPool.sol#L70 BaseRewardPool.sol#L71 BaseRewardPool.sol#L72 ExtraRewardStashV1.sol#L29 VE3DLocker.sol#L106 VeAssetDepositor.sol#L28 VirtualBalanceRewardPool.sol#L74 VirtualBalanceRewardPool.sol#L75 VirtualBalanceRewardPool.sol#L78 VirtualBalanceRewardPool.sol#L79 VirtualBalanceRewardPool.sol#L80

G02 Looping Over Storage Variables

Whenever looping over the length of a storage variable in a for loop, it will cost an SLOAD (100 gas) every iteration. By cacheing the length in a local variable before the loop you can save 97 gas (3 gas for the MLOAD) per iteration past the first one. Another small gas savings that can be done in for loops is replacing the ++i with i++. This saves 1 variable assignment per iteration. (5 gas per iteration)

BaseRewardPool.sol#L176 - can cache extraRewards.length BaseRewardPool.sol#L199 - can cache extraRewards.length BaseRewardPool.sol#L218 - can cache extraRewards.length BaseRewardPool.sol#L245 - can cache extraRewards.length BaseRewardPool.sol#L282 - can cache extraRewards.length Booster.sol#L329 - can cache poolInfo.length ExtraRewardStashV2.sol#L140 - can cache tokenCount ExtraRewardStashV2.sol#L181 - can cache tokenCount ExtraRewardStashV2.sol#L213 - can cache tokenCount ExtraRewardStashV3.sol#L126 - can cache tokenCount VE3DLocker.sol#L207 - can cache rewardTokens.length VE3DLocker.sol#L315 - can cache locks.length VE3DLocker.sol#L360 - can cache locks.length VE3DLocker.sol#L457 - can cache locks.length VE3DLocker.sol#L720 - can cache rewardTokens.length VE3DLocker.sol#L803 - can cache rewardTokens.length VE3DRewardPool.sol#L148 - can cache rewardTokens.length VE3DRewardPool.sol#L281 - can cache rewardTokens.length

G03 Minimizing SLOAD's

Whenever a storage variable is used more than once but never modified, gas can be saved by cacheing to memory and using that instead. It will cost 103 gas to SLOAD/MSTORE initially, and then 3 gas per MLOAD for each use vs 100 gas per use to SLOAD for a saving of roughly 97 gas for every use past the first one.

Booster.sol#L578-L592 - can cache feeToken to save up to ~400 Gas Booster.sol#L526-L530 - can cache treasury to save ~200 Gas Booster.sol#L526-L528 - can cache platformFee to save ~100 Gas ExtraRewardStashV1.sol#L82-L120 - can cache staker to save ~200 Gas VE3DLocker.sol#L764-L767 - can cache rdata.periodFinish to save ~100 Gas VeTokenMinter.sol#L59-L63 - can cache totalCliffs to save ~200 Gas

G04 Reasigning variable with same value

In Booster.sol isShutdown is already initialised to the default value false when it is created and does not need to be reasigned to the same value in the constructor.

Booster.sol#L58 Booster.sol#L110

G05 Immutable Variables

Variables that are initialised in the constructor and then never modified can be changed to immutable. Consider changing the following:

In BaseRewardPool.sol operator, rewardManager & pid. BaseRewardPool.sol#L62-L65

In VeAssetDepositor.sol maxTime. VeAssetDepositor.sol#L30

In VoterProxy.sol name. VoterProxy.sol#L30

In VeTokenMinter.sol reductionPerCliff and totalCliffs. VeTokenMinter.sol#L18-L19

G06 Custom Errors

As your using a solidity version greater 0.8.4 you can replace revert strings with custom errors. This will save in deployment costs and runtime costs. I ran a test in remix comparing a revert string vs custom errors and found that replacing a single revert string with a custom error saved 12,404 gas in deployment cost and 86 gas on each function call.

contract Test { uint256 a; function check() external { require(a != 0, "check failed"); } } (114,703 in deployment cost, 23392 when function check() called) vs contract Test { uint256 a; error checkFailed(); function check() external { if (a != 0) { revert checkFailed(); } } } (102,299 in deployment cost, 23306 when function check() called)

There are 156 revert strings throughout the various contracts and replacing each of these with custom errors could have a fairly large impact on gas costs.

G07 Long Revert Strings

If opting to not use custom errors, revert strings can be kept < 32 bytes in length to save gas. I ran a test in remix and with optimisation set to 10,000 and found the savings for a single short revert string vs long string to be 9,377 gas in deployment cost and 18 gas on function call.

contract Test { uint256 a; function check() external { require(a != 0, "short error message"); (Deployment cost: 114,799, Cost on function call: 23,392) vs require(a != 0, "A longer Error Message over 32 bytes in length"); (Deployment cost: 124,176, Cost on function call: 23,410) } }

BitMath.sol#L8 BitMath.sol#L45 FixedPoint.sol#L85 FixedPoint.sol#L105 FixedPoint.sol#L131 FixedPoint.sol#L149 Migrations.sol#L9

G08 Uint > 0 Checks in Require Statements

When checking whether a uint is > 0 in a requrie statement you can save a small amount of gas by replacing with != 0. This is only true if optimiser is turned on and in a require statement. I ran a test in remix with optimisation set to 10,000 and found the savings for a single occurance is 632 in deployment cost and 6 gas on each function call.

contract Test { uint256 a; function check() external { require(a > 0); (Deployment cost: 79,763, Cost on function call: 23,305) vs require(a != 0); (Deployment cost: 79,331, Cost on function call: 23,299) } }

BitMath.sol#L8 BitMath.sol#L45 BoringMath.sol#L20 BoringMath.sol#L102 BoringMath.sol#L122 BoringMath.sol#L142 FixedPoint.sol#L131 BaseRewardPool.sol#L173 BaseRewardPool.sol#L196 BaseRewardPool.sol#L215 VE3DLocker.sol#L180 VE3DLocker.sol#L520 VE3DLocker.sol#L670 VE3DLocker.sol#L781 VE3DRewardPool.sol#L210 VE3DRewardPool.sol#L234 VE3DRewardPool.sol#L253 VeAssetDepositor.sol#L132