Aragon Protocol contest - 0xAgro's results

QA Report

Finding Summary

Low Severity

1. Unchecked Cast May Overflow
2. Low Test Coverage
3. Draft EIP Used

Non-Critical

1. Contracts Missing @title Tag
2. Inconsistent Comment Initial Spacing
3. Inconsistent Named Returns
4. bytes.concat() Can Be Used Over abi.encodePacked()
5. Explicit Function Scope Not Used
6. Space Between License and Pragma
7. Extra Spaces In License Identifier
8. Inconsistent Comment Capitalization
9. Inconsistent Trailing Period
10. Only Single Line NatSpec Comments Used
11. Named Returns Not Returning Named Variable
12. ERC20 Token Recovery
13. Constants Used Multiple Times

Style Guide Violations

1. Maximum Line Length
2. Order of Functions
3. Whitespace in Expressions
4. Function Declaration Style
5. Order of Layout

Low Severity

1. 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 library (which is used in other parts of the codebase).

/packages/contracts/src/framework/utils/RegistryUtils.sol

17:	uint8 char = uint8(nameBytes[i]);

/packages/contracts/src/plugins/governance/multisig/Multisig.sol

175:	uint16 newAddresslistLength = uint16(addresslistLength() - _members.length);
414:	uint16 addresslistLength_ = uint16(addresslistLength());

/packages/contracts/src/framework/plugin/repo/PluginRepo.sol

210:	uint16 latestBuild = uint16(buildsPerRelease[_release]);

/packages/contracts/src/core/dao/DAO.sol

192:	if (!hasBit(_allowFailureMap, uint8(i))) {
198:	failureMap = flipBit(failureMap, uint8(i));

2. Low Test Coverage

The Scoping Details states that the test coverage is 60%. The code coverage of the files in scope is not 100%. Consider a test coverage of 100%.

3. Draft EIP Used

ERC4824 is used in the codebase. EIP 4824 is in draft and could change, thus possibly impacting Aragon in the future. EIP 4824 states:

This EIP is not yet recommended for general use or implementation, as it is subject to normative (breaking) changes.

Non-Critical

1. Contracts Missing @title Tag

11 out of 62 of the contracts in scope are missing a @title tag. Given that 51 contracts all have a @title tag, consider adding one per the 11 remaining contracts.

Proxy.sol, UncheckedMath.sol, RegistryUtils.sol, ENSMigration.sol, PluginSetupProcessorHelpers.sol, Ratio.sol, IMerkleMinter.sol, IMerkleDistributor.sol, BitMap.sol, auth.sol, and IGovernanceWrappedERC20.sol are missing a @title tag.

2. Inconsistent Comment Initial Spacing

Some comments have an initial space after // or /// while others do not. It is best for code clearity to keep a consistent style. All contracts in the codebase have initial space comments. CounterV2.sol, and CounterV1.sol have some non-initial space comments (//dao.execute(...)).

3. 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)): DAOFactory.sol, PluginSetupProcessor.sol, and Ratio.sol.
2. The following contracts only have non-named returns (EX. returns(uint256)): Proxy.sol, UncheckedMath.sol, TokenFactory.sol, RegistryUtils.sol, Admin.sol, PluginRepo.sol, PluginSetupProcessorHelpers.sol, MultiplyHelper.sol, Addresslist.sol, CounterV2.sol, CounterV1.sol, MerkleDistributor.sol, PermissionManager.sol, CallbackHandler.sol, BitMap.sol, PluginUUPSUpgradeable.sol, DaoAuthorizableUpgradeable.sol, DaoAuthorizable.sol, Plugin.sol, PluginCloneable.sol, GovernanceWrappedERC20.sol, and GovernanceERC20.sol.
3. The following contracts have both: MajorityVotingBase.sol, AddresslistVotingSetup.sol, AddresslistVoting.sol, TokenVoting.sol, TokenVotingSetup.sol, MultisigSetup.sol, Multisig.sol, AdminSetup.sol, PluginRepoFactory.sol, PluginSetup.sol, CounterV2PluginSetup.sol, CounterV1PluginSetup.sol, MerkleMinter.sol, DAO.sol, ProposalUpgradeable.sol, and Proposal.sol.

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

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

/packages/contracts/src/framework/utils/ens/ENSSubdomainRegistrar.sol

