Platform: Code4rena
Start Date: 31/01/2023
Pot Size: $90,500 USDC
Total HM: 47
Participants: 169
Period: 7 days
Judge: LSDan
Total Solo HM: 9
Id: 211
League: ETH
Rank: 17/169
Findings: 1
Award: $1,330.24
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: c3phas
Also found by: 0xSmartContract, 0xackermann, 0xdaydream, Aymen0909, CodingNameKiki, Dewaxindo, Diana, IllIllI, Madalad, NoamYakov, Pheonix, Polaris_tow, ReyAdmirado, Rolezn, arialblack14, atharvasama, cryptostellar5, descharre, eyexploit, lukris02, saneryee
1330.2424 USDC - $1,330.24
keccak256() should only need to be called on a specific string literal onceNB: Some functions have been truncated where necessary to just show affected parts of the code Through out the report some places might be denoted with audit tags to show the actual place affected.
Use immutable if you want to assign a permanent value at construction. Use constants if you already know the permanent value. Both get directly embedded in bytecode, saving SLOAD. Variables only set in the constructor and never edited afterwards should be marked as immutable, as it would avoid the expensive storage-writing operation in the constructor (around 20 000 gas per variable) and replace the expensive storage-reading operations (around 2100 gas per reading) to a less expensive value reading (3 gas)
Gas Per variable: 2.1k
Total Instances: 8
Gas Saved: 8 * 2.1k=16.8k
File: /src/utils/MultiRewardEscrow.sol 191: address public feeRecipient;
diff --git a/src/utils/MultiRewardEscrow.sol b/src/utils/MultiRewardEscrow.sol index cf50b08..67744e0 100644 --- a/src/utils/MultiRewardEscrow.sol +++ b/src/utils/MultiRewardEscrow.sol @@ -188,7 +188,7 @@ contract MultiRewardEscrow is Owned { FEE LOGIC //////////////////////////////////////////////////////////////*/ - address public feeRecipient; + address public immutable feeRecipient;
File: /src/vault/DeploymentController.sol 23: ICloneFactory public cloneFactory; 24: ICloneRegistry public cloneRegistry; 25: ITemplateRegistry public templateRegistry;
File: /src/vault/VaultController.sol 535: IMultiRewardEscrow public escrow;
File: /src/vault/VaultController.sol 717: IAdminProxy public adminProxy;
File: /src/vault/VaultController.sol 387: IVaultRegistry public vaultRegistry;
File: /src/vault/VaultController.sol 822: IPermissionRegistry public permissionRegistry;
Since INITIAL_CHAIN_ID and INITIAL_DOMAIN_SEPARATOR are no longer immutable, but are state variables, you end up looking up their value every time, which incurs a very large gas penalty. It's cheaper to just calculate it every time.
File: /src/vault/Vault.sol 709: function DOMAIN_SEPARATOR() public view returns (bytes32) { 710: return 711: block.chainid == INITIAL_CHAIN_ID 712: ? INITIAL_DOMAIN_SEPARATOR 713: : computeDomainSeparator(); 714: }
File: /src/utils/MultiRewardStaking.sol 487: function DOMAIN_SEPARATOR() public view returns (bytes32) { 488: return block.chainid == INITIAL_CHAIN_ID ? INITIAL_DOMAIN_SEPARATOR : computeDomainSeparator(); 489: }
File: /src/vault/adapter/abstracts/AdapterBase.sol 677: function DOMAIN_SEPARATOR() public view virtual returns (bytes32) { 678: return 679: block.chainid == INITIAL_CHAIN_ID 680: ? INITIAL_DOMAIN_SEPARATOR 681: : computeDomainSeparator(); 682: }
Note the following lines and the explanation to understand the why and how the packing would be achieved
This packing is only achievable as we can reduce the size of time variable from uint256 to uint64 which should be safe as they are just timestamps
File: /src/vault/Vault.sol 468: uint256 public feesUpdatedAt; 508: uint256 public proposedFeeTime; 567: uint256 public proposedAdapterTime;
As feesUpdatedAt,proposedFeeTime,proposedAdapterTime are simply variables representing timestamps, we could get away with making them to be of type uint64 which should be safe for around 532 years
feesUpdatedAt with asset to save 1 SLOT (2.1k gas)For feesUpdatedAt we could pack it with IERC20 public asset; on Line 37
diff --git a/src/vault/Vault.sol b/src/vault/Vault.sol index 7a8f941..4bce208 100644 --- a/src/vault/Vault.sol +++ b/src/vault/Vault.sol @@ -35,6 +35,7 @@ contract Vault is uint256 constant SECONDS_PER_YEAR = 365.25 days; IERC20 public asset; + uint64 public feesUpdatedAt; uint8 internal _decimals; bytes32 public contractName; @@ -465,7 +466,6 @@ contract Vault is uint256 public highWaterMark = 1e18; uint256 public assetsCheckpoint; - uint256 public feesUpdatedAt;
proposedFeeTime with feeRecipient to save 1 SLOT(2.1k gas)Change proposedFeeTime to a uint64 and pack it with address feeRecipient on Line 510
diff --git a/src/vault/Vault.sol b/src/vault/Vault.sol index 7a8f941..a71094b 100644 --- a/src/vault/Vault.sol +++ b/src/vault/Vault.sol @@ -505,9 +505,9 @@ contract Vault is VaultFees public fees; VaultFees public proposedFees; - uint256 public proposedFeeTime; address public feeRecipient; + uint64 public proposedFeeTime;
proposedAdapterTime with proposedAdapter to save 1 SLOT(2.1k gas)Change proposedAdapterTime to a uint64 and pack it with IERC4626 proposedAdapter on Line 566
diff --git a/src/vault/Vault.sol b/src/vault/Vault.sol index 7a8f941..da2c9a4 100644 --- a/src/vault/Vault.sol +++ b/src/vault/Vault.sol @@ -564,7 +564,7 @@ contract Vault is IERC4626 public adapter; IERC4626 public proposedAdapter; - uint256 public proposedAdapterTime; + uint64 public proposedAdapterTime;
External calls are expensive. Consider caching the following:
File: /src/vault/adapter/yearn/YearnAdapter.sol 89: function _shareValue(uint256 yShares) internal view returns (uint256) { 90: if (yVault.totalSupply() == 0) return yShares; //@audit: initial call 92: return 93: yShares.mulDiv( 94: _freeFunds(), 95: yVault.totalSupply(),//@audit: 2nd call 96: Math.Rounding.Down 97: ); 98: }
diff --git a/src/vault/adapter/yearn/YearnAdapter.sol b/src/vault/adapter/yearn/YearnAdapter.sol index d951e63..12114a3 100644 --- a/src/vault/adapter/yearn/YearnAdapter.sol +++ b/src/vault/adapter/yearn/YearnAdapter.sol @@ -87,12 +87,13 @@ contract YearnAdapter is AdapterBase { /// @notice Determines the current value of `yShares` in assets function _shareValue(uint256 yShares) internal view returns (uint256) { - if (yVault.totalSupply() == 0) return yShares; + uint256 _yVaultTotalSupply = yVault.totalSupply(); + if (_yVaultTotalSupply == 0) return yShares; return yShares.mulDiv( _freeFunds(), - yVault.totalSupply(), + _yVaultTotalSupply, Math.Rounding.Down ); }
File: /src/vault/adapter/yearn/YearnAdapter.sol 34: function initialize( 47: _name = string.concat( 48: "Popcorn Yearn", 49: IERC20Metadata(asset()).name(),//@audit: 1st call 50: " Adapter" 51: ); 52: _symbol = string.concat("popY-", IERC20Metadata(asset()).symbol());//@audit: 2nd call
diff --git a/src/vault/adapter/yearn/YearnAdapter.sol b/src/vault/adapter/yearn/YearnAdapter.sol index d951e63..f69ad26 100644 --- a/src/vault/adapter/yearn/YearnAdapter.sol +++ b/src/vault/adapter/yearn/YearnAdapter.sol @@ -44,12 +44,14 @@ contract YearnAdapter is AdapterBase { yVault = VaultAPI(IYearnRegistry(externalRegistry).latestVault(_asset)); + address asset_ = asset(); + _name = string.concat( "Popcorn Yearn", - IERC20Metadata(asset()).name(), + IERC20Metadata(asset_).name(), " Adapter" ); - _symbol = string.concat("popY-", IERC20Metadata(asset()).symbol()); + _symbol = string.concat("popY-", IERC20Metadata(asset_).symbol()); IERC20(_asset).approve(address(yVault), type(uint256).max); }
File: /src/vault/adapter/beefy/BeefyAdapter.sol 46: function initialize( 47: bytes memory adapterInitData, 48: address registry, 49: bytes memory beefyInitData 50: ) external initializer { 59: if (IBeefyVault(_beefyVault).want() != asset()) //@audit: 1st call 60: revert InvalidBeefyVault(_beefyVault); 66: _name = string.concat( 67: "Popcorn Beefy", 68: IERC20Metadata(asset()).name(), //@audit: 2nd call 69: " Adapter" 70: ); 71: _symbol = string.concat("popB-", IERC20Metadata(asset()).symbol());//@audit: 3rd call 80: IERC20(asset()).approve(_beefyVault, type(uint256).max);//@audit: 4th call
File: /src/vault/Vault.sol 447: function accruedPerformanceFee() public view returns (uint256) { 448: uint256 highWaterMark_ = highWaterMark; 449: uint256 shareValue = convertToAssets(1e18); 450: uint256 performanceFee = fees.performance; 452: return 453: performanceFee > 0 && shareValue > highWaterMark 454: ? performanceFee.mulDiv( 455: (shareValue - highWaterMark) * totalSupply(), 456: 1e36, 457: Math.Rounding.Down 458: ) 459: : 0; 460: }
diff --git a/src/vault/Vault.sol b/src/vault/Vault.sol index 7a8f941..a977451 100644 --- a/src/vault/Vault.sol +++ b/src/vault/Vault.sol @@ -450,9 +450,9 @@ contract Vault is uint256 performanceFee = fees.performance; return - performanceFee > 0 && shareValue > highWaterMark + performanceFee > 0 && shareValue > highWaterMark_ ? performanceFee.mulDiv( - (shareValue - highWaterMark) * totalSupply(), + (shareValue - highWaterMark_) * totalSupply(), 1e36, Math.Rounding.Down )
File: /src/vault/Vault.sol 57: function initialize( 77: asset = asset_; //@audit: asset cached here 80: asset.approve(address(adapter_), type(uint256).max);//@audit: use cached value here
diff --git a/src/vault/Vault.sol b/src/vault/Vault.sol index 7a8f941..0addeb4 100644 --- a/src/vault/Vault.sol +++ b/src/vault/Vault.sol @@ -77,7 +77,7 @@ contract Vault is asset = asset_; adapter = adapter_; - asset.approve(address(adapter_), type(uint256).max); + asset_.approve(address(adapter_), type(uint256).max);
Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
Affected code:
File: /src/vault/adapter/yearn/YearnAdapter.sol 89: function _shareValue(uint256 yShares) internal view returns (uint256) { 101: function _freeFunds() internal view returns (uint256) { 109: function _yTotalAssets() internal view returns (uint256) { 114: function _calculateLockedProfit() internal view returns (uint256) {
File: /src/vault/VaultController.sol 120: function _deployVault(VaultInitParams memory vaultData, IDeploymentController _deploymentController) 121: internal 122: returns (address vault) 123: { 141: function _registerCreatedVault( 142: address vault, 143: address staking, 144: VaultMetadata memory metadata 145: ) internal { 154: function _handleVaultStakingRewards(address vault, bytes memory rewardsData) internal { 225: function __deployAdapter( 226: DeploymentArgs memory adapterData, 227: bytes memory baseAdapterData, 228: IDeploymentController _deploymentController 229: ) internal returns (address adapter) { 242: function _encodeAdapterData(DeploymentArgs memory adapterData, bytes memory baseAdapterData) 243: internal 244: returns (bytes memory) 245: { 256: function _deployStrategy(DeploymentArgs memory strategyData, IDeploymentController _deploymentController) 257: internal 258: returns (address strategy) 259: { 390: function _registerVault(address vault, VaultMetadata memory metadata) internal { 692: function _verifyAdapterConfiguration(address adapter, bytes32 adapterId) internal view {
File: /src/utils/MultiRewardStaking.sol 191: function _lockToken( 192: address user, 193: IERC20 rewardToken, 194: uint256 rewardAmount, 195: EscrowInfo memory escrowInfo 196: ) internal {
Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times saves ~42 gas per access due to not having to perform the same offset calculation every time. Help the Optimizer by saving a storage variable's reference instead of repeatedly fetching it
To help the optimizer,declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array.
As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex] and use it.
File: /src/vault/TemplateRegistry.sol 52: function addTemplateCategory(bytes32 templateCategory) external onlyOwner { 53: if (templateCategoryExists[templateCategory]) revert TemplateCategoryExists(templateCategory); 55: templateCategoryExists[templateCategory] = true; 56: templateCategories.push(templateCategory); 58: emit TemplateCategoryAdded(templateCategory); 59: }
File: /src/utils/MultiRewardStaking.sol 402: function _accrueRewards(IERC20 _rewardToken, uint256 accrued) internal { 403: uint256 supplyTokens = totalSupply(); 404: uint224 deltaIndex; 405: if (supplyTokens != 0) 406: deltaIndex = accrued.mulDiv(uint256(10**decimals()), supplyTokens, Math.Rounding.Down).safeCastTo224(); 408: rewardInfos[_rewardToken].index += deltaIndex; 409: rewardInfos[_rewardToken].lastUpdatedTimestamp = block.timestamp.safeCastTo32(); 410: }
Here, the values emitted shouldn’t be read from storage. The existing memory values should be used instead:
_quitPeriod : saves 1 SLOAD: ~100 gasFile: /src/vault/Vault.sol 629: function setQuitPeriod(uint256 _quitPeriod) external onlyOwner { 630: if (_quitPeriod < 1 days || _quitPeriod > 7 days) 631: revert InvalidQuitPeriod(); 633: quitPeriod = _quitPeriod; 635: emit QuitPeriodSet(quitPeriod); 636: }
diff --git a/src/vault/Vault.sol b/src/vault/Vault.sol index 7a8f941..302ba9f 100644 --- a/src/vault/Vault.sol +++ b/src/vault/Vault.sol @@ -632,7 +632,7 @@ contract Vault is quitPeriod = _quitPeriod; - emit QuitPeriodSet(quitPeriod); + emit QuitPeriodSet(_quitPeriod); }
File: /src/vault/Vault.sol 57: function initialize( 77: asset = asset_; //@audit: asset cached here 97: emit VaultInitialized(contractName, address(asset));
diff --git a/src/vault/Vault.sol b/src/vault/Vault.sol index 7a8f941..8562995 100644 --- a/src/vault/Vault.sol +++ b/src/vault/Vault.sol @@ -94,7 +94,7 @@ contract Vault is abi.encodePacked("Popcorn", name(), block.timestamp, "Vault") ); - emit VaultInitialized(contractName, address(asset)); + emit VaultInitialized(contractName, address(asset_)); }
Saves 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.
File: /src/vault/TemplateRegistry.sol 33: mapping(bytes32 => bool) public templateExists; 35: mapping(bytes32 => bool) public templateCategoryExists;
File: /src/utils/MultiRewardStaking.sol 216: mapping(address => mapping(IERC20 => uint256)) public userIndex; 218: mapping(address => mapping(IERC20 => uint256)) public accruedRewards;
When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct
File: /src/utils/MultiRewardEscrow.sol 157: Escrow memory escrow = escrows[escrowId];
diff --git a/src/utils/MultiRewardEscrow.sol b/src/utils/MultiRewardEscrow.sol index cf50b08..6ee7fe9 100644 --- a/src/utils/MultiRewardEscrow.sol +++ b/src/utils/MultiRewardEscrow.sol @@ -154,13 +154,13 @@ contract MultiRewardEscrow is Owned { function claimRewards(bytes32[] memory escrowIds) external { for (uint256 i = 0; i < escrowIds.length; i++) { bytes32 escrowId = escrowIds[i]; - Escrow memory escrow = escrows[escrowId]; + Escrow storage escrow = escrows[escrowId]; uint256 claimable = _getClaimableAmount(escrow); if (claimable == 0) revert NotClaimable(escrowId); - escrows[escrowId].balance -= claimable; - escrows[escrowId].lastUpdateTime = block.timestamp.safeCastTo32(); + escrow.balance -= claimable; + escrow.lastUpdateTime = block.timestamp.safeCastTo32(); escrow.token.safeTransfer(escrow.account, claimable); emit RewardsClaimed(escrow.token, escrow.account, claimable);
File: /src/utils/MultiRewardStaking.sol 254: RewardInfo memory rewards = rewardInfos[rewardToken];
As rewards now points to the storage variable we can just use it to effect changes to the storage one, this way we no longer need to write to rewardInfos[rewardToken]; at the end as writing to rewards would have a similar impact
diff --git a/src/utils/MultiRewardStaking.sol b/src/utils/MultiRewardStaking.sol index 95ebefd..2cf3289 100644 --- a/src/utils/MultiRewardStaking.sol +++ b/src/utils/MultiRewardStaking.sol @@ -251,7 +251,7 @@ contract MultiRewardStaking is ERC4626Upgradeable, OwnedUpgradeable { ) external onlyOwner { if (asset() == address(rewardToken)) revert RewardTokenCantBeStakingToken(); - RewardInfo memory rewards = rewardInfos[rewardToken]; + RewardInfo storage rewards = rewardInfos[rewardToken]; if (rewards.lastUpdatedTimestamp > 0) revert RewardTokenAlreadyExist(rewardToken); if (amount > 0) { @@ -276,7 +276,7 @@ contract MultiRewardStaking is ERC4626Upgradeable, OwnedUpgradeable { ? block.timestamp.safeCastTo32() : _calcRewardsEnd(0, rewardsPerSecond, amount); - rewardInfos[rewardToken] = RewardInfo({ + rewards = RewardInfo({ ONE: ONE, rewardsPerSecond: rewardsPerSecond, rewardsEndTimestamp: rewardsEndTimestamp,
File: /src/utils/MultiRewardStaking.sol 297: RewardInfo memory rewards = rewardInfos[rewardToken];
Note the changes done at the last lines. We no longer need to asign the variable at the end as we are not dealing with a copy of the variable but a reference to the storage variable, thus any changes to our locally cached storage variable would change the actual variable stored in storage
diff --git a/src/utils/MultiRewardStaking.sol b/src/utils/MultiRewardStaking.sol index 95ebefd..b0ce54a 100644 --- a/src/utils/MultiRewardStaking.sol +++ b/src/utils/MultiRewardStaking.sol @@ -294,7 +294,7 @@ contract MultiRewardStaking is ERC4626Upgradeable, OwnedUpgradeable { * @dev The `rewardsEndTimestamp` gets calculated based on `rewardsPerSecond` and `amount`. */ function changeRewardSpeed(IERC20 rewardToken, uint160 rewardsPerSecond) external onlyOwner { - RewardInfo memory rewards = rewardInfos[rewardToken]; + RewardInfo storage rewards = rewardInfos[rewardToken]; if (rewardsPerSecond == 0) revert ZeroAmount(); if (rewards.lastUpdatedTimestamp == 0) revert RewardTokenDoesntExist(rewardToken); @@ -310,8 +310,8 @@ contract MultiRewardStaking is ERC4626Upgradeable, OwnedUpgradeable { rewardsPerSecond, remainder ); - rewardInfos[rewardToken].rewardsPerSecond = rewardsPerSecond; - rewardInfos[rewardToken].rewardsEndTimestamp = rewardsEndTimestamp; + rewards.rewardsPerSecond = rewardsPerSecond; + rewards.rewardsEndTimestamp = rewardsEndTimestamp; }
File: /src/utils/MultiRewardStaking.sol 328: RewardInfo memory rewards = rewardInfos[rewardToken];
File: /src/utils/MultiRewardStaking.sol 414: RewardInfo memory rewards = rewardInfos[_rewardToken];
Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas) in a function that may ultimately revert in the unhappy case.
File: /src/utils/MultiRewardStaking.sol 243: function addRewardToken( 244: IERC20 rewardToken, 245: uint160 rewardsPerSecond, 246: uint256 amount, 247: bool useEscrow, 248: uint192 escrowPercentage, 249: uint32 escrowDuration, 250: uint32 offset 251: ) external onlyOwner { 252: if (asset() == address(rewardToken)) revert RewardTokenCantBeStakingToken(); 254: RewardInfo memory rewards = rewardInfos[rewardToken]; 255: if (rewards.lastUpdatedTimestamp > 0) revert RewardTokenAlreadyExist(rewardToken); 257: if (amount > 0) { 258: if (rewardsPerSecond == 0) revert ZeroRewardsSpeed(); 259: rewardToken.safeTransferFrom(msg.sender, address(this), amount); 260: }
The first check involves a functional call which is expensive. As we also revert on a functional parameter (rewardsPerSecond) failure to pass a certain check (in this case , we revert if it's equal to zero) it would be way cheaper to reorder the if's cheak to have the cheap check for functional parameter before performing the more expensive ones.
diff --git a/src/utils/MultiRewardStaking.sol b/src/utils/MultiRewardStaking.sol index 95ebefd..b1723a0 100644 --- a/src/utils/MultiRewardStaking.sol +++ b/src/utils/MultiRewardStaking.sol @@ -249,13 +249,13 @@ contract MultiRewardStaking is ERC4626Upgradeable, OwnedUpgradeable { uint32 escrowDuration, uint32 offset ) external onlyOwner { + if (rewardsPerSecond == 0) revert ZeroRewardsSpeed(); if (asset() == address(rewardToken)) revert RewardTokenCantBeStakingToken(); RewardInfo memory rewards = rewardInfos[rewardToken]; if (rewards.lastUpdatedTimestamp > 0) revert RewardTokenAlreadyExist(rewardToken); if (amount > 0) { - if (rewardsPerSecond == 0) revert ZeroRewardsSpeed(); rewardToken.safeTransferFrom(msg.sender, address(this), amount); }
File: /src/utils/MultiRewardStaking.sol 296: function changeRewardSpeed(IERC20 rewardToken, uint160 rewardsPerSecond) external onlyOwner { 297: RewardInfo memory rewards = rewardInfos[rewardToken]; 299: if (rewardsPerSecond == 0) revert ZeroAmount(); 300: if (rewards.lastUpdatedTimestamp == 0) revert RewardTokenDoesntExist(rewardToken); 301: if (rewards.rewardsPerSecond == 0) revert RewardsAreDynamic(rewardToken); 303: _accrueRewards(rewardToken, _accrueStatic(rewards));
As we have a check for a functional parameter, it is better to do that check first before anything else. This way we don't waste any gas if the function parameter does not the required condition . In the above function we shouldn't waste gas on RewardInfo memory rewards = rewardInfos[rewardToken]; if we could end up reverting on if (rewardsPerSecond == 0) revert ZeroAmount();
diff --git a/src/utils/MultiRewardStaking.sol b/src/utils/MultiRewardStaking.sol index 95ebefd..bb98e33 100644 --- a/src/utils/MultiRewardStaking.sol +++ b/src/utils/MultiRewardStaking.sol @@ -294,9 +294,10 @@ contract MultiRewardStaking is ERC4626Upgradeable, OwnedUpgradeable { * @dev The `rewardsEndTimestamp` gets calculated based on `rewardsPerSecond` and `amount`. */ function changeRewardSpeed(IERC20 rewardToken, uint160 rewardsPerSecond) external onlyOwner { + if (rewardsPerSecond == 0) revert ZeroAmount(); + RewardInfo memory rewards = rewardInfos[rewardToken]; - if (rewardsPerSecond == 0) revert ZeroAmount(); if (rewards.lastUpdatedTimestamp == 0) revert RewardTokenDoesntExist(rewardToken); if (rewards.rewardsPerSecond == 0) revert RewardsAreDynamic(rewardToken);
File: /src/vault/Vault.sol 57: function initialize( 58: IERC20 asset_, 59: IERC4626 adapter_, 60: VaultFees calldata fees_, 61: address feeRecipient_, 62: address owner 63: ) external initializer { 64: __ERC20_init( 65: string.concat( 66: "Popcorn ", 67: IERC20Metadata(address(asset_)).name(), 68: " Vault" 69: ), 70: string.concat("pop-", IERC20Metadata(address(asset_)).symbol()) 71: ); 72: __Owned_init(owner); 74: if (address(asset_) == address(0)) revert InvalidAsset(); 75: if (address(asset_) != adapter_.asset()) revert InvalidAdapter(); 77: asset = asset_; 78: adapter = adapter_; 80: asset.approve(address(adapter_), type(uint256).max); 82: _decimals = IERC20Metadata(address(asset_)).decimals(); 84: INITIAL_CHAIN_ID = block.chainid; 85: INITIAL_DOMAIN_SEPARATOR = computeDomainSeparator(); 87: feesUpdatedAt = block.timestamp; 88: fees = fees_; 90: if (feeRecipient_ == address(0)) revert InvalidFeeRecipient();
As feeRecipient_ is a functional parameter, we should check it first before performing other operations to prevent wasting gas if we will ultimately revert due to feeRecipient_ not passing the required checks.
diff --git a/src/vault/Vault.sol b/src/vault/Vault.sol index 7a8f941..10d6b1b 100644 --- a/src/vault/Vault.sol +++ b/src/vault/Vault.sol @@ -71,6 +71,7 @@ contract Vault is ); __Owned_init(owner); + if (feeRecipient_ == address(0)) revert InvalidFeeRecipient(); if (address(asset_) == address(0)) revert InvalidAsset(); if (address(asset_) != adapter_.asset()) revert InvalidAdapter(); @@ -87,7 +88,6 @@ contract Vault is feesUpdatedAt = block.timestamp; fees = fees_; - if (feeRecipient_ == address(0)) revert InvalidFeeRecipient(); feeRecipient = feeRecipient_; contractName = keccak256(
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
File: /src/utils/MultiRewardStaking.sol 466: keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"),
File: /src/utils/MultiRewardStaking.sol 495: keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
File: /src/utils/MultiRewardStaking.sol 497:
File: /src/vault/Vault.sol 685: keccak256( 686: "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)" 687: ),
File: /src/vault/Vault.sol 720: keccak256( 721: "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)" 722: ),
File: /src/vault/Vault.sol 724: keccak256("1"),
File: /src/vault/adapter/abstracts/AdapterBase.sol 653: keccak256( 654: "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)" 655: ), 688: keccak256( 689: "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)" 690: ), 692: keccak256("1"),
File: /src/vault/adapter/abstracts/AdapterBase.sol 158: underlyingBalance += _underlyingBalance() - underlyingBalance_;
- underlyingBalance += _underlyingBalance() - underlyingBalance_; + underlyingBalance = underlyingBalance + _underlyingBalance() - underlyingBalance_;
File: /src/vault/adapter/abstracts/AdapterBase.sol 225: underlyingBalance -= underlyingBalance_ - _underlyingBalance();
When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed
File: /src/utils/MultiRewardEscrow.sol //@audit: uint32 duration, uint32 offset 88: function lock( 89: IERC20 token, 90: address account, 91: uint256 amount, 92: uint32 duration, 93: uint32 offset 94: ) external {
File: /src/utils/MultiRewardStaking.sol //@audit: uint160 rewardsPerSecond,uint192 escrowPercentage,uint32 escrowDuration, uint32 offset 243: function addRewardToken( 244: IERC20 rewardToken, 245: uint160 rewardsPerSecond, 246: uint256 amount, 247: bool useEscrow, 248: uint192 escrowPercentage, 249: uint32 escrowDuration, 250: uint32 offset 251: ) external onlyOwner { //@audit: uint160 rewardsPerSecond 296: function changeRewardSpeed(IERC20 rewardToken, uint160 rewardsPerSecond) external onlyOwner { //@audit: uint32 rewardsEndTimestamp, uint160 rewardsPerSecond, 351: function _calcRewardsEnd( 352: uint32 rewardsEndTimestamp, 353: uint160 rewardsPerSecond, 354: uint256 amount 355: ) internal returns (uint32) {
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 see resource
File: /src/vault/adapter/yearn/YearnAdapter.sol 151: return _depositLimit - assets;
The operation _depositLimit - assets cannot underflow due to the check on Line 150 that ensures that _depositLimit is greater than assets before performing the arithmetic operation
File: /src/utils/MultiRewardStaking.sol 357: amount += uint256(rewardsPerSecond) * (rewardsEndTimestamp - block.timestamp);
The operation rewardsEndTimestamp - block.timestamp cannot underflow due to the check on Line 356 that ensures that rewardsEndTimestamp is greater than block.timestamp
File: /src/utils/MultiRewardStaking.sol 395: elapsed = rewards.rewardsEndTimestamp - rewards.lastUpdatedTimestamp;
The operation rewards.rewardsEndTimestamp - rewards.lastUpdatedTimestamp cannot underflow due to the check on Line 394 that ensures that rewards.rewardsEndTimestamp is greater than rewards.lastUpdatedTimestamp before perfoming the arithmetic operation
File: /src/vault/Vault.sol 455: (shareValue - highWaterMark) * totalSupply(),
The operation shareValue - highWaterMark cannot underflow as this operation would only be performed if shareValue is greater than highWaterMark due to the condition check on Line 453
File: /src/vault/adapter/abstracts/AdapterBase.sol 537: (shareValue - highWaterMark_) * totalSupply(),
The operation shareValue - highWaterMark_ cannot underflow due to the check on Line 535 that ensures that shareValue is greater than highWaterMark_ before performing the arithmetic operation
The majority of Solidity for loops increment a uint256 variable that starts at 0. These increment operations never need to be checked for over/underflow because the variable will never reach the max number of uint256 (will run out of gas long before that happens). The default over/underflow check wastes gas in every iteration of virtually every for loop.
File: /src/vault/PermissionRegistry.sol 42: for (uint256 i = 0; i < len; i++) {
The above should be modified to:
diff --git a/src/vault/PermissionRegistry.sol b/src/vault/PermissionRegistry.sol index 1a61e1c..fa1c7bb 100644 --- a/src/vault/PermissionRegistry.sol +++ b/src/vault/PermissionRegistry.sol @@ -39,12 +39,15 @@ contract PermissionRegistry is Owned { uint256 len = targets.length; if (len != newPermissions.length) revert Mismatch(); - for (uint256 i = 0; i < len; i++) { + for (uint256 i = 0; i < len;) { if (newPermissions[i].endorsed && newPermissions[i].rejected) revert Mismatch(); emit PermissionSet(targets[i], newPermissions[i].endorsed, newPermissions[i].rejected); permissions[targets[i]] = newPermissions[i]; + unchecked { + ++i; + } }
Other Instances to modify
File: /src/utils/MultiRewardEscrow.sol 53: for (uint256 i = 0; i < escrowIds.length; i++) { 155: for (uint256 i = 0; i < escrowIds.length; i++) { 210: for (uint256 i = 0; i < tokens.length; i++) {
File: /src/utils/MultiRewardStaking.sol 171: for (uint8 i; i < _rewardTokens.length; i++) { 373: for (uint8 i; i < _rewardTokens.length; i++) {
File: /src/vault/VaultController.sol 319: for (uint8 i = 0; i < len; i++) { 337: for (uint8 i = 0; i < len; i++) { 357: for (uint8 i = 0; i < len; i++) { 374: for (uint8 i = 0; i < len; i++) { 437: for (uint256 i = 0; i < len; i++) { 494: for (uint256 i = 0; i < len; i++) { 523: for (uint256 i = 0; i < len; i++) { 564: for (uint256 i = 0; i < len; i++) { 587: for (uint256 i = 0; i < len; i++) { 607: for (uint256 i = 0; i < len; i++) { 620: for (uint256 i = 0; i < len; i++) { 633: for (uint256 i = 0; i < len; i++) { 646: for (uint256 i = 0; i < len; i++) { 766: for (uint256 i = 0; i < len; i++) { 806: for (uint256 i = 0; i < len; i++) {
#0 - c4-judge
2023-02-28T17:22:07Z
dmvt marked the issue as grade-a
#1 - c4-judge
2023-03-01T22:35:13Z
dmvt marked the issue as selected for report