Platform: Code4rena
Start Date: 21/04/2022
Pot Size: $100,000 USDC
Total HM: 18
Participants: 60
Period: 7 days
Judge: gzeon
Total Solo HM: 10
Id: 112
League: ETH
Rank: 14/60
Findings: 2
Award: $1,032.54
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: IllIllI
Also found by: 0v3rf10w, 0x52, 0xDjango, 0xkatana, Dravee, Funen, Kenshin, Ruhum, StyxRave, Tadashi, TerrierLover, TrungOre, antonttc, berndartmueller, catchup, csanuragjain, defsec, dipp, fatherOfBlocks, hake, horsefacts, hubble, jayjonah8, joestakey, kebabsec, kenta, m4rio_eth, oyc_109, pauliax, peritoflores, rayn, remora, robee, securerodd, simon135, sorrynotsorry, sseefried, z3s
204.213 USDC - $204.21
Few vulnerabilities were found examining the contracts. The main concerns are with unchecked inputs in setter functions.
There are a few typos in some comments
Non-Critical
Instances include:
CompoundHandler.sol:90: bytes memory//no parameter is declared
Preparable.sol:148: _setConfig is declared twice
Manual Analysis
Correct the typos.
Some of the function comments are missing function parameters or returns
Non-Critical
Instances include:
AaveHandler.sol:26: topUp() //bytes memory extra
CompoundHandler.sol:26: topUp() //bytes memory extra
CompoundHandler.sol:109: topUp() //CToken ctoken
TopUpAction.sol:203: register() //return value missing comment TopUpAction.sol:442: addUsableToken() //return value missing comment
Manual Analysis
Add a comment for these parameters
Some functions are missing comments.
Non-Critical
Instances include:
AaveHandler.sol:69: getUserFactor()
CompoundHandler.sol:128: _getAccountBorrowsAndSupply()
CTokenRegistry.sol:128: _isCTokenUsable()
TopUpAction.sol:36: lockFunds() TopUpAction.sol:91: getSwapper() TopUpAction.sol:180: initialize() TopUpAction.sol:821: _updateTopUpHandler() TopUpAction.sol:871: _removePosition() TopUpAction.sol:884: _removeUserPosition()
Manual Analysis
Add comments to all functions
When there are mappings that use the same key value, having separate fields is error prone, for instance in case of deletion or with future new fields.
Non-Critical
Instances include:
BkdLocker.sol:26:mapping(address => uint256) public balances; BkdLocker.sol:27:mapping(address => uint256) public boostFactors; BkdLocker.sol:28:mapping(address => uint256) public lastUpdated; BkdLocker.sol:29:mapping(address => WithdrawStash[]) public stashedGovTokens; BkdLocker.sol:30:mapping(address => uint256) public totalStashed;
Manual Analysis
Group the related data in a struct and use one mapping. For instance, the mitigation could be:
struct UserData { uint256 balances; uint256 boostFactors; uint256 lastUpdated; WithdrawStash[] stashedGovTokens; uint256 totalStashed; }
And it would be used as a state variable:
mapping(address => UserData) userData;
Setters should emit an event so that Dapps can detect important changes to storage
Non-Critical
Instances include:
ChainlinkOracleProvider.sol:40: setStalePriceDelay()
Controller.sol:31: setInflationManager()
CvxCrvRewardsLocker.sol:119: setWithdrawalFlag()
Manual Analysis
Emit an event in all setters.
There should be a non-zero (address or integer) check in all setters
Non-Critical
Instances include:
ConvexStrategyBase.sol:181: setCommunityReserve() ConvexStrategyBase.sol:227: setImbalanceToleranceIn() ConvexStrategyBase.sol:243: setImbalanceToleranceOut()
CvxCrvRewardsLocker.sol:150: setTreasury() CvxCrvRewardsLocker.sol:189: setDelegate()
Manual Analysis
Add non-zero checks.
🌟 Selected for report: joestakey
Also found by: 0v3rf10w, 0x1f8b, 0x4non, 0xDjango, 0xNazgul, 0xkatana, 0xmint, Dravee, Funen, IllIllI, MaratCerby, NoamYakov, Tadashi, TerrierLover, Tomio, WatchPug, catchup, defsec, fatherOfBlocks, hake, horsefacts, kenta, oyc_109, pauliax, rayn, rfa, robee, saian, securerodd, simon135, slywaters, sorrynotsorry, tin537, z3s
828.3304 USDC - $828.33
Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.
In particular, in for loops, when using the length of a storage array as the condition being checked after each loop, caching the array length in memory can yield significant gas savings if the array length is high.
Instances include:
scope: topUp()
weth is read twiceAaveHandler.sol:44 AaveHandler.sol:45
lendingPool is read 4 timesAaveHandler.sol:50 AaveHandler.sol:53 AaveHandler.sol:60 AaveHandler.sol:65
scope: _getAccountBorrowsAndSupply()
comptroller is read (2 + assets.length) times. Number of read depends on the length of assets as it is in a for loopCompoundHandler.sol:132 CompoundHandler.sol:134 CompoundHandler.sol:142
scope: _isCTokenUsable()
comptroller is read 3 timesCTokenRegistry.sol:77 CTokenRegistry.sol:79 CTokenRegistry.sol:80
scope: resetPosition()
addressProvider is read twiceTopUpAction.sol:284 TopUpAction.sol:295
scope: execute()
addressProvider is read 3 timesTopUpAction.sol:562 TopUpAction.sol:604 TopUpAction.sol:632
scope: _withdraw()
vault is read twiceBkdEthCvx.sol:77 BkdEthCvx.sol:93
scope: _withdraw()
vault is read twiceBkdTriHopCvx.sol:175 BkdTriHopCvx.sol:201
scope: addRewardToken()
_strategySwapper is read twiceConvexStrategyBase.sol:279 ConvexStrategyBase.sol:280
scope: harvestable()
crvCommunityReserveShare is read twiceConvexStrategyBase.sol:307 ConvexStrategyBase.sol:311
_rewardTokens.length() is read _rewardTokens.length() times. Number of read depends on the length of _rewardsTokens as it is in a for loopConvexStrategyBase.sol:313
scope: _harvest()
_rewardTokens.length() is read _rewardTokens.length() times. Number of read depends on the length of _rewardsTokens as it is in a for loopConvexStrategyBase.sol:380
scope: _sendCommunityReserveShare()
cvxCommunityReserveShare is read twiceConvexStrategyBase.sol:398 ConvexStrategyBase.sol:409
scope: _handleExcessDebt()
reserve is read 3 timesVault.sol:645 Vault.sol:648 Vault.sol:649
scope: _handleExcessDebt()
totalDebt is read twiceVault.sol:657 Vault.sol:658
scope: stakeFor()
token is read 4 timesVault.sol:324 Vault.sol:331 Vault.sol:338 Vault.sol:339
scope: unStakeFor()
token is read 4 timesVault.sol:365 Vault.sol:376 Vault.sol:382 Vault.sol:384
Manual Analysis
cache these storage variables in memory
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.
Try to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.
Instances include:
scope: hasAnyRole()
RoleManager.sol:73: bytes32[] memory roles
scope: topUp()
AaveHandler.sol:40: bytes memory extra
scope: topUp()
CompoundHandler.sol:54: bytes memory extra
scope: getHealthFactor()
TopUpAction.sol:760: bytes memory extra
scope: canExecute()
TopUpKeeperHelper.sol:108: ITopUpAction.RecordKey memory key
scope: _canExecute()
TopUpKeeperHelper.sol:131: ITopUpAction.RecordWithMeta memory position
scope: _positionToTopup()
TopUpKeeperHelper.sol:145: ITopUpAction.RecordWithMeta memory position
scope: _shortenTopups()
TopUpKeeperHelper.sol:159: TopupData[] memory topups
scope: initialize()
Erc20Pool.sol:15: string memory name_
scope: initialize()
EthPool.sol:13: string memory name_
scope: initialize()
LiquidityPool.sol:702: string memory name_
scope: initialize()
LpToken.sol:29: string memory name_ LpToken.sol:30: string memory symbol_
Manual Analysis
Replace memory with calldata
>0 is less gas efficient than !0 if you enable the optimizer at 10k AND you’re in a require statement.
Detailed explanation with the opcodes here
Instances include:
scope: register()
TopUpAction.sol:210
scope: execute()
TopUpAction.sol:554
scope: claimKeeperFeesForPool()
TopUpActionFeeHandler.sol:123
scope: updateDepositCap()
LiquidityPool.sol:401
scope: calcRedeem()
LiquidityPool.sol:471 LiquidityPool.sol:473
scope: redeem()
LiquidityPool.sol:549
scope: withdrawFromReserve()
Vault.sol:164
scope: depositFees()
BkdLocker.sol:90 BkdLocker.sol:91 BkdLocker.sol:136
Manual Analysis
Replace > 0 with !0
In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.
Using strict comparison operators hence saves gas
Instances include:
TopUpAction.sol:61 TopUpAction.sol:212 TopUpAction.sol:224 TopUpAction.sol:328 TopUpAction.sol:360 TopUpAction.sol:361 TopUpAction.sol:500 TopUpAction.sol:501 TopUpAction.sol:576 TopUpAction.sol:584 TopUpAction.sol:724
TopUpActionFeeHandler.sol:54 TopUpActionFeeHandler.sol:151 TopUpActionFeeHandler.sol:163 TopUpActionFeeHandler.sol:196 TopUpActionFeeHandler.sol:208
ChainLinkOracleProvider.sol:41 ChainLinkOracleProvider.sol:57
EthPool.sol:442 EthPool.sol:208 EthPool.sol:518 EthPool.sol:525 EthPool.sol:551 EthPool.sol:562 EthPool.sol:690 EthPool.sol:811 EthPool.sol:812
BkdEthCvx.sol:76
BkdTriHopCvx.sol:174
ConvexStrategyBase.sol:197 ConvexStrategyBase.sol:214
StrategySwapper.sol:110
CvxMintAmount.sol:21
Preparable.sol:29 Preparable.sol:110
Vault.sol:88 Vault.sol:89 Vault.sol:90 Vault.sol:167 Vault.sol:264 Vault.sol:323 Vault.sol:392 Vault.sol:437 Vault.sol:482 Vault.sol:712 Vault.sol:763
VaultReserve.sol:59 VaultReserve.sol:75 VaultReserve.sol:103
BkdLocker.sol:119 BkdLocker.sol:140 BkdLocker.sol:281
Controller:98
CvxCrvRewardsLocker:84
GasBank:68 GasBank:76
StakerVault:107 StakerVault:150 StakerVault:153 StakerVault:324 StakerVault:368 StakerVault:371
Manual Analysis
Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable
example:
-require(maxFee >= minFee, Error.INVALID_AMOUNT); +require(maxFee > minFee - 1, Error.INVALID_AMOUNT);
When the comparison is with a constant storage variable, you can also do the increment in the storage variable declaration
example:
-require(maxFee <= _MAX_WITHDRAWAL_FEE, Error.INVALID_AMOUNT) +require(maxFee < _MAX_WITHDRAWAL_FEE_PLUS_ONE, Error.INVALID_AMOUNT)
However, when 1 is negligible compared to the variable (with is the case here as the variable is in the order of 10**16), it is not necessary to increment.
Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here
Custom errors are defined using the error statement
Instances include:
RoleManager.sol:44 RoleManager.sol:110 RoleManager.sol:111
AaveHandler.sol:51
CompoundHandler.sol:74 CompoundHandler.sol:80 CompoundHandler.sol:123 CompoundHandler.sol:141 CompoundHandler.sol:148
TopUpAction.sol:67 TopUpAction.sol:98 TopUpAction.sol:185 TopUpAction.sol:209 TopUpAction.sol:210 TopUpAction.sol:211 TopUpAction.sol:212 TopUpAction.sol:213 TopUpAction.sol:217 TopUpAction.sol:218 TopUpAction.sol:224 TopUpAction.sol:282 TopUpAction.sol:328 TopUpAction.sol:359 TopUpAction.sol:546 TopUpAction.sol:553 TopUpAction.sol:554 TopUpAction.sol:560 TopUpAction.sol:575 TopUpAction.sol:583 TopUpAction.sol:676 TopUpAction.sol:723 TopUpAction.sol:928
TopUpActionFeeHandler.sol:67 TopUpActionFeeHandler.sol:68 TopUpActionFeeHandler.sol:87 TopUpActionFeeHandler.sol:123 TopUpActionFeeHandler.sol:151 TopUpActionFeeHandler.sol:161 TopUpActionFeeHandler.sol:196 TopUpActionFeeHandler.sol:206
ChainLinkOracleProvider.sol:31 ChainLinkOracleProvider.sol:41 ChainLinkOracleProvider.sol:53 ChainLinkOracleProvider.sol:57 ChainLinkOracleProvider.sol:58
Erc20Pool.sol:20 Erc20Pool.sol:30
EthPool.sol:25 EthPool.sol:26
LiquidityPool.sol:76 LiquidityPool.sol:136 LiquidityPool.sol:137 LiquidityPool.sol:155 LiquidityPool.sol:179 LiquidityPool.sol:208 LiquidityPool.sol:331 LiquidityPool.sol:333 LiquidityPool.sol:387 LiquidityPool.sol:399 LiquidityPool.sol:400 LiquidityPool.sol:401 LiquidityPool.sol:441 LiquidityPool.sol:471 LiquidityPool.sol:473 LiquidityPool.sol:517 LiquidityPool.sol:525 LiquidityPool.sol:549 LiquidityPool.sol:551 LiquidityPool.sol:562 LiquidityPool.sol:811 LiquidityPool.sol:812
PoolFactory.sol:159 PoolFactory.sol:162 PoolFactory.sol:165 PoolFactory.sol:170 PoolFactory.sol:180 PoolFactory.sol:184
BkdTriHopCvx.sol:133 BkdTriHopCvx.sol:147
ConvexStrategyBase.sol:117 ConvexStrategyBase.sol:144 ConvexStrategyBase.sol:197 ConvexStrategyBase.sol:198 ConvexStrategyBase.sol:214 ConvexStrategyBase.sol:215 ConvexStrategyBase.sol:260 ConvexStrategyBase.sol:273
StrategySwapper.sol:69 StrategySwapper.sol:110 StrategySwapper.sol:111 StrategySwapper.sol:123 StrategySwapper.sol:124 StrategySwapper.sol:139
Preparable.sol:28 Preparable.sol:29 Preparable.sol:86 Preparable.sol:98 Preparable.sol:110 Preparable.sol:111
Erc20Vault.sol:20
Vault.sol:88 Vault.sol:89 Vault.sol:90 Vault.sol:164 Vault.sol:165 Vault.sol:167 Vault.sol:194 Vault.sol:195 Vault.sol:198 Vault.sol:264 Vault.sol:392 Vault.sol:429 Vault.sol:762
VaultReserve.sol:51 VaultReserve.sol:59 VaultReserve.sol:73 VaultReserve.sol:75
AddressProvider.sol:64 AddressProvider.sol:70 AddressProvider.sol:96 AddressProvider.sol:100 AddressProvider.sol:170 AddressProvider.sol:179 AddressProvider.sol:188 AddressProvider.sol:230 AddressProvider.sol:231 AddressProvider.sol:249 AddressProvider.sol:259 AddressProvider.sol:284 AddressProvider.sol:285 AddressProvider.sol:314 AddressProvider.sol:417 AddressProvider.sol:423
BkdLocker.sol:58 BkdLocker.sol:90 BkdLocker.sol:91 BkdLocker.sol:118 BkdLocker.sol:136 BkdLocker.sol:207
Controller:32 Controller:33 Controller:80
CvxCrvRewardsLocker:83 CvxCrvRewardsLocker:135
GasBank:42 GasBank:68 GasBank:69 GasBank:76 GasBank:91
LpToken:34
StakerVault:70 StakerVault:93 StakerVault:106 StakerVault:107 StakerVault:139 StakerVault:150 StakerVault:153 StakerVault:203 StakerVault:224 StakerVault:324 StakerVault:340 StakerVault:367 StakerVault:371
SwapperRegistry:35
Manual Analysis
Replace require statements with custom errors.
If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.
Instances include:
RoleManager.sol:80 RoleManager.sol:110 RoleManager.sol:111
CompoundHandler.sol:135
CTokenRegistry.sol:61
TopUpAction.sol:188 TopUpAction.sol:452 TopUpAction.sol:456 TopUpAction.sol:479 TopUpAction.sol:506 TopUpAction.sol:891
TopUpActionKeeperHandler.sol:43 TopUpActionKeeperHandler.sol:46 TopUpActionKeeperHandler.sol:72 TopUpActionKeeperHandler.sol:93 TopUpActionKeeperHandler.sol:165
LiquidityPool.sol:483
ConvexStrategyBase.sol:313 ConvexStrategyBase.sol:380
Vault.sol:42 Vault.sol:135 Vault.sol:583
BkdLocker.sol:133 BkdLocker.sol:310
Controller:114 Controller:117
CvxCrvRewardsLocker:43
StakerVault:144 StakerVault:260
Manual Analysis
Remove explicit initialization for default values.
Prefix increments are cheaper than postfix increments.
Instances include:
RoleManager.sol:80
CompoundHandler.sol:135
CTokenRegistry.sol:61
TopUpAction.sol:188 TopUpAction.sol:456 TopUpAction.sol:479 TopUpAction.sol:506 TopUpAction.sol:891
TopUpActionKeeperHandler.sol:43 TopUpActionKeeperHandler.sol:46 TopUpActionKeeperHandler.sol:50 TopUpActionKeeperHandler.sol:72 TopUpActionKeeperHandler.sol:93 TopUpActionKeeperHandler.sol:165
ConvexStrategyBase.sol:313 ConvexStrategyBase.sol:380
BkdLocker.sol:310
Controller:117
StakerVault:260
Manual Analysis
change variable++ to ++variable
Redundant code should be avoided as it costs unnecessary gas
Instances include:
Preparable.sol:140: address oldValue = currentAddresses[key]; currentAddresses[key] = value; pendingAddresses[key] = address(0); deadlines[key] = 0; emit ConfigUpdatedAddress(key, oldValue, value); return value;
We can update currentAddresses[key] after emitting the event to save the gas of the declaration of oldValue:
+emit ConfigUpdatedAddress(key, currentAddresses[key], value); pendingAddresses[key] = address(0); deadlines[key] = 0; currentAddresses[key] = value; return value;
Manual Analysis
see Proof of Concept for mitigation steps.
Require statements including conditions with the && operator can be broken down in multiple require statements to save gas.
Instances include:
TopUpAction:360: require( newSwapperSlippage >= _MIN_SWAPPER_SLIPPAGE && newSwapperSlippage <= _MAX_SWAPPER_SLIPPAGE, Error.INVALID_AMOUNT );
ConvexStrategyBase:274: require( token_ != address(_CVX) && token_ != address(underlying) && token_ != address(_CRV), Error.INVALID_TOKEN_TO_ADD );
SwapperRegistry:35: require( fromToken != toToken && fromToken != address(0) && toToken != address(0) && newSwapper != address(0), Error.INVALID_TOKEN_PAIR );
Manual Analysis
Breakdown each condition in a separate require statement (though require statements should be replaced with custom errors)
require( newSwapperSlippage >=_MIN_SWAPPER_SLIPPAGE); require(newSwapperSlippage <= _MAX_SWAPPER_SLIPPAGE, Error.INVALID_AMOUNT)
Solidity contracts have contiguous 32 bytes (256 bits) slots used in storage. By arranging the variables, it is possible to minimize the number of slots used within a contract's storage and therefore reduce deployment costs.
address type variables are each of 20 bytes size (way less than 32 bytes). However, they here take up a whole 32 bytes slot (they are contiguous).
As bool type variables are of size 1 byte, there's a slot here that can get saved by moving them closer to an address
Instances include:
VaultStorage.sol:11: address public pool; uint256 public totalDebt; bool public strategyActive;
Manual Analysis
Place strategyActive after pool to save one storage slot
address public pool; +bool public strategyActive; uint256 public totalDebt;
The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.
if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas
Instances include:
LiquidityPool.sol:751: underlyingBalance - underlyingToWithdraw; //because of the condition line 745, the underflow check is unnecessary
BkdEthCvx.sol:83: uint256 requiredUnderlyingAmount = amount - underlyingBalance; //because of the condition line 76, the underflow check is unnecessary
BkdTriHopCvx.sol:181: uint256 requiredUnderlyingAmount = amount - underlyingBalance; //because of the condition line 174, the underflow check is unnecessary
CvxMintAmount.sol:24: uint256 remaining = _CLIFF_COUNT - currentCliff; //because of the condition line 21, the underflow check is unnecessary
Vault.sol:24: uint256 remaining = _CLIFF_COUNT - currentCliff; //because of the condition line 21, the underflow check is unnecessary Vault.sol:125: uint256 requiredWithdrawal = amount - availableUnderlying_; //because of the condition line 122, the underflow check is unnecessary Vault.sol:130: uint256 newTarget = (allocated - requiredWithdrawal) //because of the condition line 127, the underflow check is unnecessary Vault.sol:141: uint256 totalUnderlyingAfterWithdraw = totalUnderlying - amount; //because of the condition line 122, the underflow check is unnecessary Vault.sol:440: waitingForRemovalAllocated = _waitingForRemovalAllocated - withdrawn; //because of the condition line 437, the underflow check is unnecessary Vault.sol:444: uint256 profit = withdrawn - allocated; //because of the condition line 443, the underflow check is unnecessary Vault.sol:452: allocated -= withdrawn; //because of the condition line 443, the underflow check is unnecessary Vault.sol:591: uint256 profit = allocatedUnderlying - amountAllocated; //because of the condition line 589, the underflow check is unnecessary Vault.sol:595: profit -= currentDebt; //because of the condition line 593, the underflow check is unnecessary Vault.sol:600: currentDebt -= profit; //because of the condition line 593, the underflow check is unnecessary Vault.sol:605: uint256 loss = amountAllocated - allocatedUnderlying; //because of the condition line 603, the underflow check is unnecessary Vault.sol:784: uint256 withdrawAmount = allocatedUnderlying - target; //because of the condition line 782, the underflow check is unnecessary Vault.sol:790: uint256 depositAmount = target - allocatedUnderlying; //because of the condition line 788, the underflow check is unnecessary
StakerVault.sol:164: uint256 srcTokensNew = srcTokens - amount; //because of the condition line 153, the underflow check is unnecessary
Manual Analysis
Place the arithmetic operations in an unchecked block
#0 - chase-manning
2022-04-28T09:39:53Z
I consider this report to be of particularly high quality