86:	bytes32 subnode = keccak256(abi.encodePacked(node, _label));
86:	bytes32 subnode = keccak256(abi.encodePacked(node, _label));

/packages/contracts/src/framework/plugin/repo/PluginRepo.sol

252:	return keccak256(abi.encodePacked(_tag.release, _tag.build));
252:	return keccak256(abi.encodePacked(_tag.release, _tag.build));

/packages/contracts/src/plugins/token/MerkleDistributor.sol

105:	bytes32 node = keccak256(abi.encodePacked(_index, _to, _amount));
105:	bytes32 node = keccak256(abi.encodePacked(_index, _to, _amount));

/packages/contracts/src/core/permission/PermissionManager.sol

347:	return keccak256(abi.encodePacked("PERMISSION", _who, _where, _permissionId));
347:	return keccak256(abi.encodePacked("PERMISSION", _who, _where, _permissionId));

5. Explicit Function Scope Not Used

Function scopes should be explicit and not left out if public (the default). The following are not explicitly deemed public:

/packages/contracts/src/utils/Proxy.sol

• createERC1967Proxy.

/packages/contracts/src/utils/UncheckedMath.sol

• _uncheckedIncrement, _uncheckedAdd, _uncheckedSub.

/packages/contracts/src/framework/utils/RegistryUtils.sol

• isSubdomainValid.

/packages/contracts/src/framework/plugin/setup/PluginSetupProcessorHelpers.sol

• _getPluginInstallationId, _getPreparedSetupId, _getAppliedSetupId, hashHelpers, hashPermissions.

/packages/contracts/src/plugins/utils/Ratio.sol

• _applyRatioCeiled.

/packages/contracts/src/core/utils/BitMap.sol

• hasBit, flipBit.

/packages/contracts/src/core/utils/auth.sol

• _auth.

6. Space Between License and Pragma

All contracts in scope have a space between the license statement and pragma (ex). Although it does not void the Solidity Style Guide, all example contracts in the Style Guide do not have a space between the license statement and pragma (ex). This format can also be seen throughout the Solidy Documentation (ex). Consider removing the needless space.

7. Extra Spaces In License Identifier

In PluginRepo.sol there are 3 extra spaces in the license identifier. Contracts like GovernanceERC20.sol do not have this extra space. Consider removing the extra spaces in PluginRepo.sol.

Example

/packages/contracts/src/framework/plugin/repo/PluginRepo.sol

1:	// SPDX-License-Identifier:    AGPL-3.0-or-later

/packages/contracts/src/token/ERC20/governance/GovernanceERC20.sol

1:	// SPDX-License-Identifier: AGPL-3.0-or-later

8. Inconsistent Comment Capitalization

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

9. Inconsistent Trailing Period

Some comments like this have a trailing . but other lines like this do not.

Consider sticking to a single comment style for a less distracting developer experience.

10. Only Single Line NatSpec Comments Used

All NatSpec comments in the codebase use single line NatSpec comments through /// like so:

19:	/// @title GovernanceWrappedERC20
20:	/// @author Aragon Association
21:	/// @notice Wraps an existing [ERC-20](https://eips.ethereum.org/EIPS/eip-20) token by inheriting from `ERC20WrapperUpgradeable` and allows to use it for voting by inheriting from `ERC20VotesUpgradeable`. The latter is compatible with [OpenZepplin `Votes`](https://docs.openzeppelin.com/contracts/4.x/api/governance#Votes) interface.
22:	/// The contract also supports meta transactions. To use an `amount` of underlying tokens for voting, the token owner has to
23:	/// 1. call `approve` for the tokens to be used by this contract
24:	/// 2. call `depositFor` to wrap them, which safely transfers the underlying [ERC-20](https://eips.ethereum.org/EIPS/eip-20) tokens to the contract and mints wrapped [ERC-20](https://eips.ethereum.org/EIPS/eip-20) tokens.
25:	/// To get the [ERC-20](https://eips.ethereum.org/EIPS/eip-20) tokens back, the owner of the wrapped tokens can call `withdrawFor`, which  burns the wrapped tokens [ERC-20](https://eips.ethereum.org/EIPS/eip-20) tokens and safely transfers the underlying tokens back to the owner.
26:	/// @dev This contract intentionally has no public mint functionality because this is the responsibility of the underlying [ERC-20](https://eips.ethereum.org/EIPS/eip-20) token contract.

