Aragon Protocol contest - tnevler's results

Report

Non-Critical Issues

[N-1]: Function defines a named return variable but then it uses return statements

Context:

1. return (plugin, preparedSetupData); L343
2. return (initData, preparedSetupData); L494
3. return (plugin, preparedSetupData); L96
4. return IMerkleDistributor(distributorAddr); L94

Recommendation:

Choose named return variable or return statement. It is unnecessary to use both.

[N-2]: Use of immutable instead of constant keccak expression

Context:

1. bytes32 public constant EXECUTE_PERMISSION_ID = keccak256("EXECUTE_PERMISSION"); L40
2. bytes32 public constant UPGRADE_DAO_PERMISSION_ID = keccak256("UPGRADE_DAO_PERMISSION"); L43
3. bytes32 public constant SET_METADATA_PERMISSION_ID = keccak256("SET_METADATA_PERMISSION"); L46
4. bytes32 public constant SET_TRUSTED_FORWARDER_PERMISSION_ID = L49
5. bytes32 public constant SET_SIGNATURE_VALIDATOR_PERMISSION_ID = L53
6. bytes32 public constant REGISTER_STANDARD_CALLBACK_PERMISSION_ID = L57
7. bytes32 public constant ROOT_PERMISSION_ID = keccak256("ROOT_PERMISSION"); L15
8. bytes32 public constant UPGRADE_PLUGIN_PERMISSION_ID = keccak256("UPGRADE_PLUGIN_PERMISSION"); L35
9. bytes32 public constant REGISTER_DAO_PERMISSION_ID = keccak256("REGISTER_DAO_PERMISSION"); L15
10. bytes32 public constant MAINTAINER_PERMISSION_ID = keccak256("MAINTAINER_PERMISSION"); L49
11. bytes32 public constant UPGRADE_REPO_PERMISSION_ID = keccak256("UPGRADE_REPO_PERMISSION"); L52
12. bytes32 public constant REGISTER_PLUGIN_REPO_PERMISSION_ID = L16
13. bytes32 public constant APPLY_INSTALLATION_PERMISSION_ID = L27
14. bytes32 public constant APPLY_UPDATE_PERMISSION_ID = keccak256("APPLY_UPDATE_PERMISSION"); L31
15. bytes32 public constant APPLY_UNINSTALLATION_PERMISSION_ID = L34
16. bytes32 public constant UPGRADE_REGISTRAR_PERMISSION_ID = L18
17. bytes32 public constant REGISTER_ENS_SUBDOMAIN_PERMISSION_ID = L22
18. bytes32 public constant UPGRADE_REGISTRY_PERMISSION_ID = L18
19. bytes32 public constant MULTIPLY_PERMISSION_ID = keccak256("MULTIPLY_PERMISSION"); L14
20. bytes32 public constant MULTIPLY_PERMISSION_ID = keccak256("MULTIPLY_PERMISSION"); L12
21. bytes32 public constant EXECUTE_PROPOSAL_PERMISSION_ID = L23
22. bytes32 public constant UPDATE_ADDRESSES_PERMISSION_ID = L27
23. bytes32 public constant UPDATE_VOTING_SETTINGS_PERMISSION_ID = L183
24. bytes32 public constant UPDATE_MULTISIG_SETTINGS_PERMISSION_ID = L71
25. bytes32 public constant CHANGE_DISTRIBUTOR_PERMISSION_ID = L27

Description:

According to official solidity documentation for a constant variable, the expression assigned to it is copied to all the places where it is accessed and also re-evaluated each time. It is recommended to use immutable instead.

[N-3]: Follow Solidity standard naming conventions

Context:

