Popcorn contest - 0xAgro's results

QA Report

Finding Summary

Low Severity

1. Relatively Small block.timestamp Cast
2. Unchecked Cast May Overflow
3. TODO Left in Production Code*

Non-Critical

1. Use of Exponentiation Over Scientific Notation
2. Use of ecrecover
3. Order of Functions Not Compliant With Solidity Docs
4. Contract Layout Voids Solidity Docs
5. Long Lines
6. Order of Modifiers Not Compliant With Solidity Docs
7. Contracts Missing @title Tag
8. Inconsistent Comment Initial Spacing
9. Inconsistent Named Returns
10. Spelling Mistakes
11. Inconsistent Header Centering
12. Low Coverage
13. Inconsistent Pragma Spacing
14. bytes.concat() Can Be Used Over abi.encodePacked()
15. Inconsistent Comment Capitalization
16. Named Returns Not Returning Named Variable
17. Inconsistent Trailing Period
18. Confusing Word shortening
19. Inconsistent Headers
20. Lack of Overflow Error Message
21. Unused Cache Variable
22. Add view Modifier
23. Unused Param
24. Assumptive Language
25. Lack of Interface Documentation

Low Severity

1. Relatively Small block.timestamp Cast

As of now block.timestamp.safeCastTo32() will always revert when the block.timestamp is greater than type(uint32).max := 4294967295. block.timestamp returns the current seconds since unix epoch. A unix time of 4294967295 would imply a date and time of Sunday, 7 February 2106 06:28:15 GMT (can be verified here). In approximetly 83 years all functions that implement the call block.timestamp.safeCastTo32() will always revert.

/src/utils/MultiRewardEscrow.sol

114:	uint32 start = block.timestamp.safeCastTo32() + offset;
163:	escrows[escrowId].lastUpdateTime = block.timestamp.safeCastTo32();
175:	block.timestamp.safeCastTo32() < escrow.start

/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();
409:	rewardInfos[_rewardToken].lastUpdatedTimestamp = block.timestamp.safeCastTo32();

2. Unchecked Cast May Overflow

As of Solidity 0.8 overflows are handled automatically; however, not for casting. For example uint32(4294967300) will result in 4 without reversion. Consider using a SafeCast.

/src/vault/VaultController.sol

314:	uint8 len = uint8(vaults.length);
336:	uint8 len = uint8(vaults.length);
353:	uint8 len = uint8(vaults.length);
373:	uint8 len = uint8(vaults.length);
436:	uint8 len = uint8(vaults.length);
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);

3. TODO Left in Production Code

It is understood that the knowledge of this TODO and generally why TODOs should be removed is understood by the team here. This issue is left in as it does not technically void scope and can be beneficial for future auditors.

TODOs should not be in production code. Each TODO should either be discarded or implemented if needed prior to production.

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

516:    // TODO use deterministic fee recipient proxy

Non-Critical

1. Use of Exponentiation Over Scientific Notation

For better style and less computation consider replacing any power of 10 exponentiation (10**3) with its equivalent scientific notation (1e3).

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

25:	uint256 constant DEGRADATION_COEFFICIENT = 10**18;

2. Use of ecrecover

Consider using OpenZeppelin's ECDSA contract instead of raw ecrecover.

/src/utils/MultiRewardStaking.sol