As described in the Solidity Documentation multi-line comments may use the /** format. The use of /// for multi-line is allowed by Solidity standards; however, for comments like this it is far easier to read using the mulit-line /** style. Consider using the mulit-line /** NstSpec style for long NatSpec comments.

11. 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. The following functions have named returned but still return one or more variables.

/packages/contracts/src/framework/plugin/setup/PluginSetupProcessor.sol

• prepareInstallation, prepareUpdate.

/packages/contracts/src/plugins/counter-example/v2/CounterV2PluginSetup.sol

• prepareInstallation.

/packages/contracts/src/plugins/counter-example/v1/CounterV1PluginSetup.sol

• prepareInstallation.

/packages/contracts/src/plugins/token/MerkleMinter.sol

• merkleMint.

12. ERC20 Token Recovery

Consider adding a recovery function for Tokens that are accidently sent to the core contracts of the protocol.

13. Constants Used Multiple Times

There is a relatively large amount of constants used in the codebase (seen here for example). In some cases like this and this the same constant is referenced in different contracts. Consider addings a constants contract to hold all constants used in Aragon.

Style Guide Violations

1. Maximum Line Length

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:

/packages/contracts/src/utils/Proxy.sol

• 7, 11.

/packages/contracts/src/framework/utils/TokenFactory.sol

• 8, 72, 74, 77.

/packages/contracts/src/framework/utils/InterfaceBasedRegistry.sol

• 6, 21, 42, 52, 74.

/packages/contracts/src/framework/utils/RegistryUtils.sol

• 9.

/packages/contracts/src/plugins/governance/majority-voting/IMajorityVoting.sol

• 13, 44, 49, 54, 84, 91.

/packages/contracts/src/framework/utils/ens/ENSSubdomainRegistrar.sol

• 15, 72, 110.

/packages/contracts/src/framework/utils/ens/ENSMigration.sol

• 2.

/packages/contracts/src/plugins/governance/majority-voting/MajorityVotingBase.sol

• 27, 31, 33, 34, 38, 62, 63, 69, 71, 78, 90, 95, 108, 109, 118, 119, 136, 148, 333, 428, 429, 432, 447, 527, 535, 560, 580, 593.

/packages/contracts/src/plugins/governance/majority-voting/token/TokenVoting.sol

• 16, 25, 59.

/packages/contracts/src/plugins/governance/majority-voting/token/TokenVotingSetup.sol

• 39.

/packages/contracts/src/plugins/governance/multisig/IMultisig.sol

• 9, 11, 15, 21, 38.

/packages/contracts/src/plugins/governance/multisig/Multisig.sol

• 33, 96, 158, 201, 203, 304.

/packages/contracts/src/plugins/governance/admin/Admin.sol

• 61.

/packages/contracts/src/plugins/governance/admin/AdminSetup.sol

• 75.

/packages/contracts/src/framework/dao/DAOFactory.sol

• 41, 50, 60, 62, 134.

/packages/contracts/src/framework/dao/DAORegistry.sol

• 72.

/packages/contracts/src/framework/plugin/repo/PluginRepo.sol

• 9, 18, 32, 255.

/packages/contracts/src/framework/plugin/repo/PluginRepoRegistry.sol

• 71.

/packages/contracts/src/framework/plugin/repo/PluginRepoFactory.sol

• 28, 41, 44, 61, 64, 111.

/packages/contracts/src/framework/plugin/repo/IPluginRepo.sol

• 14.

/packages/contracts/src/framework/plugin/setup/PluginSetupProcessor.sol

• 17, 21, 22, 37, 48, 49, 50, 57, 63, 72, 73, 85, 86, 98, 110, 111, 130, 134, 141, 158, 170, 171, 181, 213, 245, 285, 394, 395, 444, 551, 611, 646, 647, 649.

/packages/contracts/src/framework/plugin/setup/IPluginSetup.sol

• 10, 13, 14, 20, 22, 23, 32, 44, 55, 63.

/packages/contracts/src/framework/plugin/setup/PluginSetupProcessorHelpers.sol

• 9, 41.

/packages/contracts/src/framework/plugin/setup/PluginSetup.sol

• 29.

/packages/contracts/src/plugins/utils/Addresslist.sol

• 22, 45.

/packages/contracts/src/plugins/counter-example/v2/CounterV2.sol

• 31, 51, 68.

/packages/contracts/src/plugins/counter-example/v1/CounterV1.sol

• 49.

/packages/contracts/src/plugins/token/MerkleDistributor.sol

• 128.

/packages/contracts/src/plugins/token/IMerkleMinter.sol

• 36.

/packages/contracts/src/plugins/token/IMerkleDistributor.sol

• 6.

/packages/contracts/src/plugins/token/MerkleMinter.sol

• 19, 68, 97.

/packages/contracts/src/core/permission/PermissionManager.sol

• 12, 14, 26, 53, 59, 64, 73, 85, 99, 104, 113, 118, 119, 129, 134, 192, 197, 205, 206, 207, 210, 228, 229, 281, 291, 325, 337, 353, 361.

/packages/contracts/src/core/permission/PermissionLib.sol

• 22, 32, 36.

/packages/contracts/src/core/permission/IPermissionCondition.sol

• 7.

/packages/contracts/src/core/utils/CallbackHandler.sol

• 7, 28, 52.

/packages/contracts/src/core/utils/auth.sol

• 14.

/packages/contracts/src/core/dao/IDAO.sol

• 19, 40, 41, 43, 58, 96, 121, 127.

/packages/contracts/src/core/dao/DAO.sol

• 24, 99, 134, 260, 262, 268, 338.

/packages/contracts/src/core/dao/IEIP4824.sol

• 9.

/packages/contracts/src/core/plugin/PluginUUPSUpgradeable.sol

• 6, 15, 53, 59, 65.

/packages/contracts/src/core/plugin/dao-authorizable/DaoAuthorizableUpgradeable.sol

• 12, 30, 37.

/packages/contracts/src/core/plugin/dao-authorizable/DaoAuthorizable.sol

• 12, 29.

/packages/contracts/src/core/plugin/Plugin.sol

• 13.

/packages/contracts/src/core/plugin/PluginCloneable.sol

• 13.

/packages/contracts/src/core/plugin/proposal/ProposalUpgradeable.sol

• 12, 42, 69, 82.

/packages/contracts/src/core/plugin/proposal/Proposal.sol

• 12, 42, 69.

/packages/contracts/src/core/plugin/proposal/IProposal.sol

• 18.

/packages/contracts/src/token/ERC20/governance/GovernanceWrappedERC20.sol

• 6, 8, 10, 11, 21, 22, 24, 25, 26.

/packages/contracts/src/token/ERC20/governance/IGovernanceWrappedERC20.sol

• 7, 10, 15.

/packages/contracts/src/token/ERC20/governance/GovernanceERC20.sol

• 5, 7, 8, 19.

2. Order of Functions

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

• ENSSubdomainRegistrar.sol: external functions are positioned after internal functions.
• MajorityVotingBase.sol: public functions are positioned after internal functions.
• AddresslistVoting.sol: external functions are positioned after public functions.
• TokenVoting.sol: external functions are positioned after public functions.
• Multisig.sol: external functions are positioned after public functions.
• Admin.sol: external functions are positioned after public functions.
• PluginRepo.sol: public functions are positioned after internal functions.
• PluginSetup.sol: public functions are positioned after internal functions.
• MerkleDistributor.sol: external functions are positioned after public functions.
• MerkleMinter.sol: external functions are positioned after public functions.
• PermissionManager.sol: external functions are positioned after internal functions.
• DAO.sol: external functions are positioned after internal functions.
• PluginUUPSUpgradeable.sol: public functions are positioned after internal functions.
• DaoAuthorizableUpgradeable.sol: public functions are positioned after internal functions.
• PluginCloneable.sol: public functions are positioned after internal functions.
• GovernanceERC20.sol: external functions are positioned after public functions.

3. Whitespace in Expressions

The Solidity Style Guide recommends to "[a]void extraneous whitespace [i]mmediately inside parenthesis, brackets or braces, with the exception of single line function declarations". The Style Guide also recommends against spaces before semicolons. Consider removing spaces before semicolons.

/packages/contracts/src/plugins/governance/majority-voting/addresslist/AddresslistVoting.sol

130:	for (uint256 i; i < _actions.length; ) {

/packages/contracts/src/plugins/governance/majority-voting/token/TokenVoting.sol

124:	for (uint256 i; i < _actions.length; ) {

/packages/contracts/src/plugins/governance/multisig/Multisig.sol

252:	for (uint256 i; i < _actions.length; ) {

/packages/contracts/src/framework/plugin/setup/PluginSetupProcessor.sol

674:	} catch (bytes memory ) {
686:	} catch (bytes memory ) {

/packages/contracts/src/plugins/utils/Addresslist.sol

60:	for (uint256 i; i < _newAddresses.length; ) {
78:	for (uint256 i; i < _exitingAddresses.length; ) {

/packages/contracts/src/core/permission/PermissionManager.sol

150:	for (uint256 i; i < items.length; ) {
170:	for (uint256 i; i < _items.length; ) {

/packages/contracts/src/core/dao/DAO.sol

184:	for (uint256 i = 0; i < _actions.length; ) {

4. Function Declaration Style

The Solidity Style Guide states that "[f]or short function declarations, it is recommended for the opening brace of the function body to be kept on the same line as the function declaration". It is not clear what length is exactly meant by "short" or "long". The maximum line length of 120 characters may be an indication, and in that case any expanded function declaration under 120 characters should be on a single line.

The following functions in their respective contracts are not compliant by this standard:

/packages/contracts/src/framework/utils/InterfaceBasedRegistry.sol

• _authorizeUpgrade.

/packages/contracts/src/framework/utils/ens/ENSSubdomainRegistrar.sol

• _authorizeUpgrade, setDefaultResolver.

/packages/contracts/src/plugins/governance/majority-voting/MajorityVotingBase.sol

• __MajorityVotingBase_init, vote, getVoteOption, canVote, isSupportThresholdReachedEarly.

/packages/contracts/src/plugins/governance/majority-voting/addresslist/AddresslistVoting.sol

• addAddresses, removeAddresses.

/packages/contracts/src/plugins/governance/multisig/Multisig.sol

• addAddresses, removeAddresses.

/packages/contracts/src/framework/dao/DAORegistry.sol

• initialize, register.

/packages/contracts/src/framework/plugin/repo/PluginRepo.sol

• _authorizeUpgrade.

/packages/contracts/src/framework/plugin/repo/PluginRepoFactory.sol

• createPluginRepo.

/packages/contracts/src/framework/plugin/setup/PluginSetupProcessor.sol

• validatePreparedSetupId, _upgradeProxy.

/packages/contracts/src/framework/plugin/setup/PluginSetupProcessorHelpers.sol

• _getAppliedSetupId, hashPermissions.

/packages/contracts/src/framework/plugin/setup/PluginSetup.sol

• createERC1967Proxy.

/packages/contracts/src/plugins/counter-example/MultiplyHelper.sol

• multiply.

/packages/contracts/src/plugins/utils/Addresslist.sol

• isListedAtBlock.

/packages/contracts/src/plugins/counter-example/v1/CounterV1.sol

• initialize.

/packages/contracts/src/plugins/token/MerkleDistributor.sol

• initialize, claim.

/packages/contracts/src/core/permission/PermissionManager.sol

• grant, revoke, isPermissionRestrictedForAnyAddr.

/packages/contracts/src/core/utils/CallbackHandler.sol

• _handleCallback.

/packages/contracts/src/core/utils/auth.sol

• _auth.

/packages/contracts/src/core/dao/DAO.sol

• isPermissionRestrictedForAnyAddr, setMetadata, deposit.

/packages/contracts/src/core/plugin/PluginUUPSUpgradeable.sol

• _authorizeUpgrade.

/packages/contracts/src/token/ERC20/governance/GovernanceWrappedERC20.sol

• initialize, decimals, _mint, _burn.

5. Order of Layout

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

• TokenFactory.sol: State is positioned after Events.
• IDAO.sol: Events are positioned after Functions.
• PluginUUPSUpgradeable.sol: State is positioned after a Function.
• DaoAuthorizableUpgradeable.sol: Modifier is positioned after Functions.
• DaoAuthorizable.sol: Modifier is positioned after Functions.

The following are also techically not compliant; however, understandably use a __gap for reserved space (compliant otherwise):

• ENSSubdomainRegistrar.sol, MajorityVotingBase.sol, TokenVoting.sol, Multisig.sol, DAORegistry.sol, PluginRepoRegistry.sol, Addresslist.sol, CounterV2.sol, CounterV1.sol, MerkleDistributor.sol, MerkleMinter.sol, PermissionManager.sol, DAO.sol, ProposalUpgradeable.sol, InterfaceBasedRegistry.sol, AddresslistVoting.sol, CallbackHandler.sol.