Popcorn contest - arialblack14's results

QA REPORT

---

Summary of low risk issues

Note: The instances of issue with id L-4 are not listed in the c4udit output.

Total: 4 issues.

Summary of non-critical issues

Total: 6 issues.

---

Low Risk Issues

<a id=L1>[L-1]</a> Use of block.timestamp

Description

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Recommendation

Use oracle instead of block.timestamp

Instances (30):

File: 2023-01-popcorn/src/utils/MultiRewardEscrow.sol

114: uint32 start = block.timestamp.safeCastTo32() + offset;
163: escrows[escrowId].lastUpdateTime = block.timestamp.safeCastTo32();
175: block.timestamp.safeCastTo32() < escrow.start
181: (escrow.balance * (block.timestamp - uint256(escrow.lastUpdateTime))) /

File: 2023-01-popcorn/src/utils/MultiRewardStaking.sol

276: ? block.timestamp.safeCastTo32()
284: lastUpdatedTimestamp: block.timestamp.safeCastTo32()
309: prevEndTime > block.timestamp ? prevEndTime : block.timestamp.safeCastTo32(),
346: rewardInfos[rewardToken].lastUpdatedTimestamp = block.timestamp.safeCastTo32();
356: if (rewardsEndTimestamp > block.timestamp)
357: amount += uint256(rewardsPerSecond) * (rewardsEndTimestamp - block.timestamp);
359: return (block.timestamp + (amount / uint256(rewardsPerSecond))).safeCastTo32();
392: if (rewards.rewardsEndTimestamp > block.timestamp) {
393: elapsed = block.timestamp - rewards.lastUpdatedTimestamp;
409: rewardInfos[_rewardToken].lastUpdatedTimestamp = block.timestamp.safeCastTo32();
454: if (deadline < block.timestamp) revert PermitDeadlineExpired(deadline);

File: 2023-01-popcorn/src/vault/Vault.sol

87: feesUpdatedAt = block.timestamp;
94: abi.encodePacked("Popcorn", name(), block.timestamp, "Vault")
434: totalAssets() * (block.timestamp - feesUpdatedAt),
488: if (managementFee > 0) feesUpdatedAt = block.timestamp;
534: proposedFeeTime = block.timestamp;
536: emit NewFeesProposed(newFees, block.timestamp);
541: if (block.timestamp < proposedFeeTime + quitPeriod)
583: proposedAdapterTime = block.timestamp;
585: emit NewAdapterProposed(newAdapter, block.timestamp);
595: if (block.timestamp < proposedAdapterTime + quitPeriod)
673: if (deadline < block.timestamp) revert PermitDeadlineExpired(deadline);

File: 2023-01-popcorn/src/vault/adapter/abstracts/AdapterBase.sol

86: lastHarvest = block.timestamp;
441: ((lastHarvest + harvestCooldown) < block.timestamp)
641: if (deadline < block.timestamp) revert PermitDeadlineExpired(deadline);

File: 2023-01-popcorn/src/vault/adapter/yearn/YearnAdapter.sol

115: uint256 lockedFundsRatio = (block.timestamp - yVault.lastReport()) *

<a id=L5>[L-2]</a> Use call() instead of transfer()

Description

The transfer() function only allows the recipient to use 2300 gas. If the recipient uses more than that, transfers will fail. In the future gas costs might change increasing the likelihood of that happening. Keep in mind that call() introduces the risk of reentrancy, but as the code follows the CEI pattern it should be fine.

Recommendation

Replace transfer() calls with call(), keep in mind to check whether the call was successful by validating the return value.

Instances (1):

File: 2023-01-popcorn/src/utils/MultiRewardStaking.sol

182: _rewardTokens[i].transfer(user, rewardAmount);

<a id=L6>[L-3]</a> Lock pragmas to specific compiler version.

Description

Pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or EthPM package. Otherwise, the developer would need to manually update the pragma in order to compile locally.

Recommendation

Ethereum Smart Contract Best Practices - Lock pragmas to specific compiler version. solidity-specific/locking-pragmas

Instances (35):

File: 2023-01-popcorn/src/interfaces/IEIP165.sol

2: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/interfaces/IMultiRewardEscrow.sol

3: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/interfaces/IMultiRewardStaking.sol

3: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/interfaces/vault/IAdapter.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/interfaces/vault/IAdminProxy.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/interfaces/vault/ICloneFactory.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/interfaces/vault/ICloneRegistry.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/interfaces/vault/IDeploymentController.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/interfaces/vault/IERC4626.sol

3: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/interfaces/vault/IPermissionRegistry.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/interfaces/vault/IStrategy.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/interfaces/vault/ITemplateRegistry.sol

3: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/interfaces/vault/IVault.sol

3: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/interfaces/vault/IVaultController.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/interfaces/vault/IVaultRegistry.sol

3: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/interfaces/vault/IWithRewards.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/utils/EIP165.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/utils/MultiRewardEscrow.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/utils/MultiRewardStaking.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/vault/AdminProxy.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/vault/CloneFactory.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/vault/CloneRegistry.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/vault/DeploymentController.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/vault/PermissionRegistry.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/vault/TemplateRegistry.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/vault/Vault.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/vault/VaultController.sol

3: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/vault/VaultRegistry.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/vault/adapter/abstracts/AdapterBase.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/vault/adapter/abstracts/OnlyStrategy.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/vault/adapter/abstracts/WithRewards.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/vault/adapter/beefy/BeefyAdapter.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/vault/adapter/beefy/IBeefy.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/vault/adapter/yearn/IYearn.sol

4: pragma solidity ^0.8.15;

File: 2023-01-popcorn/src/vault/adapter/yearn/YearnAdapter.sol

4: pragma solidity ^0.8.15;

<a id=L8>[L-4]</a> Empty blocks should be removed or emit something.

Description

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified solidity if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}}