459:	address recoveredAddress = ecrecover(

/src/vault/Vault.sol

678:	address recoveredAddress = ecrecover(

3. Order of Functions Not Compliant With Solidity Docs

The Solidity Style Guide suggests the following function order: constructor, receive function (if exists), fallback function (if exists), external, public, internal, private.

The following contracts are not compliant (examples are only to prove the functions are out of order NOT a full description):

• YearnAdapter.sol: public functions are positioned after internal functions.
• MultiRewardEscrow.sol: external functions are positioned after internal functions.
• BeefyAdapter.sol: public functions are positioned after internal functions.
• MultiRewardStaking.sol: external functions are positioned after public functions.
• Vault.sol: external functions are positioned after public functions.
• VaultController.sol: external functions are positioned after internal functions.
• AdapterBase.sol: public functions are positioned after internal functions.

4. Contract Layout Voids Solidity Docs

The Solidity Style Guide suggests the following contract layout order: Type declarations, State variables, Events, Modifiers, Functions.

The following contracts are not compliant (examples are only to prove the layout are out of order NOT a full description):

• CloneFactory.sol: Events are positioned after Constructor (Functions).
• PermissionRegistry.sol: State is positioned after Constructor (Functions).
• CloneRegistry.sol: State is positioned after Constructor (Functions).
• VaultRegistry.sol: State is positioned after Constructor (Functions).
• TemplateRegistry.sol: State is positioned after Constructor (Functions).
• MultiRewardEscrow.sol: State is positioned after Constructor (Functions).
• MultiRewardStaking.sol: Events are positioned after Functions.
• Vault.sol: Events are positioned after Functions.
• VaultController.sol: Events are positioned after Constructor (Functions).
• AdapterBase.sol: State is positioned after Functions

5. Long Lines

Lines with greater length than 120 characters are used. The Solidity Style Guide suggests that all lines should be 120 characters or less in width.

The following lines are longer than 120 characters, it is suggested to shorten these lines:

/src/vault/AdminProxy.sol

• 13.

/src/vault/VaultRegistry.sol

• 41.

/src/vault/DeploymentController.sol

• 91.

/src/vault/TemplateRegistry.sol

• 49.

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

• 6.

/src/utils/MultiRewardEscrow.sol

• 6, 49.

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

• 6, 16.

/src/utils/MultiRewardStaking.sol

• 6, 7, 22, 106, 120, 291, 380.

/src/vault/Vault.sol

• 6, 55, 398, 408, 425, 426.

/src/vault/VaultController.sol

• 5, 87, 477, 604, 630.

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

• 6, 7, 52, 262, 269, 436, 465.

6. Order of Modifiers Not Compliant With Solidity Docs

The Solidity Style Guide suggests the following modifer order: Visibility, Mutability, Virtual, Override, Custom modifiers.

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

• _deposit ('internal', 'nonReentrant', 'virtual', 'override'): override (Override) positioned after nonReentrant (Custom Modifiers).

7. Contracts Missing @title Tag

21 out of 35 of the contracts in scope are missing a @title tag. Given that 14 contracts all have a @title tag, consider adding one per the 21 remaining contracts.

EIP165.sol, OnlyStrategy.sol, WithRewards.sol, IEIP165.sol, IAdminProxy.sol, IWithRewards.sol, ICloneFactory.sol, IStrategy.sol, ICloneRegistry.sol, IPermissionRegistry.sol, IVaultRegistry.sol, IYearn.sol, IAdapter.sol, IDeploymentController.sol, ITemplateRegistry.sol, IMultiRewardEscrow.sol, IERC4626.sol, IBeefy.sol, IMultiRewardStaking.sol, IVault.sol, and IVaultController.sol are missing a @title tag.

8. Inconsistent Comment Initial Spacing

Almost all comments have an initial space after // or /// while one does not. It is best for code clearity to keep a consistent style.

1. The following contracts only have initial space comments (EX. // foo): EIP165.sol, OnlyStrategy.sol, AdminProxy.sol, WithRewards.sol, CloneFactory.sol, PermissionRegistry.sol, CloneRegistry.sol, VaultRegistry.sol, DeploymentController.sol, TemplateRegistry.sol, YearnAdapter.sol, MultiRewardEscrow.sol, BeefyAdapter.sol, MultiRewardStaking.sol, Vault.sol, VaultController.sol, AdapterBase.sol, IEIP165.sol, IAdminProxy.sol, IWithRewards.sol, ICloneFactory.sol, IStrategy.sol, ICloneRegistry.sol, IPermissionRegistry.sol, IVaultRegistry.sol, IYearn.sol, IAdapter.sol, IDeploymentController.sol, ITemplateRegistry.sol, IMultiRewardEscrow.sol, IERC4626.sol, IMultiRewardStaking.sol, IVault.sol, and IVaultController.sol.
2. No contracts have only no initial space comments (EX. //foo).
3. The following contracts have both: IBeefy.sol.

9. Inconsistent Named Returns

Some functions use named returns and others do not. It is best for code clearity to keep a consistent style.

1. The following contracts only have named returns (EX. returns(uint256 foo)): AdminProxy.sol, CloneFactory.sol, and DeploymentController.sol.
2. The following contracts only have non-named returns (EX. returns(uint256)): EIP165.sol, WithRewards.sol, PermissionRegistry.sol, CloneRegistry.sol, VaultRegistry.sol, TemplateRegistry.sol, YearnAdapter.sol, MultiRewardEscrow.sol, and BeefyAdapter.sol.
3. The following contracts have both: MultiRewardStaking.sol, Vault.sol, VaultController.sol, and AdapterBase.sol.

10. Spelling Mistakes

Consider fixing all spelling mistakes.

/src/vault/AdminProxy.sol

• The word Owns is misspelled as ownes.

11. Inconsistent Header Centering

The headers in the codebase are not centered. For example, this line is not centered with this line. Consider centering all headers.

12. Low Coverage

The code coverage of the files in scope is not 100%. Consider a test coverage of 100%.

13. Inconsistent Pragma Spacing

Some contract in the codebase have a space after the license and before the pragma, but some do not. The following contracts do not have a space, all else do:

• VaultController.sol, IEIP165.sol, IVaultRegistry.sol, ITemplateRegistry.sol, IMultiRewardEscrow.sol, IERC4626.sol, IMultiRewardStaking.sol, IVault.sol.

14. bytes.concat() Can Be Used Over abi.encodePacked()

Consider using bytes.concat() instead of abi.encodePacked() in contracts with Solidity version >= 0.8.4.

/src/utils/MultiRewardEscrow.sol

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

/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(

/src/vault/Vault.sol

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

15. Inconsistent Comment Capitalization

There is an inconsistency in the capitalization of comments. For example, this line is not capitilized while this line is. Consider capitilizing all comments.

16. Named Returns Not Returning Named Variable

Using named returns may help developers understand what is being returned, but this should be in a @return tag. There is no need to confuse developers.

/src/vault/AdminProxy.sol

• execute

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

• _convertToShares

17. Inconsistent Trailing Period

There is a general inconsistency with puncuation in comments, specifically trailing .s. The following items are proof by example and not every case in the codebase:

• This @notice tag does not have a trailing . but this does.

• This @param tag does not have a trailing . but this does.

• This single line comment does not have a trailing . but this does.

18. Confusing Word shortening

It seems as though the word LOGIC is shortened to LOGC. Consider spelling the word in full to dilute possible confusion given it is only a one characer difference.

19. Inconsistent Headers

Not all headers follow the same general format. Consider sticking to the same format.

Example 1:

/*//////////////////////////////////////////////////////////////
                      REWARDS ACCRUAL LOGIC
    //////////////////////////////////////////////////////////////*/

Example 2:

// MANAGEMENT FUNCTIONS - FEES

20. Lack of Overflow Error Message

In the lock function of MultiRewardEscrow.sol L114 can overflow (revert) without a proper error message. Consider adding an error message for when a user inputs a large offset.

21. Unused Cache Variable

The variable highWaterMark_ in the function accruedPerformanceFee appears to be a cache variable that is never used. When returning from the function highWaterMark is used instead of highWaterMark_. Consider using highWaterMark_ or removing it.

22. Add view Modifier

A view modifier can be added to the following functions to get rid of compiler warnings:

• _calcRewardsEnd, _encodeAdapterData, _verifyCreatorOrOwner.

23. Unused Param

_registerVault has a parameter address vault that is never used. Consider attending to any security issues that may arrise from the non-use, and possibly removing the parameter.

24. Assumptive Language

In Vault.sol the phrase rage quit is used (ex) which seems to be in jest; however, it is not good to use assumptive language. For individuals that are unaware of the jestful nature (EX. non-native English speakers, etc.) who may also be docile, the assumption that quitting is due to "rage" may be subliminally damaging to ones initial intent with the system.

25. Lack of Interface Documentation

It is best practice to document interfaces just like you would core contracts. Most interfaces do not have much internal documentation (comments). For example: IEIP165.sol, IAdminProxy.sol, IWithRewards.sol, etc. are missing all comments aside from the license identifiers. Consider documenting all interfaces to the level you would core contracts.