Platform: Code4rena
Start Date: 05/05/2022
Pot Size: $125,000 DAI
Total HM: 17
Participants: 62
Period: 14 days
Judge: leastwood
Total Solo HM: 15
Id: 120
League: ETH
Rank: 15/62
Findings: 2
Award: $706.87
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0x4non, 0xDjango, 0xNazgul, 0xkatana, 0xsomeone, AuditsAreUS, BouSalman, BowTiedWardens, Cityscape, Funen, GimelSec, Hawkeye, JC, MaratCerby, MiloTruck, Picodes, Ruhum, TerrierLover, WatchPug, Waze, bobirichman, catchup, cccz, cryptphi, csanuragjain, delfin454000, ellahi, fatherOfBlocks, hake, horsefacts, hyh, jayjonah8, joestakey, kebabsec, kenta, mics, oyc_109, robee, samruna, shenwilly, sikorico, simon135, throttle, tintin
183.1627 DAI - $183.16
Table of Contents:
As per OpenZeppelin’s (OZ) recommendation, “The guidelines are now to make it impossible for anyone to run initialize on an implementation contract, by adding an empty constructor with the initializer modifier. So the implementation contract gets initialized automatically upon deployment.”
Note that this behaviour is also incorporated the OZ Wizard since the UUPS vulnerability discovery: “Additionally, we modified the code generated by the Wizard 19 to include a constructor that automatically initializes the implementation when deployed.”
Furthermore, this thwarts any attempts to frontrun the initialization tx of these contracts:
contracts-full/CrossChainCanonicalAlchemicTokenV2.sol: 8: function initialize( 9 string memory name, 10 string memory symbol, 11 address[] memory _bridgeTokens 12: ) public initializer { contracts-full/CrossChainCanonicalGALCX.sol: 7: function initialize( 8 string memory name, 9 string memory symbol, 10 address[] memory _bridgeTokens 11: ) public initializer {
Using this deprecated function can lead to unintended reverts and potentially the locking of funds. A deeper discussion on the deprecation of this function is in OZ issue #2219 (OpenZeppelin/openzeppelin-contracts#2219). The OpenZeppelin ERC20 safeApprove() function has been deprecated, as seen in the comments of the OpenZeppelin code.
As recommended by the OpenZeppelin comment, I suggest replacing safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead:
contracts-full/AlchemistV2.sol: 382: TokenUtils.safeApprove(yieldToken, config.adapter, type(uint256).max); 383: TokenUtils.safeApprove(adapter.underlyingToken(), config.adapter, type(uint256).max); 478: TokenUtils.safeApprove(yieldToken, adapter, type(uint256).max); 479: TokenUtils.safeApprove(ITokenAdapter(adapter).underlyingToken(), adapter, type(uint256).max); contracts-full/EthAssetManager.sol: 576: SafeERC20.safeApprove(address(tokens[i]), address(metaPool), 0); 577: SafeERC20.safeApprove(address(tokens[i]), address(metaPool), amounts[i]); 620: SafeERC20.safeApprove(address(token), address(metaPool), 0); 621: SafeERC20.safeApprove(address(token), address(metaPool), amount); 671: SafeERC20.safeApprove(address(metaPool), address(convexBooster), 0); 672: SafeERC20.safeApprove(address(metaPool), address(convexBooster), amount); contracts-full/ThreePoolAssetManager.sol: 782: SafeERC20.safeApprove(address(tokens[i]), address(threePool), 0); 783: SafeERC20.safeApprove(address(tokens[i]), address(threePool), amounts[i]); 838: SafeERC20.safeApprove(address(token), address(threePool), 0); 839: SafeERC20.safeApprove(address(token), address(threePool), amount); 879: SafeERC20.safeApprove(address(threePoolToken), address(threePool), 0); 880: SafeERC20.safeApprove(address(threePoolToken), address(threePool), amount); 908: SafeERC20.safeApprove(address(tokens[i]), address(metaPool), 0); 909: SafeERC20.safeApprove(address(tokens[i]), address(metaPool), amounts[i]); 944: SafeERC20.safeApprove(address(token), address(metaPool), 0); 945: SafeERC20.safeApprove(address(token), address(metaPool), amount); 987: SafeERC20.safeApprove(address(metaPool), address(convexBooster), 0); 988: SafeERC20.safeApprove(address(metaPool), address(convexBooster), amount); contracts-full/TransmuterBuffer.sol: 236: TokenUtils.safeApprove(registeredUnderlyings[i], alchemist, 0); //// 238: TokenUtils.safeApprove(debtToken, alchemist, 0); //// 243: TokenUtils.safeApprove(registeredUnderlyings[i], alchemist, type(uint256).max); //// 245: TokenUtils.safeApprove(debtToken, alchemist, type(uint256).max); //// 284: TokenUtils.safeApprove(underlyingToken, alchemist, type(uint256).max); //// contracts-full/adapters/fuse/FuseTokenAdapterV1.sol: 71: SafeERC20.safeApprove(underlyingToken, token, amount); contracts-full/adapters/lido/WstETHAdapterV1.sol: 105: SafeERC20.safeApprove(parentToken, address(token), mintedStEth); 129: SafeERC20.safeApprove(parentToken, curvePool, unwrappedStEth); contracts-full/adapters/vesper/VesperAdapterV1.sol: 62: SafeERC20.safeApprove(underlyingToken, token, amount); contracts-full/adapters/yearn/YearnTokenAdapter.sol: 32: TokenUtils.safeApprove(underlyingToken, token, amount);
Consider resolving the TODOs before deploying.
File: IStableMetaPool.sol 6: /// @dev TODO 7: uint256 constant N_COINS = 2;
contracts-full/TransmuterBuffer.sol: 7: import "@openzeppelin/contracts/utils/math/SafeMath.sol"; 27: using SafeMath for uint256;
Using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity:
contracts-full/EthAssetManager.sol: 367: ) external lock onlyOperator returns (uint256 minted) { //@audit unused named returns 380: ) external lock onlyOperator returns (uint256 minted) { //@audit unused named returns 393: ) external lock onlyOperator returns (uint256 withdrawn) { //@audit unused named returns 404: ) external lock onlyOperator returns (bool success) { //@audit unused named returns 415: ) external lock onlyOperator returns (bool success) { //@audit unused named returns 521: ) external lock onlyAdmin returns (bytes memory result) { //@audit unused named returns contracts-full/ThreePoolAssetManager.sol: 334: ) public view returns (uint256 delta, bool add) { //@audit unused named returns 534: ) external lock onlyOperator returns (uint256 minted) { //@audit unused named returns 547: ) external lock onlyOperator returns (uint256 minted) { //@audit unused named returns 560: ) external lock onlyOperator returns (uint256 withdrawn) { //@audit unused named returns 571: ) external lock onlyOperator returns (uint256 minted) { //@audit unused named returns 584: ) external lock onlyOperator returns (uint256 minted) { //@audit unused named returns 597: ) external lock onlyOperator returns (uint256 withdrawn) { //@audit unused named returns 608: ) external lock onlyOperator returns (bool success) { //@audit unused named returns 619: ) external lock onlyOperator returns (bool success) { //@audit unused named returns contracts-full/TransmuterBuffer.sol: 134: returns (uint256 weight) //@audit unused named returns contracts-full/TransmuterV2.sol: 350: function getUnexchangedBalance(address owner) external view override returns (uint256 unexchangedBalance) { //@audit unused named returns 370: function getExchangedBalance(address owner) external view override returns (uint256 exchangedBalance) { //@audit unused named returns 374: function getClaimableBalance(address owner) external view override returns (uint256 claimableBalance) { //@audit unused named returns 554: function _getExchangedBalance(address owner) internal view returns (uint256 exchangedBalance) { //@audit unused named returns
#0 - 0xfoobar
2022-05-30T07:40:42Z
Good QA
🌟 Selected for report: IllIllI
Also found by: 0v3rf10w, 0x1f8b, 0x4non, 0xDjango, 0xNazgul, 0xf15ers, 0xkatana, 0xsomeone, AlleyCat, BowTiedWardens, Cityscape, Fitraldys, Funen, GimelSec, Hawkeye, JC, MaratCerby, MiloTruck, Randyyy, TerrierLover, Tomio, UnusualTurtle, WatchPug, Waze, _Adam, augustg, bobirichman, catchup, csanuragjain, ellahi, fatherOfBlocks, hake, hansfriese, horsefacts, ignacio, joestakey, kenta, mics, oyc_109, robee, samruna, sashik_eth, sikorico, simon135, throttle
523.7138 DAI - $523.71
Table of Contents:
memory keyword when storage should be used>= is cheaper than >Mutex.sol and Whitelist.sol: Switching between 1 and 2 instead of 0 and 1 (or false and true) is more gas efficient++i costs less gas compared to i++ or i += 1To help the optimizer, declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array.
The effect can be quite significant.
As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex] and use it.
Instances include (check the @audit tags):
897: uint256 shares = _yieldTokens[yieldToken].totalShares - _accounts[msg.sender].balances[yieldToken]; //@audit gas: for storage optimization, declare "YieldTokenParams storage _yieldToken = _yieldTokens[yieldToken];" like L912 and "Account storage _account = _accounts[msg.sender];" like L1365 899: _yieldTokens[yieldToken].accruedWeight += amount * FIXED_POINT_SCALAR / shares;//@audit gas: use suggested _yieldToken 900: _accounts[msg.sender].lastAccruedWeights[yieldToken] = _yieldTokens[yieldToken].accruedWeight;//@audit gas: use suggested _account and _yieldToken 1003: uint256 activeBalance = _yieldTokens[yieldToken].activeBalance;//@audit gas: for storage optimization, declare "YieldTokenParams storage _yieldToken = _yieldTokens[yieldToken];" 1009: uint256 expectedValue = _yieldTokens[yieldToken].expectedValue; //@audit gas: use suggested _yieldToken 1018: _yieldTokens[yieldToken].activeBalance -= harvestable; //@audit gas: use suggested _yieldToken 1019: _yieldTokens[yieldToken].harvestableBalance += harvestable; //@audit gas: use suggested _yieldToken 1460: Sets.AddressSet storage depositedTokens = _accounts[owner].depositedTokens; //@audit gas: should declare "Account storage _account = _accounts[owner];" like L1365 1464: uint256 shares = _accounts[owner].balances[yieldToken]; //@audit gas: should use suggested _account 1521: int256 debt = _accounts[owner].debt; //@audit gas: should declare "Account storage _account = _accounts[owner];" 1523: Sets.AddressSet storage depositedTokens = _accounts[owner].depositedTokens; //@audit gas: should use suggested _account 1527: uint256 currentAccruedWeight = _yieldTokens[yieldToken].accruedWeight;//@audit gas: for storage optimization, declare "YieldTokenParams storage _yieldToken = _yieldTokens[yieldToken];" 1528: uint256 lastAccruedWeight = _accounts[owner].lastAccruedWeights[yieldToken]; //@audit gas: should use suggested _account 1532: ? unlockedCredit * FIXED_POINT_SCALAR / _yieldTokens[yieldToken].totalShares //@audit gas: should use suggested _yieldToken 1539: uint256 balance = _accounts[owner].balances[yieldToken]; //@audit gas: should use suggested _account
memory keyword when storage should be usedWhen copying a state struct in memory, there are as many SLOADs and MSTOREs as there are slots. When reading the whole struct multiple times is not needed, it's better to actually only read the relevant field(s). When only some of the fields are read several times, these particular values should be cached instead of the whole state struct.
Here, the storage keyword should be used instead of memory (see @audit tags):
1222 function _sync( 1223 address yieldToken, 1224 uint256 amount, 1225 function(uint256, uint256) internal pure returns (uint256) operation 1226 ) internal { 1227: YieldTokenParams memory yieldTokenParams = _yieldTokens[yieldToken]; //@audit gas: should use storage instead of memory (only 2 SLOADs are necessary vs 14 done here for the copy in memory) 1228 1229 uint256 amountUnderlyingTokens = _convertYieldTokensToUnderlying(yieldToken, amount); 1230 uint256 updatedActiveBalance = operation(yieldTokenParams.activeBalance, amount); //@audit gas: yieldTokenParams occurence 1 1231 uint256 updatedExpectedValue = operation(yieldTokenParams.expectedValue, amountUnderlyingTokens); //@audit gas: yieldTokenParams occurence 2 1232 1233 _yieldTokens[yieldToken].activeBalance = updatedActiveBalance; 1234 _yieldTokens[yieldToken].expectedValue = updatedExpectedValue; 1235 } 1243 function _loss(address yieldToken) internal view returns (uint256) { 1244: YieldTokenParams memory yieldTokenParams = _yieldTokens[yieldToken]; //@audit gas: should use storage instead of memory (only 2 SLOADs are necessary vs 14 done here for the copy in memory) 1245 1246 uint256 amountUnderlyingTokens = _convertYieldTokensToUnderlying(yieldToken, yieldTokenParams.activeBalance); //@audit gas: yieldTokenParams occurence 1 1247 uint256 expectedUnderlyingValue = yieldTokenParams.expectedValue; //@audit gas: yieldTokenParams occurence 2 1248 1249 return expectedUnderlyingValue > amountUnderlyingTokens 1250 ? ((expectedUnderlyingValue - amountUnderlyingTokens) * BPS) / expectedUnderlyingValue 1251 : 0; 1252 }
The code can be optimized by minimising the number of SLOADs.
SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.
See the @audit tags for details about the multiple SLOADs where a cached value should be used instead of SLOAD 2 and above:
289: _checkState(pendingAdmin != address(0)); //@audit gas: SLOAD 1 (pendingAdmin) 291: if (msg.sender != pendingAdmin) { //@audit gas: SLOAD 2 (pendingAdmin) 295: admin = pendingAdmin;//@audit gas: SLOAD 3 (pendingAdmin) 298: emit AdminUpdated(admin); //@audit gas: SLOAD 4 (previous pendingAdmin that should've been cached) 876: TokenUtils.safeTransfer(underlyingToken, transmuter, amountUnderlyingTokens); //@audit gas: SLOAD 1 (transmuter) 879: IERC20TokenReceiver(transmuter).onERC20Received(underlyingToken, amountUnderlyingTokens); //@audit gas: SLOAD 2 (transmuter) 942: TokenUtils.safeTransfer(underlyingToken, transmuter, distributeAmount); //@audit gas: SLOAD 1 (transmuter) 945: IERC20TokenReceiver(transmuter).onERC20Received(underlyingToken, distributeAmount); //@audit gas: SLOAD 2 (transmuter) 1608: if (_yieldTokens[yieldToken].totalShares == 0) { //@audit gas: SLOAD 1 (_yieldTokens[yieldToken].totalShares), should cache like in _convertSharesToYieldTokens 1611: return amount * _yieldTokens[yieldToken].totalShares / _calculateUnrealizedActiveBalance(yieldToken); //@audit gas: SLOAD 2 (_yieldTokens[yieldToken].totalShares)
304: if (pendingAdmin == address(0)) { //@audit gas: SLOAD 1 (pendingAdmin) 308: if (pendingAdmin != msg.sender) { //@audit gas: SLOAD 2 (pendingAdmin) 312: admin = pendingAdmin; //@audit gas: SLOAD 3 (pendingAdmin) 315: emit AdminUpdated(admin); //@audit gas: SLOAD 4 (pendingAdmin), as this SLOAD could be avoided by using the suggested cache 496: if (amount > (balance = weth.balanceOf(address(this)))) weth.deposit{value: amount - balance}(); //@audit gas: SLOAD 1 & 2 (weth) 498: SafeERC20.safeTransfer(address(weth), transmuterBuffer, amount); //@audit gas: SLOAD 1 (transmuterBuffer) + SLOAD 3 (weth) 500: IERC20TokenReceiver(transmuterBuffer).onERC20Received(address(weth), amount); //@audit gas: SLOAD 2 (transmuterBuffer) + SLOAD 4 (weth) 698: SafeERC20.safeTransfer(address(curveToken), rewardReceiver, curveBalance); //@audit gas: SLOAD 1 (rewardReceiver) 699: SafeERC20.safeTransfer(address(convexToken), rewardReceiver, convexBalance); //@audit gas: SLOAD 2 (rewardReceiver)
50: uint poolBalance = pools.getStakeTotalDeposited(address(this), poolId); //@audit SLOAD 1 (poolId) 51: pools.withdraw(poolId, poolBalance); //@audit SLOAD 2 (poolId) 58: pools.deposit(poolId, balance); //@audit SLOAD 3 (poolId), should use _poolId 71: pools.claim(poolId); //@audit SLOAD 1 (poolId) 76: exchangeRate += (balance * exchangeRatePrecision) / totalSupply; //@audit SLOAD 1 (exchangeRate), should cache the calculation in a var 77: emit ExchangeRateChange(exchangeRate); //@audit SLOAD 2 (exchangeRate) 79: pools.deposit(poolId, balance); //@audit SLOAD 2 (poolId)
131: require(msg.sender == pendingGovernance, "StakingPools: only pending governance"); //@audit SLOAD 1 (pendingGovernance) 133: address _pendingGovernance = pendingGovernance; //@audit SLOAD 2 (pendingGovernance), should declare this earlier and use _pendingGovernance L131
456: if (pendingAdmin == address(0)) { //@audit SLOAD 1 (pendingAdmin) 460: if (pendingAdmin != msg.sender) { //@audit SLOAD 2 (pendingAdmin) 464: admin = pendingAdmin; //@audit SLOAD 3 (pendingAdmin) 467: emit AdminUpdated(admin); //@audit SLOAD 4 (pendingAdmin), could've been avoided by caching pendingAdmin in memory before doing it in "admin" 632: SafeERC20.safeTransfer(address(curveToken), rewardReceiver, curveBalance); //@audit SLOAD 1 (rewardReceiver) 633: SafeERC20.safeTransfer(address(convexToken), rewardReceiver, convexBalance); //@audit SLOAD 2 (rewardReceiver) 719: SafeERC20.safeTransfer(address(token), transmuterBuffer, amount); //@audit SLOAD 1 (transmuterBuffer) 721: IERC20TokenReceiver(transmuterBuffer).onERC20Received(address(token), amount); //@audit SLOAD 2 (transmuterBuffer) 1014: SafeERC20.safeTransfer(address(curveToken), rewardReceiver, curveBalance); //@audit SLOAD 1 (rewardReceiver) 1015: SafeERC20.safeTransfer(address(convexToken), rewardReceiver, convexBalance); //@audit SLOAD 2 (rewardReceiver)
151: if (totalUnderlyingBuffered < flowAvailable[underlyingToken]) {//@audit SLOAD 1 (flowAvailable[underlyingToken]) 154: return flowAvailable[underlyingToken];//@audit SLOAD 2 (flowAvailable[underlyingToken]) 231: sources[alchemist] = false; //@audit SLOAD 1 (alchemist) 234: if (alchemist != address(0)) { //@audit SLOAD 2 (alchemist) 236: TokenUtils.safeApprove(registeredUnderlyings[i], alchemist, 0); //@audit SLOAD 3 (alchemist) 238: TokenUtils.safeApprove(debtToken, alchemist, 0); //@audit SLOAD 4 (alchemist) 243: TokenUtils.safeApprove(registeredUnderlyings[i], alchemist, type(uint256).max); //@audit SLOAD 5, should use _alchemist 245: TokenUtils.safeApprove(debtToken, alchemist, type(uint256).max); //@audit SLOAD 6, should use _alchemist 247: emit SetAlchemist(alchemist); //@audit SLOAD 7, should use _alchemist 267: if (!IAlchemistV2(alchemist).isSupportedUnderlyingToken(underlyingToken)) { //@audit SLOAD 1 (alchemist) 284: TokenUtils.safeApprove(underlyingToken, alchemist, type(uint256).max); //@audit SLOAD 2 (alchemist) 315: if (localBalance < flowAvailable[underlyingToken]) { //@audit SLOAD 1 (flowAvailable[underlyingToken]) 319: uint256 exchangeable = flowAvailable[underlyingToken] - currentExchanged[underlyingToken]; //@audit SLOAD 2 (flowAvailable[underlyingToken]) + SLOAD 1 (currentExchanged[underlyingToken]) 320: currentExchanged[underlyingToken] += exchangeable;//@audit SLOAD 2 (currentExchanged[underlyingToken]) 346: if (amount > flowAvailable[underlyingToken]) { //@audit SLOAD 1 (flowAvailable[underlyingToken]) 355: flowAvailable[underlyingToken] -= amount; //@audit gas: should be unchecked due to L346-L347 + //@audit SLOAD 2 (flowAvailable[underlyingToken]) 372: address[] memory supportedYieldTokens = IAlchemistV2(alchemist) //@audit SLOAD 1 (alchemist) 374: address[] memory supportedUnderlyingTokens = IAlchemistV2(alchemist) //@audit SLOAD 2 (alchemist) 377: if (registeredUnderlyings.length != supportedUnderlyingTokens.length) { //@audit SLOAD 1 (registeredUnderlyings.length) 382: for (uint256 j = 0; j < registeredUnderlyings.length; j++) { //@audit SLOAD 2+ in loop (registeredUnderlyings.length) 390: IAlchemistV2.YieldTokenParams memory params = IAlchemistV2(alchemist) //@audit SLOAD 3+ in loop (alchemist) 401: IAlchemistV2(alchemist).poke(address(this)); //@audit SLOAD 1 (alchemist) 406: IAlchemistV2(alchemist).mint(credit, address(this)); //@audit SLOAD 2 (alchemist) 443: (uint256 balance, ) = IAlchemistV2(alchemist).positions(address(this), yieldToken); //@audit SLOAD 1 (alchemist) 444: IAlchemistV2.YieldTokenParams memory params = IAlchemistV2(alchemist) //@audit SLOAD 2 (alchemist) 446: uint256 tokensPerShare = IAlchemistV2(alchemist) //@audit SLOAD 3 (alchemist) 457: lastFlowrateUpdate[underlyingToken]) * flowRate[underlyingToken]; //@audit SLOAD 1 (flowRate[underlyingToken]) and SLOAD 1 (lastFlowrateUpdate[underlyingToken]) 458: flowAvailable[underlyingToken] += marginalFlow; //@audit SLOAD 2 (flowRate[underlyingToken]) 459: lastFlowrateUpdate[underlyingToken] = block.timestamp; //@audit SLOAD 2 (lastFlowrateUpdate[underlyingToken]) 513: uint256 pricePerShare = IAlchemistV2(alchemist).getUnderlyingTokensPerShare(token); //@audit SLOAD 1 (alchemist) 515: (uint256 availableShares, uint256 lastAccruedWeight) = IAlchemistV2(alchemist).positions(address(this), token); //@audit SLOAD 2 (alchemist) 522: IAlchemistV2(alchemist).withdrawUnderlying(token, wantShares, address(this), minimumAmountOut); //@audit SLOAD 3 (alchemist) 536: if (totalUnderlyingBuffered < flowAvailable[underlyingToken]) { //@audit SLOAD 1 (flowAvailable[underlyingToken]) 539: } else if (initialLocalBalance < flowAvailable[underlyingToken]) { //@audit SLOAD 2 (flowAvailable[underlyingToken]) 541: want = flowAvailable[underlyingToken] - initialLocalBalance; //@audit SLOAD 3 (flowAvailable[underlyingToken]) 550: if (localBalance > flowAvailable[underlyingToken]) { //@audit SLOAD 4 (flowAvailable[underlyingToken]) 551: exchangeDelta = flowAvailable[underlyingToken] - currentExchanged[underlyingToken]; //@audit SLOAD 5 (flowAvailable[underlyingToken]) + //@audit SLOAD 1 (currentExchanged[underlyingToken]) 553: exchangeDelta = localBalance - currentExchanged[underlyingToken]; //@audit SLOAD 1 (currentExchanged[underlyingToken]) 557: currentExchanged[underlyingToken] += exchangeDelta; //@audit SLOAD 2 (currentExchanged[underlyingToken]) 567: TokenUtils.safeTransfer(underlyingToken, amos[underlyingToken], amount); //@audit gas: SLOAD 1 (amos[underlyingToken]) 568: IERC20TokenReceiver(amos[underlyingToken]).onERC20Received(underlyingToken, amount); //@audit gas: SLOAD 2 (amos[underlyingToken])
35: IERC20(token).safeTransferFrom(origin, sinkTransmuter, amount);//@audit SLOAD 1 (token) and SLOAD 1 (sinkTransmuter) 36: IERC20TokenReceiver(sinkTransmuter).onERC20Received(token, amount);//@audit SLOAD 2 (token) and SLOAD 2 (sinkTransmuter)
557: if (account.occupiedTick <= satisfiedTick) { //@audit SLOAD 1 (account.occupiedTick) 567: ticks.getWeight(account.occupiedTick, ticks.position) //@audit SLOAD 2 (account.occupiedTick)
SSTOREs are expensive.
Instead of potentially making multiple double-SSTOREs in for-loops here:
File: EthAssetManager.sol 214: for (uint256 i = 0; i < NUM_META_COINS; i++) { 215: _metaPoolAssetCache[i] = params.metaPool.coins(i); //@audit gas SSTORE 1 216: if (_metaPoolAssetCache[i] == IERC20(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE)) { //@audit SLOAD 217: _metaPoolAssetCache[i] = weth; //@audit SSTORE 2 + SLOAD 218: } 219: }
Consider using a ternary operator L215 and removing the rest:
_metaPoolAssetCache[i] = params.metaPool.coins(i) == IERC20(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE) ? weth : params.metaPool.coins(i);
While MSTOREs are not expensive, the following could be optimized (see @audit tags):
File: TransmuterV2.sol 554: function _getExchangedBalance(address owner) internal view returns (uint256 exchangedBalance) { 555: Account storage account = accounts[owner]; 556: 557: if (account.occupiedTick <= satisfiedTick) { 558: exchangedBalance = account.exchangedBalance; //@audit gas: see below 559: exchangedBalance += account.unexchangedBalance; //@audit gas: would be less expensive with "exchangedBalance = account.exchangedBalance + account.unexchangedBalance;" 560: return exchangedBalance; 561: } 562: 563: exchangedBalance = account.exchangedBalance; //@audit gas: see L570 564: 565: uint256 exchanged = LiquidityMath.calculateProduct( 566: account.unexchangedBalance, 567: ticks.getWeight(account.occupiedTick, ticks.position) 568: ); 569: 570: exchangedBalance += exchanged; //@audit gas: would be less expensive with "exchangedBalance = account.exchangedBalance + exchanged;" 571: 572: return exchangedBalance; 573: }
When they are the same, consider emitting the memory value instead of the storage value:
contracts-full/AlchemicTokenV2.sol: 92 function setFlashFee(uint256 newFee) external onlyAdmin { 93 flashMintFee = newFee; 94: emit SetFlashMintFee(flashMintFee); //@audit SLOAD (flashMintFee), should emit newFee 95 } contracts-full/EthAssetManager.sol: 221: emit AdminUpdated(admin); //@audit gas: SLOAD, use params.admin 222: emit OperatorUpdated(operator); //@audit gas: SLOAD, use params.operator 223: emit RewardReceiverUpdated(rewardReceiver); //@audit gas: SLOAD, use params.rewardReceiver 224: emit TransmuterBufferUpdated(transmuterBuffer); //@audit gas: SLOAD, use params.transmuterBuffer 225: emit MetaPoolSlippageUpdated(metaPoolSlippage); //@audit gas: SLOAD, use params.metaPoolSlippage contracts-full/ThreePoolAssetManager.sol: 258: emit AdminUpdated(admin); //@audit gas: SLOAD, should use params.admin 259: emit OperatorUpdated(operator); //@audit gas: SLOAD, should use params.operator 260: emit RewardReceiverUpdated(rewardReceiver); //@audit gas: SLOAD, should use params.rewardReceiver 261: emit TransmuterBufferUpdated(transmuterBuffer); //@audit gas: SLOAD, should use params.transmuterBuffer 262: emit ThreePoolSlippageUpdated(threePoolSlippage); //@audit gas: SLOAD, should use params.threePoolSlippage 263: emit MetaPoolSlippageUpdated(metaPoolSlippage); //@audit gas: SLOAD, should use params.metaPoolSlippage
Solidity version 0.8.already implements overflow and underflow checks by default. Using the SafeMath library from OpenZeppelin (which is more gas expensive than the 0.8. overflow checks) is therefore redundant.
Instances include:
TransmuterBuffer.sol:7:import "@openzeppelin/contracts/utils/math/SafeMath.sol"; TransmuterBuffer.sol:27: using SafeMath for uint256;
Consider using the built-in checks instead of SafeMath and remove SafeMath from the dependencies
Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic
I suggest wrapping with an unchecked block here (see @audit tags for more details):
1010 if (currentValue <= expectedValue) { 1011 return; 1012 } 1013 1014: uint256 harvestable = _convertUnderlyingTokensToYield(yieldToken, currentValue - expectedValue); //@audit gas: should be unchecked due to L1010-L1011
496: if (amount > (balance = weth.balanceOf(address(this)))) weth.deposit{value: amount - balance}(); //@audit gas: should be unchecked due to condition 547 if (cliff >= totalCliffs) return 0; 548 549: uint256 reduction = totalCliffs - cliff; //@audit gas: should be unchecked due to L547 550: uint256 earned = amountCurve * reduction / totalCliffs; //@audit gas: should be unchecked due to "(reduction / totalCliffs) < 1" 589: if (value > address(this).balance) weth.withdraw(value - address(this).balance); //@audit gas: should be unchecked due to condition 629: if (value > address(this).balance) weth.withdraw(value - address(this).balance); //@audit gas: should be unchecked due to condition 720 function abs(uint256 x , uint256 y) private pure returns (uint256) { 721: return x > y ? x - y : y - x; //@audit gas: should be unchecked due to condition 722 }
399 return v.minimizedBalance > v.startingBalance 400: ? (v.minimizedBalance - v.startingBalance, true) //@audit gas: should be unchecked due to condition 401: : (v.startingBalance - v.minimizedBalance, false); //@audit gas: should be unchecked due to condition 749 if (cliff >= totalCliffs) return 0; 750 751: uint256 reduction = totalCliffs - cliff; //@audit gas: should be unchecked due to L749 1036 function abs(uint256 x , uint256 y) private pure returns (uint256) { 1037: return x > y ? x - y : y - x; //@audit gas: should be unchecked due to condition 1038 } 1039 }
346 if (amount > flowAvailable[underlyingToken]) { //// 347 revert IllegalArgument(); 348 } 349 350 uint256 localBalance = TokenUtils.safeBalanceOf(underlyingToken, address(this)); 351 if (amount > localBalance) { 352 revert IllegalArgument(); 353 } 354 355: flowAvailable[underlyingToken] -= amount; //@audit gas: should be unchecked due to L346-L347 421 if (localBalance < amount) { 422 revert IllegalArgument(); 423 } 424 _updateFlow(underlyingToken); 425 426 // Don't deposit exchanged funds into the Alchemist. 427 // Doing so puts those funds at risk, and could lead to users being unable to claim 428 // their transmuted funds in the event of a vault loss. 429: if (localBalance - amount < currentExchanged[underlyingToken]) { //@audit gas: should be unchecked due to L421 539 } else if (initialLocalBalance < flowAvailable[underlyingToken]) { //// 540 // totalUnderlyingBuffered > flowAvailable so we have funds available to pull. 541: want = flowAvailable[underlyingToken] - initialLocalBalance; //@audit gas: should be unchecked due to condition L539 542 }
306 if (state.maximumWeight.n < FixedPointMath.ONE) { 307 break; 308 } 309 310 // Calculate how much weight of the distributed weight is dust. 311: state.dustedWeight = FixedPointMath.Number(state.maximumWeight.n - FixedPointMath.ONE); //@audit gas: should be unchecked due to L306-L307
From :
06: /// @notice Defines underlying token parameters. 07: struct UnderlyingTokenParams { //@audit gas: can be tightly packed 08: // The number of decimals the token has. This value is cached once upon registering the token so it is important 09: // that the decimals of the token are immutable or the system will begin to have computation errors. 10: uint8 decimals; //@audit-info gas: 1 byte 11: // A coefficient used to normalize the token to a value comparable to the debt token. For example, if the 12: // underlying token is 8 decimals and the debt token is 18 decimals then the conversion factor will be 13: // 10^10. One unit of the underlying token will be comparably equal to one unit of the debt token. 14: uint256 conversionFactor; //@audit-info gas: 32 bytes 15: // A flag to indicate if the token is enabled. 16: bool enabled; //@audit-info gas: 1 byte 17: }
to:
struct UnderlyingTokenParams { //@audit gas: can be tightly packed // The number of decimals the token has. This value is cached once upon registering the token so it is important // that the decimals of the token are immutable or the system will begin to have computation errors. uint8 decimals; //@audit-info gas: 1 byte // A flag to indicate if the token is enabled. bool enabled; //@audit-info gas: 1 byte // A coefficient used to normalize the token to a value comparable to the debt token. For example, if the // underlying token is 8 decimals and the debt token is 18 decimals then the conversion factor will be // 10^10. One unit of the underlying token will be comparably equal to one unit of the debt token. uint256 conversionFactor; //@audit-info gas: 32 bytes }
Which would save 1 storage slot.
From:
19: /// @notice Defines yield token parameters. 20: struct YieldTokenParams { //@audit gas: can be tightly packed 21: // The number of decimals the token has. This value is cached once upon registering the token so it is important 22: // that the decimals of the token are immutable or the system will begin to have computation errors. 23: uint8 decimals; //@audit-info gas: 1 byte 24: // The associated underlying token that can be redeemed for the yield-token. 25: address underlyingToken; //@audit-info gas: 20 bytes 26: // The adapter used by the system to wrap, unwrap, and lookup the conversion rate of this token into its 27: // underlying token. 28: address adapter; //@audit-info gas: 20 bytes 29: // The maximum percentage loss that is acceptable before disabling certain actions. 30: uint256 maximumLoss; //@audit-info gas: 32 bytes 31: // The maximum value of yield tokens that the system can hold, measured in units of the underlying token. 32: uint256 maximumExpectedValue; 33: // The percent of credit that will be unlocked per block. The representation of this value is a 18 decimal 34: // fixed point integer. 35: uint256 creditUnlockRate; 36: // The current balance of yield tokens which are held by users. 37: uint256 activeBalance; 38: // The current balance of yield tokens which are earmarked to be harvested by the system at a later time. 39: uint256 harvestableBalance; 40: // The total number of shares that have been minted for this token. 41: uint256 totalShares; 42: // The expected value of the tokens measured in underlying tokens. This value controls how much of the token 43: // can be harvested. When users deposit yield tokens, it increases the expected value by how much the tokens 44: // are exchangeable for in the underlying token. When users withdraw yield tokens, it decreases the expected 45: // value by how much the tokens are exchangeable for in the underlying token. 46: uint256 expectedValue; 47: // The current amount of credit which is will be distributed over time to depositors. 48: uint256 pendingCredit; 49: // The amount of the pending credit that has been distributed. 50: uint256 distributedCredit; 51: // The block number which the last credit distribution occurred. 52: uint256 lastDistributionBlock; 53: // The total accrued weight. This is used to calculate how much credit a user has been granted over time. The 54: // representation of this value is a 18 decimal fixed point integer. 55: uint256 accruedWeight; //@audit-info gas: 32 bytes 56: // A flag to indicate if the token is enabled. 57: bool enabled; //@audit-info gas: 1 byte (should move as it's taking a whole slot but is just 1 byte) 58: }
to:
struct YieldTokenParams { //@audit gas: can be tightly packed // A flag to indicate if the token is enabled. bool enabled; //@audit-info gas: 1 byte (this moved up) // The number of decimals the token has. This value is cached once upon registering the token so it is important // that the decimals of the token are immutable or the system will begin to have computation errors. uint8 decimals; //@audit-info gas: 1 byte // The associated underlying token that can be redeemed for the yield-token. address underlyingToken; //@audit-info gas: 20 bytes // The adapter used by the system to wrap, unwrap, and lookup the conversion rate of this token into its // underlying token. address adapter; //@audit-info gas: 20 bytes // The maximum percentage loss that is acceptable before disabling certain actions. uint256 maximumLoss; //@audit-info gas: 32 bytes // The maximum value of yield tokens that the system can hold, measured in units of the underlying token. uint256 maximumExpectedValue; // The percent of credit that will be unlocked per block. The representation of this value is a 18 decimal // fixed point integer. uint256 creditUnlockRate; // The current balance of yield tokens which are held by users. uint256 activeBalance; // The current balance of yield tokens which are earmarked to be harvested by the system at a later time. uint256 harvestableBalance; // The total number of shares that have been minted for this token. uint256 totalShares; // The expected value of the tokens measured in underlying tokens. This value controls how much of the token // can be harvested. When users deposit yield tokens, it increases the expected value by how much the tokens // are exchangeable for in the underlying token. When users withdraw yield tokens, it decreases the expected // value by how much the tokens are exchangeable for in the underlying token. uint256 expectedValue; // The current amount of credit which is will be distributed over time to depositors. uint256 pendingCredit; // The amount of the pending credit that has been distributed. uint256 distributedCredit; // The block number which the last credit distribution occurred. uint256 lastDistributionBlock; // The total accrued weight. This is used to calculate how much credit a user has been granted over time. The // representation of this value is a 18 decimal fixed point integer. uint256 accruedWeight; //@audit-info gas: 32 bytes }
Which would save 1 storage slot.
From
10: struct StrategyParams { //@audit gas: can be tightly packed 11: uint256 performanceFee; 12: uint256 activation; 13: uint256 debtRatio; 14: uint256 minDebtPerHarvest; 15: uint256 maxDebtPerHarvest; 16: uint256 lastReport; 17: uint256 totalDebt; 18: uint256 totalGain; 19: uint256 totalLoss; //@audit-info gas: 32 bytes 20: bool enforceChangeLimit; //@audit-info gas: 1 byte (this should move to the end) 21: uint256 profitLimitRatio; //@audit-info gas: 32 bytes 22: uint256 lossLimitRatio; //@audit-info gas: 32 bytes 23: address customCheck; //@audit-info gas: 20 bytes 24: }
to
struct StrategyParams { //@audit gas: can be tightly packed uint256 performanceFee; uint256 activation; uint256 debtRatio; uint256 minDebtPerHarvest; uint256 maxDebtPerHarvest; uint256 lastReport; uint256 totalDebt; uint256 totalGain; uint256 totalLoss; //@audit-info gas: 32 bytes uint256 profitLimitRatio; //@audit-info gas: 32 bytes uint256 lossLimitRatio; //@audit-info gas: 32 bytes address customCheck; //@audit-info gas: 20 bytes bool enforceChangeLimit; //@audit-info gas: 1 byte (this moved) }
Which would save 1 storage slot.
>= is cheaper than >Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas)
I suggest using >= instead of > to avoid some opcodes here:
AlchemistV2.sol:729: uint256 credit = amount > uint256(debt) ? uint256(debt) : amount; AlchemistV2.sol:773: uint256 actualAmount = amount > maximumAmount ? maximumAmount : amount; AlchemistV2.sol:838: uint256 actualShares = shares > maximumShares ? maximumShares : shares; EthAssetManager.sol:553: return earned > available ? available : earned; EthAssetManager.sol:711: return x > y ? y : x; EthAssetManager.sol:721: return x > y ? x - y : y - x; ThreePoolAssetManager.sol:379: v.minimizedBalance = currentDelta > examineDelta ? examineBalance : v.minimizedBalance; ThreePoolAssetManager.sol:755: return earned > available ? available : earned; ThreePoolAssetManager.sol:1027: return x > y ? y : x; ThreePoolAssetManager.sol:1037: return x > y ? x - y : y - x;
A division by 2 can be calculated by shifting one to the right.
While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.
I suggest replacing / 2 with >> 1 here:
ThreePoolAssetManager.sol:355: if ((examineBalance = (v.maximum + v.minimum) / 2) == previousBalance) break;
Mutex.sol and Whitelist.sol: Switching between 1 and 2 instead of 0 and 1 (or false and true) is more gas efficientAffected code:
SSTORE from false (0) to true (1) (or any non-zero value) costs 20000 gas.
SSTORE from 1 to 2 (or any other non-zero value) costs 5000 gas.
By storing the original value once again, a refund is triggered (https://eips.ethereum.org/EIPS/eip-2200).
Since refunds are capped to a percentage of the total transaction's gas, it is best to keep them low, to increase the likelihood of the full refund coming into effect.
Therefore, switching between 1, 2 instead of 0, 1 will be more gas efficient.
From the Solidity doc:
If you can limit the length to a certain number of bytes, always use one of bytes1 to bytes32 because they are much cheaper.
Why do Solidity examples use bytes32 type instead of string?
bytes32 uses less gas because it fits in a single word of the EVM, and string is a dynamically sized-type which has current limitations in Solidity (such as can't be returned from a function to a contract).
If data can fit into 32 bytes, then you should use bytes32 datatype rather than bytes or strings as it is cheaper in solidity. Basically, any fixed size variable in solidity is cheaper than variable size. That will save gas on the contract.
Instances of string constant that can be replaced by `bytes5 constant`` :
adapters/yearn/YearnTokenAdapter.sol:14: string public constant override version = "2.1.0"; AlchemistV2.sol:54: string public constant override version = "2.2.6"; TransmuterBuffer.sol:37: string public constant override version = "2.2.0"; TransmuterV2.sol:105: string public constant override version = "2.2.0"; WETHGateway.sol:14: string public constant version = "2.1.0";
Reading array length at each iteration of the loop consumes more gas than necessary.
In the best case scenario (length read on a memory variable), caching the array length in the stack saves around 3 gas per iteration. In the worst case scenario (external calls at each iteration), the amount of gas wasted can be massive.
Notice that, in StakingPools.sol, the array-reading is made from an external call.
Here, I suggest storing the array's length in a variable before the for-loop, and use this new variable instead:
base/Multicall.sol:14: for (uint256 i = 0; i < data.length; i++) { AlchemistV2.sol:990: for (uint256 i = 0; i < depositedTokens.v@alues.length; i++) { AlchemistV2.sol:1282: for (uint256 i = 0; i < depositedTokens.values.length; i++) { AlchemistV2.sol:1355: for (uint256 i = 0; i < depositedTokens.values.length; i++) { AlchemistV2.sol:1461: for (uint256 i = 0; i < depositedTokens.values.length; i++) { AlchemistV2.sol:1524: for (uint256 i = 0; i < depositedTokens.values.length; i++) { CrossChainCanonicalBase.sol:57: for (uint256 i = 0; i < _bridgeTokens.length; i++){ CrossChainCanonicalBase.sol:141: for (uint i = 0; i < bridgeTokensArray.length; i++){ StakingPools.sol:188: for (uint256 _poolId = 0; _poolId < _pools.length(); _poolId++) { StakingPools.sol:363: for (uint256 _poolId = 0; _poolId < _pools.length(); _poolId++) { TransmuterBuffer.sol:172: for (uint256 i = 0; i < _yieldTokens[underlyingToken].length; i++) { TransmuterBuffer.sol:186: for (uint256 i = 0; i < tokens.length; i++) { TransmuterBuffer.sol:235: for (uint256 i = 0; i < registeredUnderlyings.length; i++) { TransmuterBuffer.sol:242: for (uint256 i = 0; i < registeredUnderlyings.length; i++) { TransmuterBuffer.sol:272: for (uint256 i = 0; i < registeredUnderlyings.length; i++) { TransmuterBuffer.sol:382: for (uint256 j = 0; j < registeredUnderlyings.length; j++) { TransmuterBuffer.sol:479: for (uint256 j = 0; j < weighting.tokens.length; j++) {
++i costs less gas compared to i++ or i += 1++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.
The same is also true for i--.
i++ increments i and returns the initial value of i. Which means:
uint i = 1; i++; // == 1 but i == 2
But ++i returns the actual incremented value:
uint i = 1; ++i; // == 2 and i == 2 too, so no need for a temporary variable
In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2
Instances include:
base/Multicall.sol:14: for (uint256 i = 0; i < data.length; i++) { libraries/Tick.sol:38: self.position++; AlchemistV2.sol:990: for (uint256 i = 0; i < depositedTokens.values.length; i++) { AlchemistV2.sol:1282: for (uint256 i = 0; i < depositedTokens.values.length; i++) { AlchemistV2.sol:1355: for (uint256 i = 0; i < depositedTokens.values.length; i++) { AlchemistV2.sol:1461: for (uint256 i = 0; i < depositedTokens.values.length; i++) { AlchemistV2.sol:1524: for (uint256 i = 0; i < depositedTokens.values.length; i++) { CrossChainCanonicalBase.sol:57: for (uint256 i = 0; i < _bridgeTokens.length; i++){ CrossChainCanonicalBase.sol:141: for (uint i = 0; i < bridgeTokensArray.length; i++){ EthAssetManager.sol:214: for (uint256 i = 0; i < NUM_META_COINS; i++) { EthAssetManager.sol:567: for (uint256 i = 0; i < NUM_META_COINS; i++) { StakingPools.sol:188: for (uint256 _poolId = 0; _poolId < _pools.length(); _poolId++) { StakingPools.sol:363: for (uint256 _poolId = 0; _poolId < _pools.length(); _poolId++) { ThreePoolAssetManager.sol:250: for (uint256 i = 0; i < NUM_STABLE_COINS; i++) { ThreePoolAssetManager.sol:254: for (uint256 i = 0; i < NUM_META_COINS; i++) { ThreePoolAssetManager.sol:353: for (uint256 i = 0; i < 256; i++) { ThreePoolAssetManager.sol:773: for (uint256 i = 0; i < NUM_STABLE_COINS; i++) { ThreePoolAssetManager.sol:902: for (uint256 i = 0; i < NUM_META_COINS; i++) { TransmuterBuffer.sol:172: for (uint256 i = 0; i < _yieldTokens[underlyingToken].length; i++) { TransmuterBuffer.sol:186: for (uint256 i = 0; i < tokens.length; i++) { TransmuterBuffer.sol:235: for (uint256 i = 0; i < registeredUnderlyings.length; i++) { TransmuterBuffer.sol:242: for (uint256 i = 0; i < registeredUnderlyings.length; i++) { TransmuterBuffer.sol:272: for (uint256 i = 0; i < registeredUnderlyings.length; i++) { TransmuterBuffer.sol:382: for (uint256 j = 0; j < registeredUnderlyings.length; j++) { TransmuterBuffer.sol:387: for (uint256 i = 0; i < numYTokens; i++) { TransmuterBuffer.sol:479: for (uint256 j = 0; j < weighting.tokens.length; j++) {
I suggest using ++i instead of i++ to increment the value of an uint variable.
In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.
Instances include:
base/Multicall.sol:14: for (uint256 i = 0; i < data.length; i++) { AlchemistV2.sol:990: for (uint256 i = 0; i < depositedTokens.values.length; i++) { AlchemistV2.sol:1282: for (uint256 i = 0; i < depositedTokens.values.length; i++) { AlchemistV2.sol:1355: for (uint256 i = 0; i < depositedTokens.values.length; i++) { AlchemistV2.sol:1461: for (uint256 i = 0; i < depositedTokens.values.length; i++) { AlchemistV2.sol:1524: for (uint256 i = 0; i < depositedTokens.values.length; i++) { CrossChainCanonicalBase.sol:57: for (uint256 i = 0; i < _bridgeTokens.length; i++){ CrossChainCanonicalBase.sol:141: for (uint i = 0; i < bridgeTokensArray.length; i++){ EthAssetManager.sol:214: for (uint256 i = 0; i < NUM_META_COINS; i++) { EthAssetManager.sol:567: for (uint256 i = 0; i < NUM_META_COINS; i++) { StakingPools.sol:188: for (uint256 _poolId = 0; _poolId < _pools.length(); _poolId++) { StakingPools.sol:363: for (uint256 _poolId = 0; _poolId < _pools.length(); _poolId++) { ThreePoolAssetManager.sol:250: for (uint256 i = 0; i < NUM_STABLE_COINS; i++) { ThreePoolAssetManager.sol:254: for (uint256 i = 0; i < NUM_META_COINS; i++) { ThreePoolAssetManager.sol:353: for (uint256 i = 0; i < 256; i++) { ThreePoolAssetManager.sol:773: for (uint256 i = 0; i < NUM_STABLE_COINS; i++) { ThreePoolAssetManager.sol:902: for (uint256 i = 0; i < NUM_META_COINS; i++) { TransmuterBuffer.sol:172: for (uint256 i = 0; i < _yieldTokens[underlyingToken].length; i++) { TransmuterBuffer.sol:186: for (uint256 i = 0; i < tokens.length; i++) { TransmuterBuffer.sol:235: for (uint256 i = 0; i < registeredUnderlyings.length; i++) { TransmuterBuffer.sol:242: for (uint256 i = 0; i < registeredUnderlyings.length; i++) { TransmuterBuffer.sol:272: for (uint256 i = 0; i < registeredUnderlyings.length; i++) { TransmuterBuffer.sol:382: for (uint256 j = 0; j < registeredUnderlyings.length; j++) { TransmuterBuffer.sol:387: for (uint256 i = 0; i < numYTokens; i++) { TransmuterBuffer.sol:479: for (uint256 j = 0; j < weighting.tokens.length; j++) {
The code would go from:
for (uint256 i; i < numIterations; i++) { // ... }
to:
for (uint256 i; i < numIterations;) { // ... unchecked { ++i; } }
The risk of overflow is inexistant for uint256 here.
If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.
As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {
Instances include:
base/Multicall.sol:14: for (uint256 i = 0; i < data.length; i++) { AlchemistV2.sol:990: for (uint256 i = 0; i < depositedTokens.values.length; i++) { AlchemistV2.sol:1282: for (uint256 i = 0; i < depositedTokens.values.length; i++) { AlchemistV2.sol:1355: for (uint256 i = 0; i < depositedTokens.values.length; i++) { AlchemistV2.sol:1458: uint256 totalValue = 0; AlchemistV2.sol:1461: for (uint256 i = 0; i < depositedTokens.values.length; i++) { AlchemistV2.sol:1524: for (uint256 i = 0; i < depositedTokens.values.length; i++) { CrossChainCanonicalBase.sol:57: for (uint256 i = 0; i < _bridgeTokens.length; i++){ CrossChainCanonicalBase.sol:141: for (uint i = 0; i < bridgeTokensArray.length; i++){ EthAssetManager.sol:214: for (uint256 i = 0; i < NUM_META_COINS; i++) { EthAssetManager.sol:566: uint256 total = 0; EthAssetManager.sol:567: for (uint256 i = 0; i < NUM_META_COINS; i++) { StakingPools.sol:188: for (uint256 _poolId = 0; _poolId < _pools.length(); _poolId++) { StakingPools.sol:363: for (uint256 _poolId = 0; _poolId < _pools.length(); _poolId++) { ThreePoolAssetManager.sol:250: for (uint256 i = 0; i < NUM_STABLE_COINS; i++) { ThreePoolAssetManager.sol:254: for (uint256 i = 0; i < NUM_META_COINS; i++) { ThreePoolAssetManager.sol:353: for (uint256 i = 0; i < 256; i++) { ThreePoolAssetManager.sol:771: uint256 normalizedTotal = 0; ThreePoolAssetManager.sol:773: for (uint256 i = 0; i < NUM_STABLE_COINS; i++) { ThreePoolAssetManager.sol:901: uint256 total = 0; ThreePoolAssetManager.sol:902: for (uint256 i = 0; i < NUM_META_COINS; i++) { TransmuterBuffer.sol:172: for (uint256 i = 0; i < _yieldTokens[underlyingToken].length; i++) { TransmuterBuffer.sol:186: for (uint256 i = 0; i < tokens.length; i++) { TransmuterBuffer.sol:235: for (uint256 i = 0; i < registeredUnderlyings.length; i++) { TransmuterBuffer.sol:242: for (uint256 i = 0; i < registeredUnderlyings.length; i++) { TransmuterBuffer.sol:272: for (uint256 i = 0; i < registeredUnderlyings.length; i++) { TransmuterBuffer.sol:382: for (uint256 j = 0; j < registeredUnderlyings.length; j++) { TransmuterBuffer.sol:387: for (uint256 i = 0; i < numYTokens; i++) { TransmuterBuffer.sol:479: for (uint256 j = 0; j < weighting.tokens.length; j++) { TransmuterBuffer.sol:534: uint256 want = 0; TransmuterBuffer.sol:549: uint256 exchangeDelta = 0;
I suggest removing explicit initializations for default values.
These variables are only set in the constructor and are never edited after that:
contracts-full/EthAssetManager.sol: 157: IWETH9 public weth; //@audit gas: should be immutable contracts-full/TransmuterConduit.sol: 13: address public token; //@audit gas: should be immutable 16: address public sourceTransmuter; //@audit gas: should be immutable 19: address public sinkTransmuter; //@audit gas: should be immutable contracts-full/WETHGateway.sol: 20: address public whitelist; //@audit gas: should be immutable
Consider marking them as immutable.
Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.
Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.
Revert strings > 32 bytes:
adapters/lido/WstETHAdapterV1.sol:77: revert Unauthorized("Payments only permitted from WETH or curve pool"); adapters/rocket/RETHAdapterV1.sol:54: revert Unauthorized("Payments only permitted from WETH or rETH"); libraries/RocketPool.sol:14: keccak256(abi.encodePacked("contract.address", "rocketTokenRETH")) AlchemicTokenV1.sol:51: require(whiteList[msg.sender], "AlTokenV1: Alchemist is not whitelisted"); AlchemicTokenV1.sol:81: require(total <= ceiling[msg.sender], "AlUSD: Alchemist's ceiling was breached."); AutoleverageCurveFactoryethpool.sol:23: if (msg.value != collateralInitial) revert IllegalArgument("msg.value doesn't match collateralInitial"); EthAssetManager.sol:19:} from "./interfaces/external/curve/IEthStableMetaPool.sol"; StakingPools.sol:106: require(_governance != address(0), "StakingPools: governance address cannot be 0x0"); StakingPools.sol:124: require(_pendingGovernance != address(0), "StakingPools: pending governance address cannot be 0x0"); StakingPools.sol:131: require(msg.sender == pendingGovernance, "StakingPools: only pending governance"); StakingPools.sol:160: require(tokenPoolIds[_token] == 0, "StakingPools: token already has a pool"); StakingPools.sol:183: require(_rewardWeights.length == _pools.length(), "StakingPools: weights length mismatch"); ThreePoolAssetManager.sol:19:} from "./interfaces/external/curve/IStableMetaPool.sol"; ThreePoolAssetManager.sol:24:} from "./interfaces/external/curve/IStableSwap3Pool.sol";
I suggest shortening the revert strings to fit in 32 bytes, or using custom errors as described next.
Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)
Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:
Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g.,
revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.
Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).
Instances include:
gALCX.sol:33: require(msg.sender == owner, "Not owner"); gALCX.sol:90: require(success, "Transfer failed"); gALCX.sol:107: require(success, "Transfer failed"); StakingPools.sol:106: require(_governance != address(0), "StakingPools: governance address cannot be 0x0"); StakingPools.sol:114: require(msg.sender == governance, "StakingPools: only governance"); StakingPools.sol:124: require(_pendingGovernance != address(0), "StakingPools: pending governance address cannot be 0x0"); StakingPools.sol:131: require(msg.sender == pendingGovernance, "StakingPools: only pending governance"); StakingPools.sol:160: require(tokenPoolIds[_token] == 0, "StakingPools: token already has a pool"); StakingPools.sol:183: require(_rewardWeights.length == _pools.length(), "StakingPools: weights length mismatch");
I suggest replacing require statements with custom errors.
#0 - 0xfoobar
2022-05-30T07:02:51Z
Great details, clean writeup