Popcorn contest - descharre's results

Summary

Low Risk

Non critical

Details

Low Risk

[L-01] Missing checks for 0 address

• MultiRewardEscrow.sol#L31
• VaultController.sol#L52-L70
• DeploymentController.sol#L35-L44
• VaultController.sol#L53-L70

[L-02] Wrong check for tokenFees

The if statement reverts when the tokenFee is larger than 1e17. However the natspec commment says 1e18 is 100% and I don't see anywhere in the documentation where it says max fee can be 1e17. The if statent should instead check if it's not larger than 1e18.

• MultiRewardEscrow.sol#L211

      if (tokenFees[i] >= 1e17) revert DontGetGreedy(tokenFees[i]);

[L-03] Unspecific compiler version pragma

Avoid floating pragmas for non-library contracts. Contracts should always be deployed with the same compiler version and flags that they have been tested with. Locking the pragma helps ensure that contracts do not accidentally get deployed using a compiler which may have higher risks of undiscovered bugs.

[L-04] Return value of call is not checked

The return value of call is not checked in the execute() function of AdminProxy.sol. This can lead to unexpected failures.

A recommendation is to check the boolean success and revert the transaction if it's false.

[L-05] Account existence check for low-level calls

Low-level calls call/delegatecall/staticcall return true even if the account called is non-existent (per EVM design). Account existence must be checked before the calling.

• AdminProxy.sol#L24

Non critical

[N-01] Use indexed fields in an event

When an event is missing indexed fields it can be hard for off-chain scripts to efficiently filter those events.

• MultiRewardStaking.sol#L220
• Owned.sol#L38
• Owned.sol#L39
• OwnedUpgradeable.sol#L40-L41
• CloneRegistry.sol#L33
• PermissionRegistry.sol#L28
• TemplateRegistry.sol#L38-L40
• TemplateRegistry.sol#L88
• Vault.sol#L512-L514
• Vault.sol#L569-L570
• Vault.sol#L621
• VaultController.sol#L741
• VaultController.sol#L781
• VaultController.sol#L824
• VaultController.sol#L853
• VaultRegistry.sol#L36
• AdapterBase.sol#L491
• AdapterBase.sol#L519

[N-02] Unused named return variables

When you have named return variables and you don't use them, then it's better to just specify the type you are gonna return.

• AdminProxy.sol#L22 success and returndata are never used
• AdapterBase.sol#L385 shares is never used

[N-03] Modifiers should not make state changes

Modifiers should only implement checks and not make state changes. This violates the checks-effects-interactions-pattern.

• The modifier Vault/takeFees calls the _mint function which makes state changes. Additonally it also changes the state variables highWaterMark and feesUpdatedAt.
• The modifier syncFeeCheckpoint changes the state variable highWaterMark
• The modifier accrueRewards calls multiple function that changes state variables.
• The modifier AdapterBase/takeFees changes the state variable highWaterMark and calls the _mint function.

Summary

Details

[G-01] Make for loop unchecked

The risk of for loops getting overflowed is extremely low. Because it always increments by 1 and is limited to the arrays length. Even if the arrays are extremely long, it will take a massive amount of time and gas to let the for loop overflow. The longer the array, the more gas you will save.

-   for (uint256 i = 0; i < escrowIds.length; i++) {
+   for (uint256 i = 0; i < escrowIds.length; ) {
      selectedEscrows[i] = escrows[escrowIds[i]];
+     unchecked{
+        i++;
+      }
    }

• MultiRewardEscrow.sol#L53-L55: getEscrows() gas saved: 105
• MultiRewardEscrow.sol#L155-L167: claimRewards() gas saved: 75
• MultiRewardEscrow.sol#L210-L215: setFees() gas saved: 68 The same amount of gas can be saved for all the for loops with uint256 in VaultController.sol and PermissionRegistry.sol

[G-02] It's not necessary to do a safecast on block.timestamp

When you cast block.timestamp to a uint32 it will never overflow. That's why it's not necessary to safecast. You can cast to uint32 in the regular way instead. Saves around 50 gas.

-    block.timestamp.safeCastTo32()
+    uint32(block.timestamp)

• MultiRewardEscrow.sol#L114
• MultiRewardEscrow.sol#L163
• MultiRewardEscrow.sol#L175
• MultiRewardStaking.sol#L276
• MultiRewardStaking.sol#L284
• MultiRewardStaking.sol#L309
• MultiRewardStaking.sol#L346
• MultiRewardStaking.sol#L409

[G-03] Use unchecked block when computation can't under/overflow

When a variable is incremented by one there is no risk in using a unchecked block. It will take a massive amount of time and gas to make it overflow

• MultiRewardEscrow.sol#L102

An unchecked block can also be used when previous functions or statement make sure there is no possibility of over/underflowing

• MultiRewardEscrow.sol#L162

[G-04] Checks are unnecessary in _transfer function

All the if statements are unnecessary because they are also in the _mint and _burn functions. It's just a gas saver to do them twice

MultiRewardStaking.sol#L147-L148

• transfer() gas saved: 241
• transferFrom() gas saved: 242

[G-05] Don't assign to a memory variable when it's only used once

When a variable is only accessed once it's a waste of gas to assign it to a memory variable first. Especially when you write struct to memory like in the following example, you can save a lot of gas with this method.

MultiRewardStaking.sol#L255:addRewardToken() gas saved: 783

-    RewardInfo memory rewards = rewardInfos[rewardToken];
-    if (rewards.lastUpdatedTimestamp > 0) revert RewardTokenAlreadyExist(rewardToken);
+    if (rewardInfos[rewardToken].lastUpdatedTimestamp > 0) revert RewardTokenAlreadyExist(rewardToken);