1. uint256 internal constant MAX_ACTIONS = 256; L61 (MAX_ACTIONS should start with _)
2. address private trustedForwarder; L67 (trustedForwarder should start with _)
3. function isPermissionRestrictedForAnyAddr( L122 (isPermissionRestrictedForAnyAddr should start with _)
4. address internal constant ANY_ADDR = address(type(uint160).max); L18 (ANY_ADDR should start with _)
5. address internal constant UNSET_FLAG = address(0); L21 (UNSET_FLAG should start with _)
6. address internal constant ALLOW_FLAG = address(2); L24 (ALLOW_FLAG should start with _)
7. mapping(bytes32 => address) internal permissionsHashed; L27 (permissionsHashed should start with _)
8. function __PermissionManager_init(address _initialOwner) internal onlyInitializing { L95 (Function name must be in mixedCase)
9. function permissionHash( L342 (permissionHash should start with _)
10. function isPermissionRestrictedForAnyAddr( L354 (isPermissionRestrictedForAnyAddr should start with _)
11. IDAO private immutable dao_; L15 (dao_ should start with _)
12. CountersUpgradeable.Counter private proposalCounter; L17 (proposalCounter should start with _)
13. function __PluginCloneable_init(IDAO _dao) internal virtual onlyInitializing { L22 (Function name must be in mixedCase)
14. mapping(bytes4 => bytes4) internal callbackMagicNumbers; L11 (callbackMagicNumbers should start with _)
15. bytes4 internal constant UNREGISTERED_CALLBACK = bytes4(0); L14 (UNREGISTERED_CALLBACK should start with _)
16. function _auth( L19 (_auth should not start with _)
17. mapping(uint8 => uint16) internal buildsPerRelease; L55 (buildsPerRelease should start with _)
18. mapping(bytes32 => Version) internal versions; L58 (versions should start with _)
19. mapping(address => bytes32) internal latestTagHashForPluginSetup; L61 (latestTagHashForPluginSetup should start with _)
20. function tagHash(Tag memory _tag) internal pure returns (bytes32) { L251 (tagHash should start with _)
21. function createERC1967Proxy( L33 (createERC1967Proxy should start with _)
22. function _getPluginInstallationId(address _dao, address _plugin) pure returns (bytes32) { L32 (_getPluginInstallationId should not start with _)
23. function _getPreparedSetupId( L43 (_getPreparedSetupId should not start with _)
24. function _getAppliedSetupId( L67 (_getAppliedSetupId should not start with _)
25. function __InterfaceBasedRegistry_init( L43 (Function name must be in mixedCase)
26. function setupBases() private { L144 (setupBases should start with _)
27. bytes4 internal constant ADMIN_INTERFACE_ID = L19 (ADMIN_INTERFACE_ID should start with _)
28. bytes4 internal constant ADDRESSLIST_VOTING_INTERFACE_ID = L23 (ADDRESSLIST_VOTING_INTERFACE_ID should start with _)
29. bytes4 internal constant TOKEN_VOTING_INTERFACE_ID = L22 (TOKEN_VOTING_INTERFACE_ID should start with _)
30. IVotesUpgradeable private votingToken; L26 (votingToken should start with _)
31. bytes4 internal constant MULTISIG_INTERFACE_ID = L64 (MULTISIG_INTERFACE_ID should start with _)
32. mapping(uint256 => Proposal) internal proposals; L75 (proposals should start with _)
33. Multisig private immutable multisigBase; L16 (multisigBase should start with _)
34. function _applyRatioCeiled(uint256 _value, uint256 _ratio) pure returns (uint256 result) { L17 (_applyRatioCeiled should not start with _)

Description:

The above codes don't follow Solidity's standard naming convention.

• Internal and private functions should use mixedCase format starting with an underscore.
• Structs should be named using the CapWords style. Examples: MyCoin, Position, PositionXY.
• Events should be named using the CapWords style. Examples: Deposit, Transfer, Approval, BeforeTransfer, AfterTransfer.
• Functions should use mixedCase. Examples: getBalance, transfer, verifyOwner, addMember, changeOwner.
• Function arguments should use mixedCase. Examples: initialSupply, account, recipientAddress, senderAddress, newOwner.
• Local and State Variable should use mixedCase. Examples: totalSupply, remainingSupply, balancesOf, creatorAddress, isPreSale, tokenExchangeRate.
• Constants should be named with all capital letters with underscores separating words. Examples: MAX_BLOCKS, TOKEN_NAME, TOKEN_TICKER, CONTRACT_VERSION.
• Modifier should use mixedCase. Examples: onlyBy, onlyAfter, onlyDuringThePreSale.
• Enums, in the style of simple type declarations, should be named using the CapWords style. Examples: TokenGroup, Frame, HashStyle, CharacterLocation.

[N-4]: Wrong order of functions

Context:

1. modifier auth(bytes32 _permissionId) { L31 (modifier definition can not go after public function)
2. function pluginType() public pure override returns (PluginType) { L27 (public function can not go after internal function)
3. function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) { L43 (public function can not go after internal function)
4. function isMember(address _account) external view returns (bool) { L48 (external function can not go after public function)
5. function addAddresses( L59 (external function can not go after public function)
6. function merkleMint( L74 (external function can not go after public function)

Description:

According to official solidity documentation functions should be grouped according to their visibility and ordered:

• constructor

• receive function (if exists)

• fallback function (if exists)

• external

• public

• internal

• private

Within a grouping, place the view and pure functions last.

Recommendation:

Put the functions in the correct order according to the documentation.

[N-5]: Line is too long

Context:

1. /// @notice Internal method authorizing the upgrade of the contract via the [upgradeabilty mechanism for UUPS proxies](https://docs.openzeppelin.com/contracts/4.x/api/proxy#UUPSUpgradeable) (see [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822)). L134
2. /// @notice Emits the `NativeTokenDeposited` event to track native token deposits that weren't made via the deposit method. L260
3. /// Gas cost increases in future hard forks might break this function. As an alternative, EIP-2930-type transactions using access lists can be employed. L262
4. /// @param _input An alias being equivalent to `msg.data`. This feature of the fallback function was introduced with the [solidity compiler version 0.7.6](https://github.com/ethereum/solidity/releases/tag/v0.7.6) L268
5. /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)). L338
6. /// @notice A distinct Uniform Resource Identifier (URI) pointing to a JSON object following the "EIP-4824 DAO JSON-LD Schema". This JSON file splits into four URIs: membersURI, proposalsURI, activityLogURI, and governanceURI. The membersURI should point to a JSON file that conforms to the "EIP-4824 Members JSON-LD Schema". The proposalsURI should point to a JSON file that conforms to the "EIP-4824 Proposals JSON-LD Schema". The activityLogURI should point to a JSON file that conforms to the "EIP-4824 Activity Log JSON-LD Schema". The governanceURI should point to a flatfile, normatively a .md file. Each of the JSON files named above can be statically-hosted or dynamically-generated. L9
7. /// @notice This interface can be implemented to support more customary permissions depending on on- or off-chain state, e.g., by querying token ownershop or a secondary condition, respectively. L7
8. /// @notice A mapping storing permissions as hashes (i.e., `permissionHash(where, who, permissionId)`) and their status encoded by an address (unset, allowed, or redirecting to a `PermissionCondition`). L26
9. /// @notice Thrown for `ROOT_PERMISSION_ID` or `EXECUTE_PERMISSION_ID` permission grants where `who` or `where` is `ANY_ADDR`. L53
10. /// @notice Emitted when a permission `permission` is granted in the context `here` to the address `_who` for the contract `_where`. L59
11. /// @param condition The address `ALLOW_FLAG` for regular permissions or, alternatively, the `PermissionCondition` to be used. L64
12. /// @notice Emitted when a permission `permission` is revoked in the context `here` from the address `_who` for the contract `_where`. L73
13. /// @notice A modifier to make functions on inheriting contracts authorized. Permissions to call the function are checked through this permission manager. L85
14. /// @notice Grants permission to an address to call methods in a contract guarded by an auth modifier with the specified permission identifier. L99
15. /// @dev Note, that granting permissions with `_who` or `_where` equal to `ANY_ADDR` does not replace other permissions with specific `_who` and `_where` addresses that exist in parallel. L104
16. /// @notice Grants permission to an address to call methods in a target contract guarded by an auth modifier with the specified permission identifier if the referenced condition permits it. L113
17. /// @param _condition The `PermissionCondition` that will be asked for authorization on calls connected to the specified permission identifier. L118
18. /// @dev Note, that granting permissions with `_who` or `_where` equal to `ANY_ADDR` does not replace other permissions with specific `_who` and `_where` addresses that exist in parallel. L119
19. /// @notice Revokes permission from an address to call methods in a target contract guarded by an auth modifier with the specified permission identifier. L129
20. /// @dev Note, that revoking permissions with `_who` or `_where` equal to `ANY_ADDR` does not revoke other permissions with specific `_who` and `_where` addresses that exist in parallel. L134
21. /// @notice Checks if an address has permission on a contract via a permission identifier and considers if `ANY_ADDRESS` was used in the granting process. L192
22. /// @return Returns true if `_who` has the permissions on the target contract via the specified permission identifier. L197
23. /// @param _condition An address either resolving to a `PermissionCondition` contract address or being the `ALLOW_FLAG` address (`address(2)`). L228
24. /// @dev Note, that granting permissions with `_who` or `_where` equal to `ANY_ADDR` does not replace other permissions with specific `_who` and `_where` addresses that exist in parallel. L229
25. /// @dev Note, that revoking permissions with `_who` or `_where` equal to `ANY_ADDR` does not revoke other permissions with specific `_who` and `_where` addresses that might have been granted in parallel. L281
26. /// @notice Checks if a caller is granted permissions on a target contract via a permission identifier and redirects the approval to a `PermissionCondition` if this was specified in the setup. L291
27. /// @notice Generates the hash for the `permissionsHashed` mapping obtained from the word "PERMISSION", the contract address, the address owning the permission, and the permission identifier. L337
28. /// @dev By default, every permission is unrestricted and it is the derived contract's responsibility to override it. Note, that the `ROOT_PERMISSION_ID` is included not required to be set it again. L353
29. /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)). L361
30. /// @notice A modifier to make functions on inheriting contracts authorized. Permissions to call the function are checked through the associated DAO's permission manager. L29
31. /// @param allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert. L18
32. /// @notice An abstract contract containing the traits and internal functionality to create and execute proposals that can be inherited by upgradeable DAO plugins. L12
33. /// @param _allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert. L42
34. /// @param _allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert. L69
35. /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)). L82
36. /// @notice An abstract, non-upgradeable contract to inherit from when creating a plugin being deployed via the minimal clones pattern (see [ERC-1167](https://eips.ethereum.org/EIPS/eip-1167)). L13
37. /// @notice This contract handles callbacks by registering a magic number together with the callback function's selector. It provides the `_handleCallback` function that inherting have to call inside their `fallback()` function (`_handleCallback(msg.callbackSelector, msg.data)`). This allows to adaptively register ERC standards (e.g., [ERC-721](https://eips.ethereum.org/EIPS/eip-721), [ERC-1115](https://eips.ethereum.org/EIPS/eip-1155), or future versions of [ERC-165](https://eips.ethereum.org/EIPS/eip-165)) and returning the required magic numbers for the associated callback functions for the inheriting contract so that it doesn't need to be upgraded. L7
38. /// @dev This function is supposed to be called via `_handleCallback(msg.sig, msg.data)` in the `fallback()` function of the inheriting contract. L28
39. /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)). L52
40. /// @notice A free function checking if a caller is granted permissions on a target contract via a permission identifier that redirects the approval to a `PermissionCondition` if this was specified in the setup. L14
41. /// @param data The bytes-encoded data containing the input parameters for the installation as specified in the plugin's build metadata JSON file. L41
42. /// @notice Creates a new plugin version as the latest build for an existing release number or the first build for a new release number for the provided `PluginSetup` contract address and metadata. L14
43. /// @notice Internal method authorizing the upgrade of the contract via the [upgradeabilty mechanism for UUPS proxies](https://docs.openzeppelin.com/contracts/4.x/api/proxy#UUPSUpgradeable) (see [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822)). L255
44. /// @notice The interface required for a plugin setup contract to be consumed by the `PluginSetupProcessor` for plugin installations, updates, and uninstallations. L10
45. /// @param helpers The address array of helpers (contracts or EOAs) associated with this plugin version after the installation or update. L13
46. /// @param permissions The array of multi-targeted permission operations to be applied by the `PluginSetupProcessor` to the installing or updating DAO. L14
47. /// @notice The payload for plugin updates and uninstallations containing the existing contracts as well as optional data to be consumed by the plugin setup. L20
48. /// @param data The bytes-encoded data containing the input parameters for the preparation of update/uninstall as specified in the corresponding ABI on the version's metadata. L23
49. /// @param _data The bytes-encoded data containing the input parameters for the installation as specified in the plugin's build metadata JSON file. L32
50. /// @return initData The initialization data to be passed to upgradeable contracts when the update is applied in the `PluginSetupProcessor`. L44
51. /// @return permissions The array of multi-targeted permission operations to be applied by the `PluginSetupProcessor` to the uninstalling DAO. L55
52. /// @dev The implementation can be instantiated via the `new` keyword, cloned via the minimal clones pattern (see [ERC-1167](https://eips.ethereum.org/EIPS/eip-1167)), or proxied via the UUPS pattern (see [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822)). L63
53. /// @notice A convenience function to create an [ERC-1967](https://eips.ethereum.org/EIPS/eip-1967) proxy contract pointing to an implementation and being associated to a DAO. L29
54. /// @param _preparationType The type of preparation the plugin is currently undergoing. Without this, it is possible to call `applyUpdate` even after `applyInstallation` is called. L41
55. /// @notice Internal method authorizing the upgrade of the contract via the [upgradeabilty mechanism for UUPS proxies](https://docs.openzeppelin.com/contracts/4.x/api/proxy#UUPSUpgradeable) (see [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822)). L52
56. /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)). L74
57. /// @notice Creates a new `GovernanceERC20` token or a `GovernanceWrappedERC20` from an existing [ERC-20](https://eips.ethereum.org/EIPS/eip-20) token depending on the address used in the `TokenConfig` provided. L72
58. /// @param _tokenConfig The token configuration struct containing the name, and symbol of the token to be create, but also an address. For `address(0)`, a new governance token is created. For any other address pointing to an [ERC-20](https://eips.ethereum.org/EIPS/eip-20)-compatible contract, a wrapped governance token is created. L74
59. /// @return The created `MerkleMinter` contract used to mint the `ERC20VotesUpgradeable` tokens or `address(0)` if an existing token was provided. L77
60. /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)). L49
61. /// @param _allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert. L61
62. /// @notice The majority voting implementation using an [OpenZepplin `Votes`](https://docs.openzeppelin.com/contracts/4.x/api/governance#Votes) compatible governance token. L16
63. /// @notice Checks if the support value defined as $$\texttt{support} = \frac{N_\text{yes}}{N_\text{yes}+N_\text{no}}$$ for a proposal vote is greater than the support threshold. L44
64. /// @notice Checks if the worst-case support value defined as $$\texttt{worstCaseSupport} = \frac{N_\text{yes}}{ N_\text{total}-N_\text{abstain}}$$ for a proposal vote is greater than the support threshold. L49
65. /// @notice Checks if the participation value defined as $$\texttt{participation} = \frac{N_\text{yes}+N_\text{no}+N_\text{abstain}}{N_\text{total}}$$ for a proposal vote is greater or equal than the minimum participation value. L54
66. /// @param _allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert. L33
67. /// @param _allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert. L201
68. /// @param allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert. L304
69. /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)). L97

Description:

Maximum suggested line length is 120 characters.