Popcorn contest - Diana's results

01 Best practice is to prevent signature malleability

Use OpenZeppelin’s ECDSA contract rather than calling ecrecover() directly

There are 3 instances of this issue

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/utils/MultiRewardStaking.sol

File: src/utils/MultiRewardStaking.sol

459: address recoveredAddress = ecrecover(

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/Vault.sol

File: src/vault/Vault.sol

678: address recoveredAddress = ecrecover(

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol

File: src/vault/adapter/abstracts/AdapterBase.sol

646: address recoveredAddress = ecrecover(

---

02 Missing events for functions that change critical parameters

The afunctions that change critical parameters should emit events. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services. The alternative of directly querying on-chain contract state for such changes is not considered practical for most users/usages.

Missing events and timelocks do not promote transparency and if such changes immediately affect users’ perception of fairness or trustworthiness, they could exit the protocol causing a reduction in liquidity which could negatively impact protocol TVL and reputation.

Recommended Mitigation Steps

Add events to all functions that change critical parameters.

There are 4 instances of this issue

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/VaultController.sol

File: 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 {
764: function setAdapterPerformanceFees(address[] calldata adapters) external onlyOwner {
804: function setAdapterHarvestCooldowns(address[] calldata adapters) external onlyOwner {

---

03 Adding a return statement when the function defines a named return variable, is redundant

There are 2 instances of this issue

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/AdminProxy.sol

File: src/vault/AdminProxy.sol

19-22: function execute(address target, bytes calldata callData)
    external
    onlyOwner
    returns (bool success, bytes memory returndata)

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol

File: src/vault/adapter/abstracts/AdapterBase.sol

380-385: function _convertToShares(uint256 assets, Math.Rounding rounding)
        internal
        view
        virtual
        override
        returns (uint256 shares)

---

04 Use scientific notation rather than exponentiation

Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18)

Scientific notation should be used for better code readability

There are 3 instances of this issue

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/utils/MultiRewardStaking.sol

File: src/utils/MultiRewardStaking.sol

274: uint64 ONE = (10**IERC20Metadata(address(rewardToken)).decimals()).safeCastTo64();
406: deltaIndex = accrued.mulDiv(uint256(10**decimals()), supplyTokens, Math.Rounding.Down).safeCastTo224();

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/yearn/YearnAdapter.sol

File: src/vault/adapter/yearn/YearnAdapter.sol

25: uint256 constant DEGRADATION_COEFFICIENT = 10**18;

---

05 Open TODOS

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

There is 1 instance of this issue

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol

File: src/vault/adapter/abstracts/AdapterBase.sol

516: // TODO use deterministic fee recipient proxy

---

06 Typos

There are 3 instances of this issue

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/interfaces/vault/ITemplateRegistry.sol

File: src/interfaces/vault/ITemplateRegistry.sol

13: /// @Notice Optional - Metadata CID which can be used by the frontend to add informations to a vault/adapter...

Typo: It should be information instead of informations

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/VaultController.sol

File: src/vault/VaultController.sol

87: * @dev This function is the one stop solution to create a new vault with all necessary admin functions or auxiliery contracts.
181: * @notice Deploy a new Adapter with our without a strategy. Caller must be owner.

Typo: In line 87, it should be auxiliary instead of auxiliery In line 181, it should be or instead of our

---

07 Use a more recent version of solidity

It's a best practice to use the latest compiler version. Older compilers might be susceptible to some bugs. We recommend changing the solidity version pragma to the latest version to enforce the use of an up to date compiler.

List of known compiler bugs and their severity can be found here: https://etherscan.io/solcbuginfo

This issue exists in all the In-scope contracts. There are 35 instances of this issue

EIP165.sol#L4 , OnlyStrategy.sol#L4 , AdminProxy.sol#L4 , WithRewards.sol#L4 , CloneFactory.sol#L4 , PermissionRegistry.sol#L4 , CloneRegistry.sol#L4 , VaultRegistry.sol#L4 , DeploymentController.sol#L4 , TemplateRegistry.sol#L4 , YearnAdapter.sol#L4 , MultiRewardEscrow.sol#L4 , BeefyAdapter.sol#L4 , MultiRewardStaking.sol#L4 , Vault.sol#L4 , VaultController.sol#L3 , AdapterBase.sol#L4 , IEIP165.sol#L2 , IAdminProxy.sol#L4 , IWithRewards.sol#L4 , ICloneFactory.sol#L4 , IStrategy.sol#L4 , ICloneRegistry.sol#L4 , IPermissionRegistry.sol#L4 , IVaultRegistry.sol#L3 , IYearn.sol#L4 , IAdapter.sol#L4 , IDeploymentController.sol#L4 , ITemplateRegistry.sol#L3 , IMultiRewardEscrow.sol#L3 , IERC4626.sol#L3 , IBeefy.sol#L4 , IMultiRewardStaking.sol#L3 , IVault.sol#L3 , IVaultController.sol#L4

---

08 Non-library or interface files should use fixed compiler versions, not floating ones

In the contracts, floating pragmas should not be used. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

This issue exists in all the In-scope contracts. There are 35 instances of this issue

EIP165.sol#L4 , OnlyStrategy.sol#L4 , AdminProxy.sol#L4 , WithRewards.sol#L4 , CloneFactory.sol#L4 , PermissionRegistry.sol#L4 , CloneRegistry.sol#L4 , VaultRegistry.sol#L4 , DeploymentController.sol#L4 , TemplateRegistry.sol#L4 , YearnAdapter.sol#L4 , MultiRewardEscrow.sol#L4 , BeefyAdapter.sol#L4 , MultiRewardStaking.sol#L4 , Vault.sol#L4 , VaultController.sol#L3 , AdapterBase.sol#L4 , IEIP165.sol#L2 , IAdminProxy.sol#L4 , IWithRewards.sol#L4 , ICloneFactory.sol#L4 , IStrategy.sol#L4 , ICloneRegistry.sol#L4 , IPermissionRegistry.sol#L4 , IVaultRegistry.sol#L3 , IYearn.sol#L4 , IAdapter.sol#L4 , IDeploymentController.sol#L4 , ITemplateRegistry.sol#L3 , IMultiRewardEscrow.sol#L3 , IERC4626.sol#L3 , IBeefy.sol#L4 , IMultiRewardStaking.sol#L3 , IVault.sol#L3 , IVaultController.sol#L4

---

---

Gas optimization

---

---

G-01 Increments can be unchecked

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

Prior to Solidity 0.8.0, arithmetic operations would always wrap in case of under- or overflow leading to widespread use of libraries that introduce additional checks.

Since Solidity 0.8.0, all arithmetic operations revert on over- and underflow by default, thus making the use of these libraries unnecessary.

To obtain the previous behaviour, an unchecked block can be used. It saves 30-40 gas per loop.

There is 12 instances of this issue

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/utils/MultiRewardEscrow.sol

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++) {

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/utils/MultiRewardStaking.sol

File: src/utils/MultiRewardStaking.sol

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

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/PermissionRegistry.sol

File: src/vault/PermissionRegistry.sol

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

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/VaultController.sol

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++) {

---

G-02 Usage of uints or ints smaller than 32 bytes incurs overhead

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 Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed

There are 31 instances of this issue:

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/interfaces/vault/IVaultController.sol

File: src/interfaces/vault/IVaultController.sol

60: uint160[] memory rewardsSpeeds

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/utils/MultiRewardEscrow.sol

File: src/utils/MultiRewardEscrow.sol

114: uint32 start = block.timestamp.safeCastTo32() + offset;

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/utils/MultiRewardStaking.sol

File: src/utils/MultiRewardStaking.sol

33: uint8 private _decimals;
245: uint160 rewardsPerSecond,
340: uint32 rewardsEndTimestamp = rewards.rewardsEndTimestamp;
450: uint8 v,

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/Vault.sol

File: src/vault/Vault.sol

38: uint8 internal _decimals;
669: uint8 v,

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/VaultController.sol

File: 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);
437: for (uint256 i = 0; i < len; i++) {
440: uint160 rewardsPerSecond,
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);

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol

File: src/vault/adapter/abstracts/AdapterBase.sol

38: uint8 internal _decimals;
637: uint8 v,

---

G-03 x += y costs more gas than x = x + y for state variables (same applies for x -= y)

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

There are 12 instances of this issue

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/utils/MultiRewardEscrow.sol

File: src/utils/MultiRewardEscrow.sol

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

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/utils/MultiRewardStaking.sol

File: src/utils/MultiRewardStaking.sol

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

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/Vault.sol

File: src/vault/Vault.sol

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

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol

File: src/vault/adapter/abstracts/AdapterBase.sol

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

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/beefy/BeefyAdapter.sol

File: src/vault/adapter/beefy/BeefyAdapter.sol

178: assets -= assets.mulDiv(

---

G-04 Internal functions only called once can be inlined to save gas

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

There are 14 instances of this issue

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/utils/MultiRewardStaking.sol

File: src/utils/MultiRewardStaking.sol

191: function _lockToken(

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/VaultController.sol

File: src/vault/VaultController.sol

120: function _deployVault(VaultInitParams memory vaultData, IDeploymentController _deploymentController)
141: function _registerCreatedVault(
154: function _handleVaultStakingRewards(address vault, bytes memory rewardsData) internal {
225: function __deployAdapter(
242: function _encodeAdapterData(DeploymentArgs memory adapterData, bytes memory baseAdapterData)
256: function _deployStrategy(DeploymentArgs memory strategyData, IDeploymentController _deploymentController)
390: function _registerVault(address vault, VaultMetadata memory metadata) internal {
692: function _verifyAdapterConfiguration(address adapter, bytes32 adapterId) internal view {

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol

File: src/vault/adapter/abstracts/AdapterBase.sol

479: function _verifyAndSetupStrategy(bytes4[8] memory requiredSigs) internal {

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/yearn/YearnAdapter.sol

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) {

---

G-05 Using both named returns and a return statement isn't necessary

Removing unused named returns variables can reduce gas usage (MSTOREs/MLOADs) and improve code clarity. To save gas and improve code quality: consider using only one of those.

Each MLOAD and MSTORE costs 3 additional gas

There are 2 instances of this issue

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/AdminProxy.sol

File: src/vault/AdminProxy.sol

19-22: function execute(address target, bytes calldata callData)
    external
    onlyOwner
    returns (bool success, bytes memory returndata)

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol

File: src/vault/adapter/abstracts/AdapterBase.sol

380-385: function _convertToShares(uint256 assets, Math.Rounding rounding)
        internal
        view
        virtual
        override
        returns (uint256 shares)

---

G-06 Use a more recent version of solidity

This issue exists in all the In-scope contracts. There are 35 instances of this issue

EIP165.sol#L4 , OnlyStrategy.sol#L4 , AdminProxy.sol#L4 , WithRewards.sol#L4 , CloneFactory.sol#L4 , PermissionRegistry.sol#L4 , CloneRegistry.sol#L4 , VaultRegistry.sol#L4 , DeploymentController.sol#L4 , TemplateRegistry.sol#L4 , YearnAdapter.sol#L4 , MultiRewardEscrow.sol#L4 , BeefyAdapter.sol#L4 , MultiRewardStaking.sol#L4 , Vault.sol#L4 , VaultController.sol#L3 , AdapterBase.sol#L4 , IEIP165.sol#L2 , IAdminProxy.sol#L4 , IWithRewards.sol#L4 , ICloneFactory.sol#L4 , IStrategy.sol#L4 , ICloneRegistry.sol#L4 , IPermissionRegistry.sol#L4 , IVaultRegistry.sol#L3 , IYearn.sol#L4 , IAdapter.sol#L4 , IDeploymentController.sol#L4 , ITemplateRegistry.sol#L3 , IMultiRewardEscrow.sol#L3 , IERC4626.sol#L3 , IBeefy.sol#L4 , IMultiRewardStaking.sol#L3 , IVault.sol#L3 , IVaultController.sol#L4

---