Recommendation

Empty blocks should be removed or emit something (The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting.

Instances (3):

File: 2023-01-popcorn/src/vault/adapter/abstracts/AdapterBase.sol

258: function _totalAssets() internal view virtual returns (uint256) {}
265: function _underlyingBalance() internal view virtual returns (uint256) {}
276: {}

Non-critical Issues

<a id=NC3>[NC-1]</a> Missing timelock for critical changes.

Description

A timelock should be added to functions that perform critical changes.

Recommendation

TODO: Add Timelock for the following functions.

Instances (31):

File: 2023-01-popcorn/src/utils/MultiRewardEscrow.sol

207: function setFees(IERC20[] memory tokens, uint256[] memory tokenFees) external onlyOwner {

File: 2023-01-popcorn/src/utils/MultiRewardStaking.sol

251: ) external onlyOwner {
296: function changeRewardSpeed(IERC20 rewardToken, uint160 rewardsPerSecond) external onlyOwner {

File: 2023-01-popcorn/src/vault/CloneRegistry.sol

45: ) external onlyOwner {

File: 2023-01-popcorn/src/vault/DeploymentController.sol

55: function addTemplateCategory(bytes32 templateCategory) external onlyOwner {
80: function toggleTemplateEndorsement(bytes32 templateCategory, bytes32 templateId) external onlyOwner {
121: function nominateNewDependencyOwner(address _owner) external onlyOwner {

File: 2023-01-popcorn/src/vault/PermissionRegistry.sol

38: function setPermissions(address[] calldata targets, Permission[] calldata newPermissions) external onlyOwner {

File: 2023-01-popcorn/src/vault/TemplateRegistry.sol

52: function addTemplateCategory(bytes32 templateCategory) external onlyOwner {
71: ) external onlyOwner {
102: function toggleTemplateEndorsement(bytes32 templateCategory, bytes32 templateId) external onlyOwner {

File: 2023-01-popcorn/src/vault/Vault.sol

525: function proposeFees(VaultFees calldata newFees) external onlyOwner {
553: function setFeeRecipient(address _feeRecipient) external onlyOwner {
578: function proposeAdapter(IERC4626 newAdapter) external onlyOwner {
629: function setQuitPeriod(uint256 _quitPeriod) external onlyOwner {
643: function pause() external onlyOwner {
648: function unpause() external onlyOwner {

File: 2023-01-popcorn/src/vault/VaultController.sol

408: function setPermissions(address[] calldata targets, Permission[] calldata newPermissions) external onlyOwner {
543: function setEscrowTokenFees(IERC20[] calldata tokens, uint256[] calldata fees) external onlyOwner {
561: function addTemplateCategories(bytes32[] calldata templateCategories) external onlyOwner {
723: function nominateNewAdminProxyOwner(address newOwner) external onlyOwner {
751: function setPerformanceFee(uint256 newFee) external onlyOwner {
764: function setAdapterPerformanceFees(address[] calldata adapters) external onlyOwner {
791: function setHarvestCooldown(uint256 newCooldown) external onlyOwner {
804: function setAdapterHarvestCooldowns(address[] calldata adapters) external onlyOwner {
832: function setDeploymentController(IDeploymentController _deploymentController) external onlyOwner {
864: function setActiveTemplateId(bytes32 templateCategory, bytes32 templateId) external onlyOwner {

File: 2023-01-popcorn/src/vault/VaultRegistry.sol

44: function registerVault(VaultMetadata calldata _metadata) external onlyOwner {

File: 2023-01-popcorn/src/vault/adapter/abstracts/AdapterBase.sol

500: function setHarvestCooldown(uint256 newCooldown) external onlyOwner {
574: function pause() external onlyOwner {
582: function unpause() external onlyOwner {

<a id=NC5>[NC-2]</a> Use scientific notation (e.g.1e18) rather than exponentiation (e.g.10**18).

Description

Use scientific notation (e.g.1e18) rather than exponentiation (e.g.10**18) that could lead to errors.

Recommendation

Use scientific notation instead of exponentiation.

Instances (1):

File: 2023-01-popcorn/src/vault/adapter/yearn/YearnAdapter.sol

25: uint256 constant DEGRADATION_COEFFICIENT = 10**18;

<a id=NC6>[NC-3]</a> Lines are too long.

Description

Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length

Recommendation

Reduce number of characters per line to improve readability.

Instances (3):

File: 2023-01-popcorn/src/utils/MultiRewardEscrow.sol

49: * @dev there is no check to ensure that all escrows are owned by the same account. Make sure to account for this either by only sending ids for a specific account or by filtering the Escrows by account later on.

File: 2023-01-popcorn/src/utils/MultiRewardStaking.sol

7: import { ERC4626Upgradeable, ERC20Upgradeable, IERC20Upgradeable as IERC20, IERC20MetadataUpgradeable as IERC20Metadata } from "openzeppelin-contracts-upgradeable/token/ERC20/extensions/ERC4626Upgradeable.sol";

File: 2023-01-popcorn/src/vault/adapter/abstracts/AdapterBase.sol

6: import {ERC4626Upgradeable, IERC20Upgradeable as IERC20, IERC20MetadataUpgradeable as IERC20Metadata, ERC20Upgradeable as ERC20} from "openzeppelin-contracts-upgradeable/token/ERC20/extensions/ERC4626Upgradeable.sol";

<a id=NC7>[NC-4]</a> Add parameter to emitted event.

Description

No parameter has been passed to an emitted event. Add some parameter to better depict what has been done on the blockchain.

Recommendation

Consider passing a parameter to the event.

Instances (1):

File: 2023-01-popcorn/src/vault/adapter/abstracts/AdapterBase.sol

449: emit Harvested();

<a id=NC9>[NC-5]</a> Include return parameters in NatSpec comments.

Description

If Return parameters are declared, you must prefix them with /// @return.Some code analysis programs do analysis by reading NatSpec details, if they can't see the @return tag, they do incomplete analysis. Recommendation Code Style:

/// @notice information about what a function does
/// @param pageId The id of the page to get the URI for.
/// @return Returns a page's URI if it has been minted
function tokenURI(uint256 pageId) public view virtual override returns (string memory) {
if (pageId == 0 || pageId > currentId) revert("NOT_MINTED");
return string.concat(BASE_URI, pageId.toString());
}

Recommendation

Include return parameters in NatSpec comments.

Instances (3):

File: 2023-01-popcorn/src/vault/AdminProxy.sol

18: /// @notice Execute arbitrary management functions.

File: 2023-01-popcorn/src/vault/Vault.sol

472: /// @notice Minimal function to call `takeFees` modifier.

File: 2023-01-popcorn/src/vault/VaultController.sol

241: /// @notice Encodes adapter init call. Was moved into its own function to fix "stack too deep" error.

<a id=NC10>[NC-6]</a> Use of bytes.concat() instead of abi.encodePacked().

Description

Rather than using abi.encodePacked for appending bytes, since version 0.8.4, bytes.concat() is enabled.

Recommendation

Since version 0.8.4 for appending bytes, bytes.concat() can be used instead of abi.encodePacked().

Instances (7):

File: 2023-01-popcorn/src/utils/MultiRewardEscrow.sol

104: bytes32 id = keccak256(abi.encodePacked(token, account, amount, nonce));

File: 2023-01-popcorn/src/utils/MultiRewardStaking.sol

49: _name = string(abi.encodePacked("Staked ", IERC20Metadata(address(_stakingToken)).name()));
50: _symbol = string(abi.encodePacked("pst-", IERC20Metadata(address(_stakingToken)).symbol()));
461: abi.encodePacked(

File: 2023-01-popcorn/src/vault/Vault.sol

94: abi.encodePacked("Popcorn", name(), block.timestamp, "Vault")
680: abi.encodePacked(

File: 2023-01-popcorn/src/vault/adapter/abstracts/AdapterBase.sol

648: abi.encodePacked(

GAS OPTIMIZATION REPORT

---

Summary of optimizations

Total: 9 issues.

---

Gas Optimizations

<a id=G1>[G-1]</a> ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, for example when used in for and while loops

Description

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. Example for loop:

for (uint i = 0; i < length; i++) {
// do something that doesn't change the value of i
}

In this example, the for loop post condition, i.e., i++ involves checked arithmetic, which is not required. This is because the value of i is always strictly less than length <= 2**256 - 1. Therefore, the theoretical maximum value of i to enter the for-loop body is 2**256 - 2. This means that the i++ in the for loop can never overflow. Regardless, the overflow checks are performed by the compiler.

Unfortunately, the Solidity optimizer is not smart enough to detect this and remove the checks. You should manually do this by:

for (uint i = 0; i < length; i = unchecked_inc(i)) {
	// do something that doesn't change the value of i
}

function unchecked_inc(uint i) returns (uint) {
	unchecked {
		return i + 1;
	}
}

Or just:

for (uint i = 0; i < length;) {
	// do something that doesn't change the value of i
	unchecked { i++; 	}
}

Note that it’s important that the call to unchecked_inc is inlined. This is only possible for solidity versions starting from 0.8.2.

Gas savings: roughly speaking this can save 30-40 gas per loop iteration. For lengthy loops, this can be significant! (This is only relevant if you are using the default solidity checked arithmetic.)

Recommendation

Use the unchecked keyword

Instances (21):

File: 2023-01-popcorn/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: 2023-01-popcorn/src/utils/MultiRewardStaking.sol

171: for (uint8 i; i < _rewardTokens.length; i++) {
373: for (uint8 i; i < _rewardTokens.length; i++) {

File: 2023-01-popcorn/src/vault/PermissionRegistry.sol

42: for (uint256 i = 0; i < len; i++) {

File: 2023-01-popcorn/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++) {

<a id=G2>[G-2]</a> Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate.

Description

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.

For example:

mapping(address => uint256) public nonces;
-mapping (address => bool) public minters;
-mapping (address => bool) public markets;
+mapping (address => uint256) public markets_minters;
+            // address => uint256 => 1 (minters true)
+            // address => uint256 => 2 (minters false)
+            // address => uint256 => 3 (markets true)
+            // address => uint256 => 4 (markets false)

Recommendation

Where appropriate, you can combine multiple address mappings into a single mapping of an address to a struct.

Instances (3):

File: 2023-01-popcorn/src/utils/MultiRewardEscrow.sol

69: mapping(address => mapping(IERC20 => bytes32[])) public userEscrowIdsByToken;

File: 2023-01-popcorn/src/utils/MultiRewardStaking.sol

216: mapping(address => mapping(IERC20 => uint256)) public userIndex;
218: mapping(address => mapping(IERC20 => uint256)) public accruedRewards;

<a id=G3>[G-3]</a> Using storage instead of memory for structs/arrays saves gas.

Description

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 declaring 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(src)

Recommendation

Use storage for struct/array

Instances (5):

File: 2023-01-popcorn/src/utils/MultiRewardEscrow.sol

52: Escrow[] memory selectedEscrows = new Escrow[](escrowIds.length);

File: 2023-01-popcorn/src/utils/MultiRewardStaking.sol

372: IERC20[] memory _rewardTokens = rewardTokens;

File: 2023-01-popcorn/src/vault/VaultController.sol

155: address[] memory vaultContracts = new address[](1);
156: bytes[] memory rewardsDatas = new bytes[](1);

File: 2023-01-popcorn/src/vault/adapter/beefy/BeefyAdapter.sol

137: address[] memory _rewardTokens = new address[](1);

<a id=G4>[G-4]</a> Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead.

Description

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.

Recommendation

Use uint256 instead.

Instances (62):

File: 2023-01-popcorn/src/interfaces/IMultiRewardEscrow.sol

11: uint32 start;
13: uint32 end;
15: uint32 lastUpdateTime;
36: uint32 duration,
37: uint32 offset

File: 2023-01-popcorn/src/interfaces/IMultiRewardStaking.sol

16: uint64 ONE;
21: uint32 rewardsEndTimestamp;
25: uint32 lastUpdatedTimestamp;
32: uint32 escrowDuration;
34: uint32 offset;
44: uint32 escrowDuration,
45: uint32 offset

File: 2023-01-popcorn/src/interfaces/vault/IVault.sol

9: uint64 deposit;
10: uint64 withdrawal;
11: uint64 management;
12: uint64 performance;

File: 2023-01-popcorn/src/utils/MultiRewardEscrow.sol

73: event Locked(IERC20 indexed token, address indexed account, uint256 amount, uint32 duration, uint32 offset);
92: uint32 duration,
93: uint32 offset
114: uint32 start = block.timestamp.safeCastTo32() + offset;

File: 2023-01-popcorn/src/utils/MultiRewardStaking.sol

33: uint8 private _decimals;
67: function decimals() public view override(ERC20Upgradeable, IERC20Metadata) returns (uint8) {
171: for (uint8 i; i < _rewardTokens.length; i++) {
220: event RewardInfoUpdate(IERC20 rewardToken, uint160 rewardsPerSecond, uint32 rewardsEndTimestamp);
249: uint32 escrowDuration,
250: uint32 offset
274: uint64 ONE = (10**IERC20Metadata(address(rewardToken)).decimals()).safeCastTo64();
275: uint32 rewardsEndTimestamp = rewardsPerSecond == 0
307: uint32 prevEndTime = rewards.rewardsEndTimestamp;
308: uint32 rewardsEndTimestamp = _calcRewardsEnd(
340: uint32 rewardsEndTimestamp = rewards.rewardsEndTimestamp;
352: uint32 rewardsEndTimestamp,
355: ) internal returns (uint32) {
373: for (uint8 i; i < _rewardTokens.length; i++) {
450: uint8 v,

File: 2023-01-popcorn/src/vault/Vault.sol

38: uint8 internal _decimals;
100: function decimals() public view override returns (uint8) {
669: uint8 v,

File: 2023-01-popcorn/src/vault/VaultController.sol

314: uint8 len = uint8(vaults.length);
319: for (uint8 i = 0; i < len; i++) {
336: uint8 len = uint8(vaults.length);
337: for (uint8 i = 0; i < len; i++) {
353: uint8 len = uint8(vaults.length);
357: for (uint8 i = 0; i < len; i++) {
373: uint8 len = uint8(vaults.length);
374: for (uint8 i = 0; i < len; i++) {
436: uint8 len = uint8(vaults.length);
444: uint24 escrowPercentage,
446: ) = abi.decode(rewardTokenData[i], (address, uint160, uint256, bool, uint224, uint24, uint256));
488: uint8 len = uint8(vaults.length);
517: uint8 len = uint8(vaults.length);
563: uint8 len = uint8(templateCategories.length);
583: uint8 len = uint8(templateCategories.length);
606: uint8 len = uint8(vaults.length);
619: uint8 len = uint8(vaults.length);
632: uint8 len = uint8(vaults.length);
645: uint8 len = uint8(vaults.length);
765: uint8 len = uint8(adapters.length);
805: uint8 len = uint8(adapters.length);

File: 2023-01-popcorn/src/vault/adapter/abstracts/AdapterBase.sol

38: uint8 internal _decimals;
93: returns (uint8)
637: uint8 v,

<a id=G5>[G-5]</a> abi.encode() is less efficient than abi.encodePacked()

Description

abi.encode will apply ABI encoding rules. Therefore all elementary types are padded to 32 bytes and dynamic arrays include their length. Therefore it is possible to also decode this data again (with abi.decode) when the type are known.

abi.encodePacked will only use the only use the minimal required memory to encode the data. E.g. an address will only use 20 bytes and for dynamic arrays only the elements will be stored without length. For more info see the Solidity docs for packed mode.

For the input of the keccak method it is important that you can ensure that the resulting bytes of the encoding are unique. So if you always encode the same types and arrays always have the same length then there is no problem. But if you switch the parameters that you encode or encode multiple dynamic arrays you might have conflicts. For example: abi.encodePacked(address(0x0000000000000000000000000000000000000001), uint(0)) encodes to the same as abi.encodePacked(uint(0x0000000000000000000000000000000000000001000000000000000000000000), address(0)) and abi.encodePacked(uint[](1,2), uint[](3)) encodes to the same as abi.encodePacked(uint[](1), uint[](2,3)) Therefore these examples will also generate the same hashes even so they are different inputs. On the other hand you require less memory and therefore in most cases abi.encodePacked uses less gas than abi.encode.

Recommendation

Use abi.encodePacked() where possible to save gas

Instances (7):

File: 2023-01-popcorn/src/utils/MultiRewardStaking.sol

465: abi.encode(
494: abi.encode(

File: 2023-01-popcorn/src/vault/Vault.sol

684: abi.encode(
719: abi.encode(

File: 2023-01-popcorn/src/vault/VaultController.sol

219: abi.encode(asset, address(adminProxy), IStrategy(strategy), harvestCooldown, requiredSigs, strategyData.data),

File: 2023-01-popcorn/src/vault/adapter/abstracts/AdapterBase.sol

652: abi.encode(
687: abi.encode(

<a id=G6>[G-6]</a> Internal functions only called once can be inlined to save gas.

Description

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls. Check this out: link Note: To sum up, when there is an internal function that is only called once, there is no point for that function to exist.

Recommendation

Remove the function and make it inline if it is a simple function.

Instances (15):

File: 2023-01-popcorn/src/utils/MultiRewardEscrow.sol

170: function _getClaimableAmount(Escrow memory escrow) internal view returns (uint256) {

File: 2023-01-popcorn/src/utils/MultiRewardStaking.sol

102: function _convertToAssets(uint256 shares, Math.Rounding) internal pure override returns (uint256) {
402: function _accrueRewards(IERC20 _rewardToken, uint256 accrued) internal {

File: 2023-01-popcorn/src/vault/VaultController.sol

154: function _handleVaultStakingRewards(address vault, bytes memory rewardsData) internal {
390: function _registerVault(address vault, VaultMetadata memory metadata) internal {
667: function _verifyCreatorOrOwner(address vault) internal returns (VaultMetadata memory metadata) {
692: function _verifyAdapterConfiguration(address adapter, bytes32 adapterId) internal view {
700: function _verifyEqualArrayLength(uint256 length1, uint256 length2) internal pure {
836: function _setDeploymentController(IDeploymentController _deploymentController) internal {

File: 2023-01-popcorn/src/vault/adapter/abstracts/AdapterBase.sol

479: function _verifyAndSetupStrategy(bytes4[8] memory requiredSigs) internal {
594: function _protocolDeposit(uint256 assets, uint256 shares) internal virtual {

File: 2023-01-popcorn/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) {

<a id=G7>[G-7]</a> Multiple accesses of a mapping/array should use a local variable cache.

Description

The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations. Caching an array's struct avoids recalculating the array offsets into memory/calldata. Similar findings

Recommendation

Use a local variable cache.

Instances (4):

File: 2023-01-popcorn/src/utils/MultiRewardStaking.sol

182: _rewardTokens[i].transfer(user, rewardAmount);

File: 2023-01-popcorn/src/vault/PermissionRegistry.sol

43: if (newPermissions[i].endorsed && newPermissions[i].rejected) revert Mismatch();
45: emit PermissionSet(targets[i], newPermissions[i].endorsed, newPermissions[i].rejected);

File: 2023-01-popcorn/src/vault/VaultController.sol

526: rewardTokens[i].transferFrom(msg.sender, address(this), amounts[i]);

<a id=G8>[G-8]</a> >= costs less gas than >.

Description

The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=, which saves 3 gas

Recommendation

Use >= instead if appropriate.

Instances (38):

File: 2023-01-popcorn/src/utils/MultiRewardEscrow.sol

107: if (feePerc > 0) {
141: return escrows[escrowId].lastUpdateTime != 0 && escrows[escrowId].balance > 0;

File: 2023-01-popcorn/src/utils/MultiRewardStaking.sol

178: if (escrowInfo.escrowPercentage > 0) {
255: if (rewards.lastUpdatedTimestamp > 0) revert RewardTokenAlreadyExist(rewardToken);
257: if (amount > 0) {
309: prevEndTime > block.timestamp ? prevEndTime : block.timestamp.safeCastTo32(),
341: if (rewards.rewardsPerSecond > 0) {
356: if (rewardsEndTimestamp > block.timestamp)
377: if (rewards.rewardsPerSecond > 0) _accrueRewards(rewardToken, _accrueStatic(rewards));
392: if (rewards.rewardsEndTimestamp > block.timestamp) {
394: } else if (rewards.rewardsEndTimestamp > rewards.lastUpdatedTimestamp) {

File: 2023-01-popcorn/src/vault/Vault.sol

149: if (feeShares > 0) _mint(feeRecipient, feeShares);
189: if (feeShares > 0) _mint(feeRecipient, feeShares);
235: if (feeShares > 0) _mint(feeRecipient, feeShares);
273: if (feeShares > 0) _mint(feeRecipient, feeShares);
432: managementFee > 0
453: performanceFee > 0 && shareValue > highWaterMark
486: if (shareValue > highWaterMark) highWaterMark = shareValue;
488: if (managementFee > 0) feesUpdatedAt = block.timestamp;
490: if (totalFee > 0 && currentAssets > 0)
630: if (_quitPeriod < 1 days || _quitPeriod > 7 days)

File: 2023-01-popcorn/src/vault/VaultController.sol

103: if (adapterData.id > 0)
112: if (rewardsData.length > 0) _handleVaultStakingRewards(vault, rewardsData);
169: if (initialDeposit > 0) {
211: if (strategyData.id > 0) {
694: if (adapterId > 0) revert AdapterConfigFaulty();
753: if (newFee > 2e17) revert InvalidPerformanceFee(newFee);
793: if (newCooldown > 1 days) revert InvalidHarvestCooldown(newCooldown);

File: 2023-01-popcorn/src/vault/adapter/abstracts/AdapterBase.sol

116: if (assets > maxDeposit(receiver)) revert MaxError(assets);
135: if (shares > maxMint(receiver)) revert MaxError(shares);
178: if (assets > maxWithdraw(owner)) revert MaxError(assets);
198: if (shares > maxRedeem(owner)) revert MaxError(shares);
535: performanceFee_ > 0 && shareValue > highWaterMark_
551: if (newFee > 2e17) revert InvalidPerformanceFee(newFee);
563: if (fee > 0) _mint(FEE_RECIPIENT, convertToShares(fee));
566: if (shareValue > highWaterMark) highWaterMark = shareValue;

File: 2023-01-popcorn/src/vault/adapter/beefy/BeefyAdapter.sol

156: if (beefyFee > 0)
177: if (beefyFee > 0)

<a id=G9>[G-9]</a> <x> += <y> costs more gas than <x> = <x> + <y> for state variables

Description

Using the addition operator instead of plus-equals saves 113 gas

Recommendation

Use <x> = <x> + <y> instead.

Instances (12):

File: 2023-01-popcorn/src/utils/MultiRewardEscrow.sol

110: amount -= fee;
162: escrows[escrowId].balance -= claimable;

File: 2023-01-popcorn/src/utils/MultiRewardStaking.sol

357: amount += uint256(rewardsPerSecond) * (rewardsEndTimestamp - block.timestamp);
408: rewardInfos[_rewardToken].index += deltaIndex;
431: accruedRewards[_user][_rewardToken] += supplierDelta;

File: 2023-01-popcorn/src/vault/Vault.sol

228: shares += feeShares;
343: shares += shares.mulDiv(
365: assets += assets.mulDiv(
387: assets -= assets.mulDiv(

File: 2023-01-popcorn/src/vault/adapter/abstracts/AdapterBase.sol

158: underlyingBalance += _underlyingBalance() - underlyingBalance_;
225: underlyingBalance -= underlyingBalance_ - _underlyingBalance();

File: 2023-01-popcorn/src/vault/adapter/beefy/BeefyAdapter.sol

178: assets -= assets.mulDiv(