Platform: Code4rena
Start Date: 26/05/2022
Pot Size: $75,000 USDT
Total HM: 31
Participants: 71
Period: 7 days
Judge: GalloDaSballo
Total Solo HM: 18
Id: 126
League: ETH
Rank: 5/71
Findings: 5
Award: $4,587.38
🌟 Selected for report: 3
🚀 Solo Findings: 1
🌟 Selected for report: kirk-baird
Also found by: Dravee, IllIllI, Koustre, Ruhum, VAD37, csanuragjain, gzeon, unforgiven, xiaoming90
80.6857 USDT - $80.69
https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DRewardPool.sol#L102-L112 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeTokenMinter.sol#L77-L81
Admins can lock funds forever, essentially rugging their users
By adding a lot of reward tokens:
File: contracts/VE3DRewardPool.sol #5 102 function addReward( 103 address _rewardToken, 104 address _veAssetDeposits, 105 address _ve3TokenRewards, 106 address _ve3Token 107 ) external onlyOwner { 108 rewardTokenInfo[_rewardToken].veAssetDeposits = _veAssetDeposits; 109 rewardTokenInfo[_rewardToken].ve3TokenRewards = _ve3TokenRewards; 110 rewardTokenInfo[_rewardToken].ve3Token = _ve3Token; 111 rewardTokens.add(_rewardToken); 112 }
And then transferring admin privs to another account and then throwing away that account's private key (so that recovery can't be started), an admin can cause user operations to revert whenever they attempt to get their funds:
File: contracts/VE3DRewardPool.sol #4 146 modifier updateReward(address account) { 147 address _rewardToken; 148 for (uint256 i = 0; i < rewardTokens.length(); i++) { 149 _rewardToken = rewardTokens.at(i); 150 rewardTokenInfo[_rewardToken].rewardPerTokenStored = rewardPerToken(_rewardToken); 151 rewardTokenInfo[_rewardToken].lastUpdateTime = lastTimeRewardApplicable(_rewardToken); 152 if (account != address(0)) { 153 rewardTokenInfo[_rewardToken].rewards[account] = earnedReward( 154 _rewardToken, 155 account 156 ); 157 rewardTokenInfo[_rewardToken].userRewardPerTokenPaid[account] = rewardTokenInfo[ 158 _rewardToken 159 ].rewardPerTokenStored; 160 } 161 } 162 163 _; 164 }
The modifier is used on this function, and all functions where the user can claim funds:
File: contracts/VE3DRewardPool.sol #3 252 function withdraw(uint256 _amount, bool claim) public updateReward(msg.sender) {
This function also would let the admin arbitrarily withdraw any funds and would be another attack vector:
File: contracts/VeTokenMinter.sol #4 77 function withdraw(address _destination, uint256 _amount) external onlyOwner { 78 veToken.safeTransfer(_destination, _amount); 79 80 emit Withdraw(_destination, _amount); 81 }
Code inspection
Have a maximum number of reward tokens, as is done in other places in the codebase
#0 - solvetony
2022-06-15T17:07:22Z
Also parts duplicates in #239 #222 #136 Lower severity
#1 - GalloDaSballo
2022-07-24T21:09:26Z
Dup of #136
🌟 Selected for report: GalloDaSballo
Also found by: IllIllI
https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeTokenMinter.sol#L48-L49 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DRewardPool.sol#L341-L344
Operators have the ability to mint arbitrary amounts and thus can cause massive inflation, which is a tax on all holders
Unlike the original Convex code that goes to great lengths to prevent users having the ability to transfer funds/mint things, the operator role in this project is grantable by the owner admin:
File: contracts/VeTokenMinter.sol #1 32 function addOperator(address _newOperator) public onlyOwner { 33 operators.add(_newOperator); 34 }
And can do things they shouldn't need to do. mint() only requires that the message sender be a member of the operators group
File: contracts/VeTokenMinter.sol #2 48 function mint(address _to, uint256 _amount) external { 49 require(operators.contains(_msgSender()), "not an operator");
Operators also can queue new arbitrary rewards:
File: contracts/VE3DRewardPool.sol #3 341 function queueNewRewards(address _rewardToken, uint256 _rewards) external { 342 require(operators.contains(_msgSender()), "!authorized"); 343 344 _rewards = _rewards.add(rewardTokenInfo[_rewardToken].queuedRewards);
Code inspection
Don't allow the admin to arbitrarily add new operators
#0 - jetbrain10
2022-06-15T21:03:31Z
All admin role is controlled by DAO
#1 - GalloDaSballo
2022-07-25T00:52:26Z
Dup of #108
🌟 Selected for report: IllIllI
2082.6377 USDT - $2,082.64
The admin may fat-finger a change, or be malicious, and have the weights be extreme - ranging from zero to type(uint256).max, which would cause the booster to pay out unexpected amounts
No bounds checks in the update function:
File: contracts/VeTokenMinter.sol #1 41 function updateveAssetWeight(address veAssetOperator, uint256 newWeight) external onlyOwner { 42 require(operators.contains(veAssetOperator), "not an veAsset operator"); 43 totalWeight -= veAssetWeights[veAssetOperator]; 44 veAssetWeights[veAssetOperator] = newWeight; 45 totalWeight += newWeight; 46 }
The value is used by the reward contract to determine how much to mint:
File: contracts/Booster.sol #2 598 function rewardClaimed( 599 uint256 _pid, 600 address _address, 601 uint256 _amount 602 ) external returns (bool) { 603 address rewardContract = poolInfo[_pid].veAssetRewards; 604 require(msg.sender == rewardContract || msg.sender == lockRewards, "!auth"); 605 ITokenMinter veTokenMinter = ITokenMinter(minter); 606 //calc the amount of veAssetEarned 607 uint256 _veAssetEarned = _amount.mul(veTokenMinter.veAssetWeights(address(this))).div( 608 veTokenMinter.totalWeight() 609 ); 610 //mint reward tokens 611 ITokenMinter(minter).mint(_address, _veAssetEarned);
Wrong values will lead to excessive inflation/deflation
Code inspection
Have sane upper/lower limits on the values
#0 - solvetony
2022-06-15T17:10:03Z
We may consider to add this
#1 - GalloDaSballo
2022-07-25T00:51:49Z
The warden has shown how due to a lack of checks certain assets may provide a disproportionate amount of rewards.
Because this is contingent on an admin mistake, and the impact would be loss or gain of Yield; I believe Medium Severity to be appropriate.
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0x29A, 0xDjango, 0xNazgul, 0xf15ers, BouSalman, Chom, Deivitto, Dravee, ElKu, FSchmoede, Funen, GimelSec, Hawkeye, MiloTruck, Picodes, SecureZeroX, SmartSek, TerrierLover, WatchPug, _Adam, asutorufos, berndartmueller, c3phas, catchup, cccz, cogitoergosumsw, cryptphi, csanuragjain, delfin454000, dipp, ellahi, gzeon, hansfriese, horsefacts, hyh, kirk-baird, minhquanym, oyc_109, pauliax, reassor, robee, sashik_eth, shenwilly, simon135, sorrynotsorry, sseefried, unforgiven, xiaoming90, z3s
977.6786 USDT - $977.68
| Issue | Instances | |
|---|---|---|
| 1 | getAPY() returns the wrong answer during leap years | 1 |
| 2 | Change in behavior from Convex | 1 |
| 3 | safeApprove() is deprecated | 12 |
| 4 | Missing checks for address(0x0) when assigning values to address state variables | 26 |
Total: 40 instances over 4 issues
| Issue | Instances | |
|---|---|---|
| 1 | Adding a return statement when the function defines a named return variable, is redundant | 1 |
| 2 | public functions not called by the contract should be declared external instead | 8 |
| 3 | constants should be defined rather than using magic numbers | 10 |
| 4 | Numeric values having to do with time should use time units for readability | 1 |
| 5 | Large multiples of ten should use scientific notation (e.g. 1e6) rather than decimal literals (e.g. 1000000), for readability | 1 |
| 6 | Missing event for critical parameter change | 5 |
| 7 | Use a more recent version of solidity | 6 |
| 8 | Constant redefined elsewhere | 10 |
| 9 | Inconsistent spacing in comments | 17 |
| 10 | Typos | 8 |
| 11 | File is missing NatSpec | 3 |
| 12 | NatSpec is incomplete | 1 |
| 13 | Event is missing indexed fields | 20 |
| 14 | Avoid the use of sensitive terms | 1 |
Total: 92 instances over 14 issues
getAPY() returns the wrong answer during leap yearsThere are more blocks in a year during a leap year. Using a static value for the number of blocks in a year will eventually lead to the wrong answer
There is 1 instance of this issue:
File: contracts/BaseRewardPool.sol #1 343 function getAPY() external view returns (uint256) { 344 return rewardRate.mul(BLOCKS_PER_YEAR).mul(1e18).div(totalSupply()); 345: }
In the convex version of this function, the function returns the actual balanceOf() regardless of whether there was an exception or not. It may be confusing to users trying to port their code to have to account for this behavior, especially with the lack of comments about the difference
There is 1 instance of this issue:
File: contracts/VoterProxy.sol #1 227 uint256 _balance = 0; 228 229 if (escrowModle == IVoteEscrow.EscrowModle.PICKLE) { 230 try IGauge(_gauge).getReward() {} catch { 231 return _balance; 232 } 233 } else if ( 234 escrowModle == IVoteEscrow.EscrowModle.CURVE || 235 escrowModle == IVoteEscrow.EscrowModle.RIBBON 236 ) { 237 try ITokenMinter(minter).mint(_gauge) {} catch { 238 return _balance; 239 } 240 } else if (escrowModle == IVoteEscrow.EscrowModle.IDLE) { 241 try ITokenMinter(minter).distribute(_gauge) {} catch { 242 return _balance; 243 } 244 } else if (escrowModle == IVoteEscrow.EscrowModle.ANGLE) { 245 try IGauge(_gauge).claim_rewards() {} catch { 246 return _balance; 247 } 248 } 249 250: _balance = IERC20(veAsset).balanceOf(address(this));
safeApprove() is deprecatedDeprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance(). If only setting the initial allowance to the value that means infinite, safeIncreaseAllowance() can be used instead
There are 12 instances of this issue:
File: contracts/VE3DRewardPool.sol 287: IERC20(_rewardToken).safeApprove(rewardTokenInfo[_rewardToken].veAssetDeposits, 0); 288: IERC20(_rewardToken).safeApprove( 301: IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove( 305: IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove(
File: contracts/VoterProxy.sol 101: IERC20(_token).safeApprove(_gauge, 0); 102: IERC20(_token).safeApprove(_gauge, balance); 152: IERC20(veAsset).safeApprove(escrow, 0); 153: IERC20(veAsset).safeApprove(escrow, _value); 160: IERC20(veAsset).safeApprove(escrow, 0); 161: IERC20(veAsset).safeApprove(escrow, _value);
File: contracts/VeAssetDepositor.sol 162: IERC20(minter).safeApprove(_stakeAddress, _amount);
File: contracts/Booster.sol 374: IERC20(token).safeApprove(rewardContract, _amount);
address(0x0) when assigning values to address state variablesThere are 26 instances of this issue:
File: contracts/VE3DRewardPool.sol 99: rewardManager = rewardManager_;
File: contracts/VoterProxy.sol 50: veAsset = _veAsset; 51: escrow = _escrow; 52: gaugeProxy = _gaugeProxy; 54: minter = _minter; 64: owner = _owner; 74: operator = _operator; 80: depositor = _depositor;
File: contracts/VeAssetDepositor.sol 45: staker = _staker; 46: minter = _minter; 47: veAsset = _veAsset; 48: escrow = _escrow;
File: contracts/BaseRewardPool.sol 105: operator = operator_; 106: rewardManager = rewardManager_;
File: contracts/Booster.sol 111: staker = _staker; 116: minter = _minter; 117: veAsset = _veAsset; 118: feeDistro = _feeDistro; 131: feeManager = _feeM; 137: poolManager = _poolM; 152: rewardFactory = _rfactory; 153: tokenFactory = _tfactory; 159: stashFactory = _sfactory; 164: rewardArbitrator = _arb; 184: lockRewards = _rewards; 215: feeToken = _feeToken;
return statement when the function defines a named return variable, is redundantThere is 1 instance of this issue:
File: contracts/VoterProxy.sol #1 119: return balance;
public functions not called by the contract should be declared external insteadContracts are allowed to override their parents' functions and change the visibility from external to public.
There are 8 instances of this issue:
File: contracts/VE3DRewardPool.sol 114: function addOperator(address _newOperator) public onlyOwner { 118: function removeOperator(address _operator) public onlyOwner { 233: function stakeFor(address _for, uint256 _amount) public updateReward(_for) {
File: contracts/VoterProxy.sol 199: function isValidSignature(bytes32 _hash, bytes memory) public view returns (bytes4) {
File: contracts/VeTokenMinter.sol 32: function addOperator(address _newOperator) public onlyOwner { 36: function removeOperator(address _operator) public onlyOwner {
File: contracts/BaseRewardPool.sol 195: function stakeFor(address _for, uint256 _amount) public updateReward(_for) returns (bool) {
File: contracts/Booster.sol 434: function withdrawAll(uint256 _pid) public returns (bool) {
constants should be defined rather than using magic numbersThere are 10 instances of this issue:
File: contracts/VE3DRewardPool.sol /// @audit 1e18 180: .mul(1e18) /// @audit 1e18 193: .div(1e18) /// @audit 1000 358: uint256 queuedRatio = currentAtNow.mul(1000).div(_rewards);
File: contracts/VoterProxy.sol /// @audit 0xffffffff 203: return 0xffffffff;
File: contracts/VeAssetDepositor.sol /// @audit 30 62: if (_lockIncentive >= 0 && _lockIncentive <= 30) {
File: contracts/VeTokenMinter.sol /// @audit 1000 28: totalCliffs = 1000;
File: contracts/BaseRewardPool.sol /// @audit 1e18 158: lastTimeRewardApplicable().sub(lastUpdateTime).mul(rewardRate).mul(1e18).div( /// @audit 1e18 168: .div(1e18) /// @audit 1000 315: uint256 queuedRatio = currentAtNow.mul(1000).div(_rewards); /// @audit 1e18 344: return rewardRate.mul(BLOCKS_PER_YEAR).mul(1e18).div(totalSupply());
There are units for seconds, minutes, hours, days, and weeks
There is 1 instance of this issue:
File: contracts/VeAssetDepositor.sol #1 /// @audit 86400 18: uint256 private constant WEEK = 7 * 86400;
1e6) rather than decimal literals (e.g. 1000000), for readabilityThere is 1 instance of this issue:
File: contracts/VeTokenMinter.sol #1 15: uint256 public constant maxSupply = 30 * 1000000 * 1e18; //30mil
There are 5 instances of this issue:
File: contracts/VoterProxy.sol 62 function setOwner(address _owner) external { 63 require(msg.sender == owner, "!auth"); 64 owner = _owner; 65: } 67 function setOperator(address _operator) external { 68 require(msg.sender == owner, "!auth"); 69 require( 70 operator == address(0) || IDeposit(operator).isShutdown() == true, 71 "needs shutdown" 72 ); 73 74 operator = _operator; 75: } 77 function setDepositor(address _depositor) external { 78 require(msg.sender == owner, "!auth"); 79 80 depositor = _depositor; 81: }
File: contracts/VeTokenMinter.sol 41 function updateveAssetWeight(address veAssetOperator, uint256 newWeight) external onlyOwner { 42 require(operators.contains(veAssetOperator), "not an veAsset operator"); 43 totalWeight -= veAssetWeights[veAssetOperator]; 44 veAssetWeights[veAssetOperator] = newWeight; 45 totalWeight += newWeight; 46: }
File: contracts/Booster.sol 193 function setFeeInfo(uint256 _lockFeesIncentive, uint256 _stakerLockFeesIncentive) external { 194 require(msg.sender == feeManager, "!auth"); 195 196 lockFeesIncentive = _lockFeesIncentive; 197 stakerLockFeesIncentive = _stakerLockFeesIncentive; 198 199 address _feeToken = IFeeDistro(feeDistro).token(); 200 if (feeToken != _feeToken) { 201 //create a new reward contract for the new token 202 lockFees = IRewardFactory(rewardFactory).CreateTokenRewards(_feeToken, lockRewards); 203 204 if (_feeToken != veAsset) { 205 IRewards(stakerLockRewards).addReward( 206 _feeToken, 207 address(0), 208 address(0), 209 address(0), 210 address(this), 211 false 212 ); 213 } 214 215 feeToken = _feeToken; 216 } 217: }
Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions
There are 6 instances of this issue:
File: contracts/VE3DRewardPool.sol 2: pragma solidity 0.8.7;
File: contracts/VoterProxy.sol 2: pragma solidity 0.8.7;
File: contracts/VeAssetDepositor.sol 2: pragma solidity 0.8.7;
File: contracts/VeTokenMinter.sol 2: pragma solidity 0.8.7;
File: contracts/BaseRewardPool.sol 2: pragma solidity 0.8.7;
File: contracts/Booster.sol 2: pragma solidity 0.8.7;
Consider defining in only one contract so that values cannot become out of sync when only one location is updated. A cheap way to store constants in a single location is to create an internal constant in a library. If the variable is a local cache of another contract's value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don't get out of sync.
There are 10 instances of this issue:
File: contracts/VeAssetDepositor.sol /// @audit seen in /var/tmp/hh/contracts/VE3DRewardPool.sol 21: uint256 public constant FEE_DENOMINATOR = 10000; /// @audit seen in /var/tmp/hh/contracts/VoterProxy.sol 23: address public immutable veAsset; /// @audit seen in /var/tmp/hh/contracts/VoterProxy.sol 24: address public immutable escrow; /// @audit seen in /var/tmp/hh/contracts/VoterProxy.sol 27: address public immutable minter;
File: contracts/BaseRewardPool.sol /// @audit seen in /var/tmp/hh/contracts/VE3DRewardPool.sol 57: uint256 public constant duration = 7 days; /// @audit seen in /var/tmp/hh/contracts/VE3DRewardPool.sol 73: uint256 public constant newRewardRatio = 830;
File: contracts/Booster.sol /// @audit seen in /var/tmp/hh/contracts/VeAssetDepositor.sol 34: uint256 public constant FEE_DENOMINATOR = 10000; /// @audit seen in /var/tmp/hh/contracts/VeAssetDepositor.sol 42: address public immutable staker; /// @audit seen in /var/tmp/hh/contracts/VeAssetDepositor.sol 43: address public immutable minter; /// @audit seen in /var/tmp/hh/contracts/VeAssetDepositor.sol 44: address public immutable veAsset;
Some lines use // x and some use //x. The instances below point out the usages that don't follow the majority, within each file
There are 17 instances of this issue:
File: contracts/VE3DRewardPool.sol 68: // reward token => reward token info 70: // list of reward tokens
File: contracts/VoterProxy.sol 122: // Withdraw partial funds
File: contracts/Booster.sol 23: // ve3Token reward pool 25: // veToken reward pool 27: // veToken locking reward pool xVE3D 29: // caller reward 31: // platoform fee 53: address public stakerLockRewards; // veToken lock rewards xVE3D 121: /// SETTER SECTION /// 192: // Set reward token and claim contract, get from Curve's registry 249: /// END SETTER SECTION /// 295: // voteproxy so it can grab the incentive tokens off the contract after claiming rewards 296: // reward factory so that stashes can make new extra reward contracts if a new incentive is added to the gauge 323: // unstake and pull all lp tokens to this address 324: // only allow withdrawals 409: // if shutdown tokens will be in this contract
There are 8 instances of this issue:
File: contracts/VoterProxy.sol /// @audit modle -> model 234: escrowModle == IVoteEscrow.EscrowModle.CURVE ||
File: contracts/VE3DRewardPool.sol /// @audit dont 204: //fees dont apply until whitelist+veVeAsset lock begins so will report
File: contracts/VeAssetDepositor.sol /// @audit ammount 93: //increase ammount /// @audit isnt 126: //the vetoken reward contract isnt costly to claim rewards
File: contracts/VeTokenMinter.sol /// @audit 30mil 15: uint256 public constant maxSupply = 30 * 1000000 * 1e18; //30mil
File: contracts/Booster.sol /// @audit platoform 31: // platoform fee /// @audit seperate 363: //some gauges claim rewards when depositing, stash them in a seperate contract until next claim /// @audit seperate 414: //some gauges claim rewards when withdrawing, stash them in a seperate contract until next claim
There are 3 instances of this issue:
File: contracts/VE3DRewardPool.sol (various lines) #1
File: contracts/VeAssetDepositor.sol (various lines) #2
File: contracts/BaseRewardPool.sol (various lines) #3
There is 1 instance of this issue:
File: contracts/VoterProxy.sol #1 /// @audit Missing: '@param bytes' 191 /** 192 * @notice Verifies that the hash is valid 193 * @dev Snapshot Hub will call this function when a vote is submitted using 194 * snapshot.js on behalf of this contract. Snapshot Hub will call this 195 * function with the hash and the signature of the vote that was cast. 196 * @param _hash Hash of the message that was sent to Snapshot Hub to cast a vote 197 * @return EIP1271 magic value if the signature is value 198 */ 199: function isValidSignature(bytes32 _hash, bytes memory) public view returns (bytes4) {
indexed fieldsEach event should use three indexed fields if there are three or more fields
There are 20 instances of this issue:
File: contracts/VE3DRewardPool.sol 91: event RewardAdded(uint256 reward); 92: event Staked(address indexed user, uint256 amount); 93: event Withdrawn(address indexed user, uint256 amount); 94: event RewardPaid(address indexed user, uint256 reward);
File: contracts/VoterProxy.sol 39: event VoteSet(bytes32 hash, bool valid);
File: contracts/VeAssetDepositor.sol 33: event FeesUpdated(uint256 lockIncentive); 34: event InitialLockCreated(uint256 veAssetBalanceStaker, uint256 unlockInWeeks); 35: event LockUpdated(uint256 veAssetBalanceStaker, uint256 unlockInWeeks); 36: event Deposited(address indexed user, uint256 amount, bool lock);
File: contracts/VeTokenMinter.sol 24: event Withdraw(address destination, uint256 amount);
File: contracts/BaseRewardPool.sol 81: event RewardAdded(uint256 reward); 82: event Staked(address indexed user, uint256 amount); 83: event Withdrawn(address indexed user, uint256 amount); 84: event RewardPaid(address indexed user, uint256 reward); 87 event RewardUpdated( 88 address indexed user, 89 uint256 reward, 90 uint256 rewardPerTokenStored, 91 uint256 lastUpdateTime 92: ); 93: event Donated(uint256 queuedRewards);
File: contracts/Booster.sol 73: event Deposited(address indexed user, uint256 indexed poolid, uint256 amount); 74: event Withdrawn(address indexed user, uint256 indexed poolid, uint256 amount); 86 event FeesUpdated( 87 uint256 lockFees, 88 uint256 stakerFees, 89 uint256 stakerLockFee, 90 uint256 callerFees, 91 uint256 platform 92: ); 102: event Voted(uint256 indexed voteId, address indexed votingAddress, bool support);
Use alternative variants, e.g. allowlist/denylist instead of whitelist/blacklist
There is 1 instance of this issue:
File: contracts/VE3DRewardPool.sol #1 204: //fees dont apply until whitelist+veVeAsset lock begins so will report
#0 - jetbrain10
2022-06-15T21:12:03Z
high quality.
#1 - GalloDaSballo
2022-07-09T17:55:27Z
Valid Low
Return value from all external calls in the try catch is ignored (the try clause is empty and the call doesn't declare any return value), I think this finding is invalid
Valid NC
Valid Low
Valid Refactoring
Ref
Ref
NC
Ref
Valid NC
NC
##Â 8. Constant redefined elsewhere Valid Refactoring
NC
Valid NC
Valid NC
The linked events have less than 3 parameters (only 1 exception), invalid
Valid NC
#2 - GalloDaSballo
2022-07-09T17:56:09Z
2L, 5R, 7NC
#3 - GalloDaSballo
2022-08-02T21:25:37Z
For Report:
L01 - getAPY() returns the wrong answer during leap years L02 - Missing checks for address(0x0) when assigning values to address state variables
NC01 - safeApprove() is deprecated NC02 - Adding a return statement when the function defines a named return variable, is redundant NC03 - public functions not called by the contract should be declared external instead NC04 - constants should be defined rather than using magic numbers NC05 - Numeric values having to do with time should use time units for readability NC06 - Missing event for critical parameter change NC07 - Use a more recent version of solidity NC08 - Constant redefined elsewhere NC09 - Inconsistent spacing in comments NC10 - Typos NC11 - File is missing NatSpec / Incomplete NC 12 - Large multiples of ten should use scientific notation (e.g. 1e6) rather than decimal literals (e.g. 1000000), for readability NC13 - Avoid the use of sensitive terms
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0x29A, 0xKitsune, 0xNazgul, 0xf15ers, 0xkatana, Cityscape, Dravee, ElKu, FSchmoede, Funen, GalloDaSballo, Hawkeye, Kaiziron, MiloTruck, Randyyy, RoiEvenHaim, Ruhum, SecureZeroX, SmartSek, TerrierLover, TomJ, Tomio, WatchPug, Waze, _Adam, asutorufos, c3phas, catchup, cogitoergosumsw, delfin454000, ellahi, fatherOfBlocks, gzeon, hansfriese, horsefacts, jonatascm, minhquanym, oyc_109, pauliax, reassor, robee, sach1r0, saian, sashik_eth, simon135, z3s
509.1833 USDT - $509.18
| Issue | Instances | |
|---|---|---|
| 1 | Update to state var should only happen once in a function | 1 |
| 2 | Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate | 1 |
| 3 | State variables only set in the constructor should be declared immutable | 11 |
| 4 | Avoid contract existence checks by using solidity version 0.8.10 or later | 124 |
| 5 | State variables should be cached in stack variables rather than re-reading them from storage | 39 |
| 6 | Multiple accesses of a mapping/array should use a local variable cache | 30 |
| 7 | <x> += <y> costs more gas than <x> = <x> + <y> for state variables | 3 |
| 8 | internal functions only called once can be inlined to save gas | 2 |
| 9 | <array>.length should not be looked up in every loop of a for-loop | 7 |
| 10 | ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops | 13 |
| 11 | keccak256() should only need to be called on a specific string literal once | 1 |
| 12 | Using bools for storage incurs overhead | 5 |
| 13 | Using > 0 costs more gas than != 0 when used on a uint in a require() statement | 7 |
| 14 | It costs more gas to initialize variables to zero than to let the default of zero be applied | 14 |
| 15 | ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) | 13 |
| 16 | Splitting require() statements that use && saves gas | 2 |
| 17 | Using private rather than public for constants, saves gas | 9 |
| 18 | Don't compare boolean expressions to boolean literals | 7 |
| 19 | Don't use SafeMath once the solidity version is 0.8.0 or greater | 6 |
| 20 | Duplicated require()/revert() checks should be refactored to a modifier or function | 15 |
| 21 | require() or revert() statements that check input arguments should be at the top of the function | 2 |
| 22 | Empty blocks should be removed or emit something | 3 |
| 23 | Use custom errors rather than revert()/require() strings to save deployment gas | 67 |
| 24 | Functions guaranteed to revert when called by normal users can be marked payable | 7 |
| 25 | Don't use _msgSender() if not supporting EIP-2771 | 2 |
Total: 391 instances over 25 issues
The assignment of totalWeight can be optimized by summing the old and new weights
There is 1 instance of this issue:
File: contracts/VeTokenMinter.sol #1 43 totalWeight -= veAssetWeights[veAssetOperator]; 44 veAssetWeights[veAssetOperator] = newWeight; 45: totalWeight += newWeight;
address mappings can be combined into a single mapping of an address to a struct, where appropriateSaves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.
There is 1 instance of this issue:
File: contracts/BaseRewardPool.sol #1 75 mapping(address => uint256) public userRewardPerTokenPaid; 76 mapping(address => uint256) public rewards; 77: mapping(address => uint256) private _balances;
immutableAvoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).
There are 11 instances of this issue:
File: contracts/VoterProxy.sol 30: string public name; 31: IVoteEscrow.EscrowModle public escrowModle;
File: contracts/VeAssetDepositor.sol 30: uint256 private maxTime;
File: contracts/VeTokenMinter.sol 16: ERC20 public veToken; 18: uint256 public totalCliffs; 19: uint256 public reductionPerCliff;
File: contracts/BaseRewardPool.sol 55: IERC20 public rewardToken; 56: IERC20 public stakingToken; 62: address public operator; 63: address public rewardManager; 65: uint256 public pid;
Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (700 gas), to check for contract existence for external calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value
There are 124 instances of this issue:
File: contracts/VE3DRewardPool.sol 198 uint256 depositFeeRate = IVeAssetDeposit(rewardTokenInfo[_rewardToken].veAssetDeposits) 199: .lockIncentive(); 215: IRewards(extraRewards[i]).stake(msg.sender, _amount); 223: stakingToken.safeTransferFrom(msg.sender, address(this), _amount); 229: uint256 balance = stakingToken.balanceOf(msg.sender); 239: IRewards(extraRewards[i]).stake(_for, _amount); 247: stakingToken.safeTransferFrom(msg.sender, address(this), _amount); 258: IRewards(extraRewards[i]).withdraw(msg.sender, _amount); 263: stakingToken.safeTransfer(msg.sender, _amount); 287: IERC20(_rewardToken).safeApprove(rewardTokenInfo[_rewardToken].veAssetDeposits, 0); 288 IERC20(_rewardToken).safeApprove( 289 rewardTokenInfo[_rewardToken].veAssetDeposits, 290 reward 291: ); 292 IVeAssetDeposit(rewardTokenInfo[_rewardToken].veAssetDeposits).deposit( 293 reward, 294 false 295: ); 297 uint256 ve3TokenBalance = IERC20(rewardTokenInfo[_rewardToken].ve3Token).balanceOf( 298 address(this) 299: ); 301 IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove( 302 rewardTokenInfo[_rewardToken].ve3TokenRewards, 303 0 304: ); 305 IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove( 306 rewardTokenInfo[_rewardToken].ve3TokenRewards, 307 ve3TokenBalance 308: ); 309 IRewards(rewardTokenInfo[_rewardToken].ve3TokenRewards).stakeFor( 310 _account, 311 ve3TokenBalance 312: ); 314 IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeTransfer( 315 _account, 316 ve3TokenBalance 317: ); 327: IRewards(extraRewards[i]).getReward(_account); 337: IERC20(_rewardToken).safeTransferFrom(msg.sender, address(this), _amount);
File: contracts/VoterProxy.sol 70: operator == address(0) || IDeposit(operator).isShutdown() == true, 99: uint256 balance = IERC20(_token).balanceOf(address(this)); 101: IERC20(_token).safeApprove(_gauge, 0); 102: IERC20(_token).safeApprove(_gauge, balance); 103: IGauge(_gauge).deposit(balance); 117: balance = _asset.balanceOf(address(this)); 118: _asset.safeTransfer(msg.sender, balance); 129: uint256 _balance = IERC20(_token).balanceOf(address(this)); 134: IERC20(_token).safeTransfer(msg.sender, _amount); 140: uint256 amount = balanceOfPool(_gauge).add(IERC20(_token).balanceOf(address(this))); 146: IGauge(_gauge).withdraw(_amount); 152: IERC20(veAsset).safeApprove(escrow, 0); 153: IERC20(veAsset).safeApprove(escrow, _value); 154: IVoteEscrow(escrow).create_lock(_value, _unlockTime); 160: IERC20(veAsset).safeApprove(escrow, 0); 161: IERC20(veAsset).safeApprove(escrow, _value); 162: IVoteEscrow(escrow).increase_amount(_value); 168: IVoteEscrow(escrow).increase_unlock_time(_value); 174: IVoteEscrow(escrow).withdraw(); 215: IVoting(gaugeProxy).vote(_tokenVote, _weight); 218: IVoting(gaugeProxy).vote_for_gauge_weights(_tokenVote[i], _weight[i]); 230: try IGauge(_gauge).getReward() {} catch { 241: try ITokenMinter(minter).distribute(_gauge) {} catch { 245: try IGauge(_gauge).claim_rewards() {} catch { 250: _balance = IERC20(veAsset).balanceOf(address(this)); 251: IERC20(veAsset).safeTransfer(operator, _balance); 258: IGauge(_gauge).claim_rewards(); 264: IFeeDistro(_distroContract).claim(); 265: uint256 _balance = IERC20(_token).balanceOf(address(this)); 266: IERC20(_token).safeTransfer(operator, _balance); 271: return IGauge(_gauge).balanceOf(address(this));
File: contracts/VeAssetDepositor.sol 71: uint256 veVeAsset = IERC20(escrow).balanceOf(staker); 77: IStaker(staker).release(); 79: uint256 veAssetBalanceStaker = IERC20(veAsset).balanceOf(staker); 80: IStaker(staker).createLock(veAssetBalanceStaker, unlockAt); 88: uint256 veAssetBalance = IERC20(veAsset).balanceOf(address(this)); 90: IERC20(veAsset).safeTransfer(staker, veAssetBalance); 94: uint256 veAssetBalanceStaker = IERC20(veAsset).balanceOf(staker); 100: IStaker(staker).increaseAmount(veAssetBalanceStaker); 107: IStaker(staker).increaseTime(unlockAt); 136: IERC20(veAsset).safeTransferFrom(msg.sender, staker, _amount); 145: IERC20(veAsset).safeTransferFrom(msg.sender, address(this), _amount); 162: IERC20(minter).safeApprove(_stakeAddress, _amount); 163: IRewards(_stakeAddress).stakeFor(msg.sender, _amount); 174: uint256 veAssetBal = IERC20(veAsset).balanceOf(msg.sender);
File: contracts/VeTokenMinter.sol 72: veToken.safeTransfer(_to, _amount); 78: veToken.safeTransfer(_destination, _amount);
File: contracts/BaseRewardPool.sol 177: IRewards(extraRewards[i]).stake(msg.sender, _amount); 183: stakingToken.safeTransferFrom(msg.sender, address(this), _amount); 190: uint256 balance = stakingToken.balanceOf(msg.sender); 200: IRewards(extraRewards[i]).stake(_for, _amount); 208: stakingToken.safeTransferFrom(msg.sender, address(this), _amount); 219: IRewards(extraRewards[i]).withdraw(msg.sender, amount); 225: stakingToken.safeTransfer(msg.sender, amount); 246: IRewards(extraRewards[i]).withdraw(msg.sender, amount); 253: IDeposit(operator).withdrawTo(pid, amount, msg.sender); 275: rewardToken.safeTransfer(_account, reward); 276: IDeposit(operator).rewardClaimed(pid, _account, reward); 283: IRewards(extraRewards[i]).getReward(_account); 295: IERC20(rewardToken).safeTransferFrom(msg.sender, address(this), _amount);
File: contracts/Booster.sol 199: address _feeToken = IFeeDistro(feeDistro).token(); 202: lockFees = IRewardFactory(rewardFactory).CreateTokenRewards(_feeToken, lockRewards); 268: address token = ITokenFactory(tokenFactory).CreateDepositToken(_lptoken); 270: address newRewardPool = IRewardFactory(rewardFactory).CreateVeAssetRewards(pid, token); 273 address stash = IStashFactory(stashFactory).CreateStash( 274 pid, 275 veAsset, 276 _gauge, 277 staker, 278 _stashVersion 279: ); 299: IStaker(staker).setStashAccess(stash, true); 300: IRewardFactory(rewardFactory).setAccess(stash, true); 313: try IStaker(staker).withdrawAll(pool.lptoken, pool.gauge) {} catch {} 337: try IStaker(staker).withdrawAll(token, gauge) { 356: IERC20(lptoken).safeTransferFrom(msg.sender, staker, _amount); 361: IStaker(staker).deposit(lptoken, gauge); 366: IStash(stash).stashRewards(); 374: IERC20(token).safeApprove(rewardContract, _amount); 375: IRewards(rewardContract).stakeFor(msg.sender, _amount); 388: uint256 balance = IERC20(lptoken).balanceOf(msg.sender); 406: ITokenMinter(token).burn(_from, _amount); 411: IStaker(staker).withdraw(lptoken, gauge, _amount); 418: IStash(stash).stashRewards(); 422: IERC20(lptoken).safeTransfer(_to, _amount); 436: uint256 userBal = IERC20(token).balanceOf(msg.sender); 460: IStaker(staker).setVote(_hash, valid); 470: IStaker(staker).voteGaugeWeight(_gauge, _weight); 479: IStaker(staker).claimRewards(_gauge); 491: IStaker(staker).execute(gauge, uint256(0), data); 503: IStaker(staker).claimVeAsset(gauge); 509: IStash(stash).claimRewards(); 511: IStash(stash).processStash(); 515: uint256 veAssetBal = IERC20(veAsset).balanceOf(address(this)); 530: IERC20(veAsset).safeTransfer(treasury, _platform); 542: IERC20(veAsset).safeTransfer(msg.sender, _callIncentive); 547: IERC20(veAsset).safeTransfer(rewardContract, veAssetBal); 548: IRewards(rewardContract).queueNewRewards(veAssetBal); 552: IERC20(veAsset).safeTransfer(lockRewards, _lockIncentive); 553: IRewards(lockRewards).queueNewRewards(_lockIncentive); 557: IERC20(veAsset).safeTransfer(stakerRewards, _stakerIncentive); 558: IRewards(stakerRewards).queueNewRewards(veAsset, _stakerIncentive); 563: IERC20(veAsset).safeTransfer(stakerLockRewards, _stakerLockIncentive); 564: IRewards(stakerLockRewards).queueNewRewards(veAsset, _stakerLockIncentive); 578: IStaker(staker).claimFees(feeDistro, feeToken); 580: uint256 _balance = IERC20(feeToken).balanceOf(address(this)); 587: IERC20(feeToken).safeTransfer(lockFees, _lockFeesIncentive); 588: IRewards(lockFees).queueNewRewards(_lockFeesIncentive); 591: IERC20(feeToken).safeTransfer(stakerLockRewards, _stakerLockFeesIncentive); 592: IRewards(stakerLockRewards).queueNewRewards(feeToken, _stakerLockFeesIncentive); 607: uint256 _veAssetEarned = _amount.mul(veTokenMinter.veAssetWeights(address(this))).div( 608: veTokenMinter.totalWeight()
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replace each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
There are 39 instances of this issue:
File: contracts/VE3DRewardPool.sol /// @audit rewardTokenInfo[_rewardToken].veAssetDeposits 289: rewardTokenInfo[_rewardToken].veAssetDeposits, /// @audit rewardTokenInfo[_rewardToken].veAssetDeposits 292: IVeAssetDeposit(rewardTokenInfo[_rewardToken].veAssetDeposits).deposit( /// @audit rewardTokenInfo[_rewardToken].ve3Token 301: IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove( /// @audit rewardTokenInfo[_rewardToken].ve3Token 305: IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove( /// @audit rewardTokenInfo[_rewardToken].ve3Token 314: IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeTransfer( /// @audit rewardTokenInfo[_rewardToken].ve3TokenRewards 306: rewardTokenInfo[_rewardToken].ve3TokenRewards, /// @audit rewardTokenInfo[_rewardToken].ve3TokenRewards 309: IRewards(rewardTokenInfo[_rewardToken].ve3TokenRewards).stakeFor( /// @audit rewardTokenInfo[_rewardToken].periodFinish 354: rewardTokenInfo[_rewardToken].periodFinish.sub(duration) /// @audit rewardTokenInfo[_rewardToken].periodFinish 375: uint256 remaining = rewardTokenInfo[_rewardToken].periodFinish.sub(block.timestamp); /// @audit rewardTokenInfo[_rewardToken].rewardRate 376: uint256 leftover = remaining.mul(rewardTokenInfo[_rewardToken].rewardRate);
File: contracts/VoterProxy.sol /// @audit operator 70: operator == address(0) || IDeposit(operator).isShutdown() == true, /// @audit operator 251: IERC20(veAsset).safeTransfer(operator, _balance); /// @audit operator 266: IERC20(_token).safeTransfer(operator, _balance); /// @audit escrowModle 234: escrowModle == IVoteEscrow.EscrowModle.CURVE || /// @audit escrowModle 235: escrowModle == IVoteEscrow.EscrowModle.RIBBON /// @audit escrowModle 240: } else if (escrowModle == IVoteEscrow.EscrowModle.IDLE) { /// @audit escrowModle 244: } else if (escrowModle == IVoteEscrow.EscrowModle.ANGLE) {
File: contracts/VeAssetDepositor.sol /// @audit incentiveVeAsset 118: ITokenMinter(minter).mint(msg.sender, incentiveVeAsset); /// @audit incentiveVeAsset 140: _amount = _amount.add(incentiveVeAsset); /// @audit unlockTime 110: emit LockUpdated(veAssetBalanceStaker, unlockTime);
File: contracts/VeTokenMinter.sol /// @audit totalCliffs 29: reductionPerCliff = maxSupply.div(totalCliffs); /// @audit totalCliffs 61: uint256 reduction = totalCliffs.sub(cliff); /// @audit totalCliffs 63: _amount = _amount.mul(reduction).div(totalCliffs);
File: contracts/BaseRewardPool.sol /// @audit periodFinish 312: uint256 elapsedTime = block.timestamp.sub(periodFinish.sub(duration)); /// @audit periodFinish 332: uint256 remaining = periodFinish.sub(block.timestamp); /// @audit rewardRate 333: uint256 leftover = remaining.mul(rewardRate); /// @audit queuedRewards 297: emit Donated(queuedRewards);
File: contracts/Booster.sol /// @audit platformFee 528: uint256 _platform = veAssetBal.mul(platformFee).div(FEE_DENOMINATOR); /// @audit rewardFactory 300: IRewardFactory(rewardFactory).setAccess(stash, true); /// @audit treasury 530: IERC20(veAsset).safeTransfer(treasury, _platform); /// @audit stakerRewards 558: IRewards(stakerRewards).queueNewRewards(veAsset, _stakerIncentive); /// @audit stakerLockRewards 564: IRewards(stakerLockRewards).queueNewRewards(veAsset, _stakerLockIncentive); /// @audit stakerLockRewards 592: IRewards(stakerLockRewards).queueNewRewards(feeToken, _stakerLockFeesIncentive); /// @audit lockRewards 553: IRewards(lockRewards).queueNewRewards(_lockIncentive); /// @audit lockFees 588: IRewards(lockFees).queueNewRewards(_lockFeesIncentive); /// @audit feeToken 580: uint256 _balance = IERC20(feeToken).balanceOf(address(this)); /// @audit feeToken 587: IERC20(feeToken).safeTransfer(lockFees, _lockFeesIncentive); /// @audit feeToken 591: IERC20(feeToken).safeTransfer(stakerLockRewards, _stakerLockFeesIncentive); /// @audit feeToken 592: IRewards(stakerLockRewards).queueNewRewards(feeToken, _stakerLockFeesIncentive);
The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping's value in a local storage variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations. Caching an array's struct avoids recalculating the array offsets into memory
There are 30 instances of this issue:
File: contracts/VE3DRewardPool.sol /// @audit rewardTokenInfo[_rewardToken] 109: rewardTokenInfo[_rewardToken].ve3TokenRewards = _ve3TokenRewards; /// @audit rewardTokenInfo[_rewardToken] 110: rewardTokenInfo[_rewardToken].ve3Token = _ve3Token; /// @audit rewardTokenInfo[_rewardToken] 178: .sub(rewardTokenInfo[_rewardToken].lastUpdateTime) /// @audit rewardTokenInfo[_rewardToken] 179: .mul(rewardTokenInfo[_rewardToken].rewardRate) /// @audit rewardTokenInfo[_rewardToken] 194: .add(rewardTokenInfo[_rewardToken].rewards[account]); /// @audit rewardTokenInfo[_rewardToken] 287: IERC20(_rewardToken).safeApprove(rewardTokenInfo[_rewardToken].veAssetDeposits, 0); /// @audit rewardTokenInfo[_rewardToken] 289: rewardTokenInfo[_rewardToken].veAssetDeposits, /// @audit rewardTokenInfo[_rewardToken] 292: IVeAssetDeposit(rewardTokenInfo[_rewardToken].veAssetDeposits).deposit( /// @audit rewardTokenInfo[_rewardToken] 297: uint256 ve3TokenBalance = IERC20(rewardTokenInfo[_rewardToken].ve3Token).balanceOf( /// @audit rewardTokenInfo[_rewardToken] 301: IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove( /// @audit rewardTokenInfo[_rewardToken] 302: rewardTokenInfo[_rewardToken].ve3TokenRewards, /// @audit rewardTokenInfo[_rewardToken] 305: IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove( /// @audit rewardTokenInfo[_rewardToken] 306: rewardTokenInfo[_rewardToken].ve3TokenRewards, /// @audit rewardTokenInfo[_rewardToken] 309: IRewards(rewardTokenInfo[_rewardToken].ve3TokenRewards).stakeFor( /// @audit rewardTokenInfo[_rewardToken] 314: IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeTransfer( /// @audit rewardTokenInfo[_rewardToken] 346: if (block.timestamp >= rewardTokenInfo[_rewardToken].periodFinish) { /// @audit rewardTokenInfo[_rewardToken] 348: rewardTokenInfo[_rewardToken].queuedRewards = 0; /// @audit rewardTokenInfo[_rewardToken] 354: rewardTokenInfo[_rewardToken].periodFinish.sub(duration) /// @audit rewardTokenInfo[_rewardToken] 357: uint256 currentAtNow = rewardTokenInfo[_rewardToken].rewardRate * elapsedTime; /// @audit rewardTokenInfo[_rewardToken] 361: rewardTokenInfo[_rewardToken].queuedRewards = 0; /// @audit rewardTokenInfo[_rewardToken] 363: rewardTokenInfo[_rewardToken].queuedRewards = _rewards; /// @audit rewardTokenInfo[_rewardToken] 372: if (block.timestamp >= rewardTokenInfo[_rewardToken].periodFinish) { /// @audit rewardTokenInfo[_rewardToken] 373: rewardTokenInfo[_rewardToken].rewardRate = reward.div(duration); /// @audit rewardTokenInfo[_rewardToken] 375: uint256 remaining = rewardTokenInfo[_rewardToken].periodFinish.sub(block.timestamp); /// @audit rewardTokenInfo[_rewardToken] 376: uint256 leftover = remaining.mul(rewardTokenInfo[_rewardToken].rewardRate); /// @audit rewardTokenInfo[_rewardToken] 378: rewardTokenInfo[_rewardToken].rewardRate = reward.div(duration); /// @audit rewardTokenInfo[_rewardToken] 380: rewardTokenInfo[_rewardToken].currentRewards = reward; /// @audit rewardTokenInfo[_rewardToken] 381: rewardTokenInfo[_rewardToken].lastUpdateTime = block.timestamp; /// @audit rewardTokenInfo[_rewardToken] 382: rewardTokenInfo[_rewardToken].periodFinish = block.timestamp.add(duration);
File: contracts/Booster.sol /// @audit poolInfo[_pid] 486: address gauge = poolInfo[_pid].gauge;
<x> += <y> costs more gas than <x> = <x> + <y> for state variablesThere are 3 instances of this issue:
File: contracts/VeTokenMinter.sol #1 43: totalWeight -= veAssetWeights[veAssetOperator];
File: contracts/VeTokenMinter.sol #2 45: totalWeight += newWeight;
File: contracts/VeTokenMinter.sol #3 73: totalSupply += _amount;
internal functions only called once can be inlined to save gasNot inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
There are 2 instances of this issue:
File: contracts/VoterProxy.sol #1 145: function _withdrawSome(address _gauge, uint256 _amount) internal returns (uint256) {
File: contracts/Booster.sol #2 496: function _earmarkRewards(uint256 _pid) internal {
<array>.length should not be looked up in every loop of a for-loopThe overheads outlined below are PER LOOP, excluding the first loop
MLOAD (3 gas)CALLDATALOAD (3 gas)Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
There are 7 instances of this issue:
File: contracts/VoterProxy.sol 217: for (uint256 i = 0; i < _tokenVote.length; i++) {
File: contracts/BaseRewardPool.sol 176: for (uint256 i = 0; i < extraRewards.length; i++) { 199: for (uint256 i = 0; i < extraRewards.length; i++) { 218: for (uint256 i = 0; i < extraRewards.length; i++) { 245: for (uint256 i = 0; i < extraRewards.length; i++) { 282: for (uint256 i = 0; i < extraRewards.length; i++) {
File: contracts/Booster.sol 329: for (uint256 i = 0; i < poolInfo.length; i++) {
++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loopsThe unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop
There are 13 instances of this issue:
File: contracts/VE3DRewardPool.sol 148: for (uint256 i = 0; i < rewardTokens.length(); i++) { 214: for (uint256 i = 0; i < length; i++) { 238: for (uint256 i = 0; i < length; i++) { 257: for (uint256 i = 0; i < length; i++) { 281: for (uint256 i = 0; i < rewardTokens.length(); i++) { 326: for (uint256 i = 0; i < length; i++) {
File: contracts/VoterProxy.sol 217: for (uint256 i = 0; i < _tokenVote.length; i++) {
File: contracts/BaseRewardPool.sol 176: for (uint256 i = 0; i < extraRewards.length; i++) { 199: for (uint256 i = 0; i < extraRewards.length; i++) { 218: for (uint256 i = 0; i < extraRewards.length; i++) { 245: for (uint256 i = 0; i < extraRewards.length; i++) { 282: for (uint256 i = 0; i < extraRewards.length; i++) {
File: contracts/Booster.sol 329: for (uint256 i = 0; i < poolInfo.length; i++) {
keccak256() should only need to be called on a specific string literal onceIt should be saved to an immutable variable, and the variable used instead. If the hash is being used as a part of a function selector, the cast to bytes4 should also only be done once
There is 1 instance of this issue:
File: contracts/Booster.sol #1 488: bytes4(keccak256("set_rewards_receiver(address)")),
bools 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), and to avoid Gsset (20000 gas) when changing from 'false' to 'true', after having been 'true' in the past
There are 5 instances of this issue:
File: contracts/VoterProxy.sol 33: mapping(address => bool) private protectedTokens; 34: mapping(address => bool) private stashPool; 35: mapping(bytes32 => bool) private votes;
File: contracts/Booster.sol 58: bool public isShutdown; 71: mapping(address => bool) public gaugeMap;
> 0 costs more gas than != 0 when used on a uint in a require() statementThis change saves 6 gas per instance
There are 7 instances of this issue:
File: contracts/VE3DRewardPool.sol 210: require(_amount > 0, "RewardPool : Cannot stake 0"); 234: require(_amount > 0, "RewardPool : Cannot stake 0"); 253: require(_amount > 0, "RewardPool : Cannot withdraw 0");
File: contracts/VeAssetDepositor.sol 132: require(_amount > 0, "!>0");
File: contracts/BaseRewardPool.sol 173: require(_amount > 0, "RewardPool : Cannot stake 0"); 196: require(_amount > 0, "RewardPool : Cannot stake 0"); 215: require(amount > 0, "RewardPool : Cannot withdraw 0");
There are 14 instances of this issue:
File: contracts/VE3DRewardPool.sol 148: for (uint256 i = 0; i < rewardTokens.length(); i++) { 214: for (uint256 i = 0; i < length; i++) { 238: for (uint256 i = 0; i < length; i++) { 257: for (uint256 i = 0; i < length; i++) { 281: for (uint256 i = 0; i < rewardTokens.length(); i++) { 326: for (uint256 i = 0; i < length; i++) {
File: contracts/VoterProxy.sol 217: for (uint256 i = 0; i < _tokenVote.length; i++) { 227: uint256 _balance = 0;
File: contracts/BaseRewardPool.sol 176: for (uint256 i = 0; i < extraRewards.length; i++) { 199: for (uint256 i = 0; i < extraRewards.length; i++) { 218: for (uint256 i = 0; i < extraRewards.length; i++) { 245: for (uint256 i = 0; i < extraRewards.length; i++) { 282: for (uint256 i = 0; i < extraRewards.length; i++) {
File: contracts/Booster.sol 329: for (uint256 i = 0; i < poolInfo.length; i++) {
++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)Saves 6 gas per loop
There are 13 instances of this issue:
File: contracts/VE3DRewardPool.sol 148: for (uint256 i = 0; i < rewardTokens.length(); i++) { 214: for (uint256 i = 0; i < length; i++) { 238: for (uint256 i = 0; i < length; i++) { 257: for (uint256 i = 0; i < length; i++) { 281: for (uint256 i = 0; i < rewardTokens.length(); i++) { 326: for (uint256 i = 0; i < length; i++) {
File: contracts/VoterProxy.sol 217: for (uint256 i = 0; i < _tokenVote.length; i++) {
File: contracts/BaseRewardPool.sol 176: for (uint256 i = 0; i < extraRewards.length; i++) { 199: for (uint256 i = 0; i < extraRewards.length; i++) { 218: for (uint256 i = 0; i < extraRewards.length; i++) { 245: for (uint256 i = 0; i < extraRewards.length; i++) { 282: for (uint256 i = 0; i < extraRewards.length; i++) {
File: contracts/Booster.sol 329: for (uint256 i = 0; i < poolInfo.length; i++) {
require() statements that use && saves gasSee 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
There are 2 instances of this issue:
File: contracts/Booster.sol #1 261: require(msg.sender == poolManager && !isShutdown, "!add");
File: contracts/Booster.sol #2 262: require(_gauge != address(0) && _lptoken != address(0), "!param");
private rather than public for constants, saves gasIf needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table
There are 9 instances of this issue:
File: contracts/VE3DRewardPool.sol 59: uint256 public constant duration = 7 days; 60: uint256 public constant FEE_DENOMINATOR = 10000; 64: uint256 public constant newRewardRatio = 830;
File: contracts/VeAssetDepositor.sol 21: uint256 public constant FEE_DENOMINATOR = 10000;
File: contracts/VeTokenMinter.sol 15: uint256 public constant maxSupply = 30 * 1000000 * 1e18; //30mil
File: contracts/BaseRewardPool.sol 57: uint256 public constant duration = 7 days; 73: uint256 public constant newRewardRatio = 830;
File: contracts/Booster.sol 33: uint256 public constant MaxFees = 2000; 34: uint256 public constant FEE_DENOMINATOR = 10000;
if (<x> == true) => if (<x>), if (<x> == false) => if (!<x>)
There are 7 instances of this issue:
File: contracts/VoterProxy.sol 70: operator == address(0) || IDeposit(operator).isShutdown() == true, 93: if (protectedTokens[_token] == false) { 96: if (protectedTokens[_gauge] == false) { 110: require(stashPool[msg.sender] == true, "!auth"); 113: if (protectedTokens[address(_asset)] == true) {
File: contracts/Booster.sol 352: require(pool.shutdown == false, "pool is closed"); 498: require(pool.shutdown == false, "pool is closed");
SafeMath once the solidity version is 0.8.0 or greaterVersion 0.8.0 introduces internal overflow checks, so using SafeMath is redundant and adds overhead
There are 6 instances of this issue:
File: contracts/VE3DRewardPool.sol 42: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
File: contracts/VoterProxy.sol 4: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
File: contracts/VeAssetDepositor.sol 4: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
File: contracts/VeTokenMinter.sol 8: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
File: contracts/BaseRewardPool.sol 42: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
File: contracts/Booster.sol 4: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
require()/revert() checks should be refactored to a modifier or functionSaves deployment costs
There are 15 instances of this issue:
File: contracts/VE3DRewardPool.sol 142: require(msg.sender == rewardManager, "!authorized"); 234: require(_amount > 0, "RewardPool : Cannot stake 0");
File: contracts/VoterProxy.sol 68: require(msg.sender == owner, "!auth"); 92: require(msg.sender == operator, "!auth"); 159: require(msg.sender == depositor, "!auth");
File: contracts/VeAssetDepositor.sol 60: require(msg.sender == feeManager, "!auth");
File: contracts/BaseRewardPool.sol 132: require(msg.sender == rewardManager, "!authorized"); 196: require(_amount > 0, "RewardPool : Cannot stake 0");
File: contracts/Booster.sol 146: require(msg.sender == owner, "!auth"); 194: require(msg.sender == feeManager, "!auth"); 309: require(msg.sender == poolManager, "!auth"); 458: require(msg.sender == voteDelegate, "!auth"); 570: require(!isShutdown, "shutdown"); 498: require(pool.shutdown == false, "pool is closed"); 485: require(msg.sender == stash, "!auth");
require() or revert() statements that check input arguments should be at the top of the functionChecks that involve constants should come before checks that involve state variables
There are 2 instances of this issue:
File: contracts/VE3DRewardPool.sol #1 136: require(_reward != address(0), "!reward setting");
File: contracts/BaseRewardPool.sol #2 123: require(_reward != address(0), "!reward setting");
The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}})
There are 3 instances of this issue:
File: contracts/Booster.sol #1 313: try IStaker(staker).withdrawAll(pool.lptoken, pool.gauge) {} catch {}
File: contracts/Booster.sol #2 313: try IStaker(staker).withdrawAll(pool.lptoken, pool.gauge) {} catch {}
File: contracts/Booster.sol #3 339: } catch {}
revert()/require() strings to save deployment gasCustom errors are available from solidity version 0.8.4. The instances below match or exceed that version
There are 67 instances of this issue:
File: contracts/VE3DRewardPool.sol 135: require(msg.sender == rewardManager, "!authorized"); 136: require(_reward != address(0), "!reward setting"); 142: require(msg.sender == rewardManager, "!authorized"); 210: require(_amount > 0, "RewardPool : Cannot stake 0"); 234: require(_amount > 0, "RewardPool : Cannot stake 0"); 253: require(_amount > 0, "RewardPool : Cannot withdraw 0"); 342: require(operators.contains(_msgSender()), "!authorized");
File: contracts/VoterProxy.sol 63: require(msg.sender == owner, "!auth"); 68: require(msg.sender == owner, "!auth"); 69 require( 70 operator == address(0) || IDeposit(operator).isShutdown() == true, 71 "needs shutdown" 72: ); 78: require(msg.sender == owner, "!auth"); 84: require(msg.sender == operator, "!auth"); 92: require(msg.sender == operator, "!auth"); 110: require(stashPool[msg.sender] == true, "!auth"); 128: require(msg.sender == operator, "!auth"); 139: require(msg.sender == operator, "!auth"); 151: require(msg.sender == depositor, "!auth"); 159: require(msg.sender == depositor, "!auth"); 167: require(msg.sender == depositor, "!auth"); 173: require(msg.sender == depositor, "!auth"); 186: require(msg.sender == operator, "!auth"); 211: require(msg.sender == operator, "!auth"); 225: require(msg.sender == operator, "!auth"); 257: require(msg.sender == operator, "!auth"); 263: require(msg.sender == operator, "!auth"); 279: require(msg.sender == operator, "!auth"); 282: require(success, "!success");
File: contracts/VeAssetDepositor.sol 54: require(msg.sender == feeManager, "!auth"); 60: require(msg.sender == feeManager, "!auth"); 69: require(msg.sender == feeManager, "!auth"); 132: require(_amount > 0, "!>0");
File: contracts/VeTokenMinter.sol 42: require(operators.contains(veAssetOperator), "not an veAsset operator"); 49: require(operators.contains(_msgSender()), "not an operator");
File: contracts/BaseRewardPool.sol 122: require(msg.sender == rewardManager, "!authorized"); 123: require(_reward != address(0), "!reward setting"); 124: require(extraRewards.length < EXTRA_REWARD_POOLS, "!extra reward pools exceed"); 132: require(msg.sender == rewardManager, "!authorized"); 173: require(_amount > 0, "RewardPool : Cannot stake 0"); 196: require(_amount > 0, "RewardPool : Cannot stake 0"); 215: require(amount > 0, "RewardPool : Cannot withdraw 0"); 301: require(msg.sender == operator, "!authorized");
File: contracts/Booster.sol 124: require(msg.sender == owner, "!auth"); 130: require(msg.sender == feeManager, "!auth"); 136: require(msg.sender == poolManager, "!auth"); 146: require(msg.sender == owner, "!auth"); 163: require(msg.sender == owner, "!auth"); 169: require(msg.sender == voteDelegate, "!auth"); 179: require(msg.sender == owner, "!auth"); 194: require(msg.sender == feeManager, "!auth"); 226: require(msg.sender == feeManager, "!auth"); 231: require(total <= MaxFees, ">MaxFees"); 244: require(msg.sender == feeManager, "!auth"); 261: require(msg.sender == poolManager && !isShutdown, "!add"); 262: require(_gauge != address(0) && _lptoken != address(0), "!param"); 309: require(msg.sender == poolManager, "!auth"); 326: require(msg.sender == owner, "!auth"); 350: require(!isShutdown, "shutdown"); 352: require(pool.shutdown == false, "pool is closed"); 360: require(gauge != address(0), "!gauge setting"); 448: require(msg.sender == rewardContract, "!auth"); 458: require(msg.sender == voteDelegate, "!auth"); 468: require(msg.sender == voteDelegate, "!auth"); 477: require(msg.sender == stash, "!auth"); 485: require(msg.sender == stash, "!auth"); 498: require(pool.shutdown == false, "pool is closed"); 570: require(!isShutdown, "shutdown"); 604: require(msg.sender == rewardContract || msg.sender == lockRewards, "!auth");
payableIf a function modifier such as onlyOwner 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
There are 7 instances of this issue:
File: contracts/VE3DRewardPool.sol 102 function addReward( 103 address _rewardToken, 104 address _veAssetDeposits, 105 address _ve3TokenRewards, 106 address _ve3Token 107: ) external onlyOwner { 114: function addOperator(address _newOperator) public onlyOwner { 118: function removeOperator(address _operator) public onlyOwner {
File: contracts/VeTokenMinter.sol 32: function addOperator(address _newOperator) public onlyOwner { 36: function removeOperator(address _operator) public onlyOwner { 41: function updateveAssetWeight(address veAssetOperator, uint256 newWeight) external onlyOwner { 77: function withdraw(address _destination, uint256 _amount) external onlyOwner {
_msgSender() if not supporting EIP-2771Use msg.sender if the code does not implement EIP-2771 trusted forwarder support
There are 2 instances of this issue:
File: contracts/VE3DRewardPool.sol #1 342: require(operators.contains(_msgSender()), "!authorized");
File: contracts/VeTokenMinter.sol #2 49: require(operators.contains(_msgSender()), "not an operator");
#0 - GalloDaSballo
2022-07-14T02:11:58Z
Immutables would save 23100 already
#1 - GalloDaSballo
2022-07-28T20:22:05Z
Huge 5k gas finding
Please provide an example of how the Struct would save gas as while the advice is valid, without an example and benchmarks we cannot verify your statement (you can use Struct, but if values are never used together it won't save gas)
11 * 2100 23000
I had a very hard time finding any proof that the finding is valid
So I wrote a POC with Gas-Lab, Optimizer on, 200 runs <img width="635" alt="Screenshot 2022-07-29 at 01 03 49" src="https://user-images.githubusercontent.com/13383782/181651456-1f41ec88-0882-440f-ba73-e652fe251c37.png"> <img width="655" alt="Screenshot 2022-07-29 at 01 04 14" src="https://user-images.githubusercontent.com/13383782/181651505-871b3600-8f5f-44dd-9d2f-51f29ad65dce.png">
0.8.7 is indeed more expensive (134 gas).
This is in line with Evm.codes that says EXTCODESIZE costs 100 gas
Will mark as valid and give 100 gas per instance
124 * 100 12400
Let's say 39 * 94 (94 is the gas saved for the second SLOAD, consecutive ones will save 97) 3666
Let's say 30 per instance 30 * 30 900
I wasn't able to verify this statement under multiple solidity versions, please provide a POC of this refactoring saving gas next time
16 * 2 + 3 (MLOAD for computation) 35
#2 - GalloDaSballo
2022-07-28T23:06:19Z
6 / 7 are SLOADS saved 97 per instance
One is calldata, 3 gas
97 * 6 + 3 585
Because you provided 30 gas benchmark from hrkshn let's say 30 per instance 30 * 13 390
30 gas would be saved if using a constant
100 * 5 500
I think using 1 and 2 is a great idea for saving gas on governance operations also
6 * 7
42
3 * 14 42
Already included in 10.
3 per instance 2 * 3 6
It will but it will also change functionality
3 per instance 7 * 3 21
Would have liked to see a benchmark of gas saved
Will reduce bytecode at cost of extra jump
Valid but hard to quantify
Don't think you can catch without curlys
Valid but hard to quantify
Valid but doesn't save gas for end-users
Saves the jump, 8 * 2
#3 - GalloDaSballo
2022-07-28T23:07:34Z
Most thorough submission, would like for the warden to add:
Overall best report for the contest
#4 - GalloDaSballo
2022-07-28T23:08:49Z
Total Gas Saved 46633