Aragon Protocol contest - 0xSmartContract's results

Summary

Low Risk Issues List

Total 10 issues

Non-Critical Issues List

Total 20 issues

Suggestions

[L-01] Insufficient coverage

Description: The test coverage rate of the project is ~60%. Testing all functions is best practice in terms of security criteria. Due to its capacity, test coverage is expected to be 100%.

1 result - 1 file

README.md:
  212: - What is the overall line coverage percentage provided by your tests?:  60

[L-02] No Storage Gap for Admin.sol

src/plugins/governance/admin/Admin.sol:
  15: contract Admin is IMembership, PluginCloneable, ProposalUpgradeable {

Impact

For upgradeable contracts, inheriting contracts may introduce new variables. In order to be able to add new variables to the upgradeable contract without causing storage collisions, a storage gap should be added to the upgradeable contract.

If no storage gap is added, when the upgradable contract introduces new variables, it may override the variables in the inheriting contract.

Storage gaps are a convention for reserving storage slots in a base contract, allowing future versions of that contract to use up those slots without affecting the storage layout of child contracts. To create a storage gap, declare a fixed-size array in the base contract with an initial number of slots. This can be an array of uint256 so that each element reserves a 32 byte slot. Use the naming convention __gap so that OpenZeppelin Upgrades will recognize the gap:

Classification for a similar problem: https://code4rena.com/reports/2022-05-alchemix/#m-05-no-storage-gap-for-upgradeable-contract-might-lead-to-storage-slot-collision

contract Base {
    uint256 base1;
    uint256[49] __gap;
}

contract Child is Base {
    uint256 child;
}

Openzeppelin Storage Gaps notification:

Storage Gaps
This makes the storage layouts incompatible, as explained in Writing Upgradeable Contracts. 
The size of the __gap array is calculated so that the amount of storage used by a contract 
always adds up to the same number (in this case 50 storage slots).

Recommended Mitigation Steps

Consider adding a storage gap at the end of the upgradeable contract

uint256[50] private __gap;

[L-03] Project Upgrade and Stop Scenario should be

At the start of the project, the system may need to be stopped or upgraded, I suggest you have a script beforehand and add it to the documentation. This can also be called an " EMERGENCY STOP (CIRCUIT BREAKER) PATTERN ".

https://github.com/maxwoe/solidity_patterns/blob/master/security/EmergencyStop.sol

[L-04] Use Fuzzing Test for complicated code bases

I recommend fuzzing testing in complex code structures, especially Aragon, where there is an innovative code base and a lot of computation.

Recommendation:

Use should fuzzing test like Echidna.

As Alberto Cuesta Canada said:

Fuzzing is not easy, the tools are rough, and the math is hard, but it is worth it. Fuzzing gives me a level of confidence in my smart contracts that I didn’t have before. Relying just on unit testing anymore and poking around in a testnet seems reckless now.

https://medium.com/coinmonks/smart-contract-fuzzing-d9b88e0b0a05

[L-05] Add to Event-Emit for critical functions

6 result


src/framework/utils/TokenFactory.sol:
  144:     function setupBases() private {
  145:         distributorBase = new MerkleDistributor();
  146:         governanceERC20Base = address(
  147:             new GovernanceERC20(
  148:                 IDAO(address(0)),
  149:                 "baseName",
  150:                 "baseSymbol",
  151:                 GovernanceERC20.MintSettings(new address[](0), new uint256[](0))
  152:             )
  153:         );
  154:         governanceWrappedERC20Base = address(
  155:             new GovernanceWrappedERC20(IERC20Upgradeable(address(0)), "baseName", "baseSymbol")
  156:         );
  157:         merkleMinterBase = address(new MerkleMinter());
  158:     }

src/framework/utils/ens/ENSSubdomainRegistrar.sol:
  100:     function setDefaultResolver(
  101:         address _resolver
  102:     ) external auth(REGISTER_ENS_SUBDOMAIN_PERMISSION_ID) {
  103:         if (_resolver == address(0)) {
  104:             revert InvalidResolver({node: node, resolver: _resolver});
  105:         }
  106: 
  107:         resolver = _resolver;
  108:     }


src/framework/utils/TokenFactory.sol:
  144:     function setupBases() private {
  145:         distributorBase = new MerkleDistributor();
  146:         governanceERC20Base = address(
  147:             new GovernanceERC20(
  148:                 IDAO(address(0)),
  149:                 "baseName",
  150:                 "baseSymbol",
  151:                 GovernanceERC20.MintSettings(new address[](0), new uint256[](0))
  152:             )
  153:         );
  154:         governanceWrappedERC20Base = address(
  155:             new GovernanceWrappedERC20(IERC20Upgradeable(address(0)), "baseName", "baseSymbol")
  156:         );
  157:         merkleMinterBase = address(new MerkleMinter());
  158:     }


src/framework/utils/ens/ENSSubdomainRegistrar.sol:
  82:     function registerSubnode(
  83:         bytes32 _label,
  84:         address _targetAddress
  85:     ) external auth(REGISTER_ENS_SUBDOMAIN_PERMISSION_ID) {
  86:         bytes32 subnode = keccak256(abi.encodePacked(node, _label));
  87:         address currentOwner = ens.owner(subnode);
  88: 
  89:         if (currentOwner != address(0)) {
  90:             revert AlreadyRegistered(subnode, currentOwner);
  91:         }
  92: 
  93:         ens.setSubnodeOwner(node, _label, address(this));
  94:         ens.setResolver(subnode, resolver);
  95:         Resolver(resolver).setAddr(subnode, _targetAddress);
  96:     }


src/core/plugin/dao-authorizable/DaoAuthorizableUpgradeable.sol:
  20:     function __DaoAuthorizableUpgradeable_init(IDAO _dao) internal onlyInitializing {
  21:         dao_ = _dao;
  22:     }


src/plugins/governance/multisig/MultisigSetup.sol:
  24:     function prepareInstallation(
  25:         address _dao,
  26:         bytes calldata _data
  27:     ) external returns (address plugin, PreparedSetupData memory preparedSetupData) {
  28:         // Decode `_data` to extract the params needed for deploying and initializing `Multisig` plugin.
  29:         (address[] memory members, Multisig.MultisigSettings memory multisigSettings) = abi.decode(
  30:             _data,
  31:             (address[], Multisig.MultisigSettings)
  32:         );
  33: 
  34:         // Prepare and Deploy the plugin proxy.
  35:         plugin = createERC1967Proxy(
  36:             address(multisigBase),
  37:             abi.encodeWithSelector(Multisig.initialize.selector, _dao, members, multisigSettings)
  38:         );
  39: 
  40:         // Prepare permissions
  41:         PermissionLib.MultiTargetPermission[]
  42:             memory permissions = new PermissionLib.MultiTargetPermission[](3);
  43: 
  44:         // Set permissions to be granted.
  45:         // Grant the list of prmissions of the plugin to the DAO.
  46:         permissions[0] = PermissionLib.MultiTargetPermission(
  47:             PermissionLib.Operation.Grant,
  48:             plugin,
  49:             _dao,
  50:             PermissionLib.NO_CONDITION,
  51:             multisigBase.UPDATE_MULTISIG_SETTINGS_PERMISSION_ID()
  52:         );
  53: 
  54:         permissions[1] = PermissionLib.MultiTargetPermission(
  55:             PermissionLib.Operation.Grant,
  56:             plugin,
  57:             _dao,
  58:             PermissionLib.NO_CONDITION,
  59:             multisigBase.UPGRADE_PLUGIN_PERMISSION_ID()
  60:         );
  61: 
  62:         // Grant `EXECUTE_PERMISSION` of the DAO to the plugin.
  63:         permissions[2] = PermissionLib.MultiTargetPermission(
  64:             PermissionLib.Operation.Grant,
  65:             _dao,
  66:             plugin,
  67:             PermissionLib.NO_CONDITION,
  68:             DAO(payable(_dao)).EXECUTE_PERMISSION_ID()
  69:         );
  70: 
  71:         preparedSetupData.permissions = permissions;
  72:     }

[L-06] ProposoalCreated events are missing parameters

The Proposal.sol and ProposolUpgradable.sol contracts have very important function; _createProposal

However, only amounts are published in emits, whereas msg.sender must be specified in every transaction, a contract or web page listening to events cannot react to users, emit does not serve the purpose. Basically, this event cannot be used

2 results - 2 files

src/core/plugin/proposal/Proposal.sol:
  45:     function _createProposal(
  46:         address _creator,
  47:         bytes calldata _metadata,
  48:         uint64 _startDate,
  49:         uint64 _endDate,
  50:         IDAO.Action[] calldata _actions,
  51:         uint256 _allowFailureMap
  52:     ) internal virtual returns (uint256 proposalId) {
  53:         proposalId = _createProposalId();
  54: 
  55:         emit ProposalCreated({
  56:             proposalId: proposalId,
  57:             creator: _creator,
  58:             metadata: _metadata,
  59:             startDate: _startDate,
  60:             endDate: _endDate,
  61:             actions: _actions,
  62:             allowFailureMap: _allowFailureMap
  63:         });
  64:     }

src/core/plugin/proposal/ProposalUpgradeable.sol:
  45:     function _createProposal(
  46:         address _creator,
  47:         bytes calldata _metadata,
  48:         uint64 _startDate,
  49:         uint64 _endDate,
  50:         IDAO.Action[] calldata _actions,
  51:         uint256 _allowFailureMap
  52:     ) internal virtual returns (uint256 proposalId) {
  53:         proposalId = _createProposalId();
  54: 
  55:         emit ProposalCreated({
  56:             proposalId: proposalId,
  57:             creator: _creator,
  58:             metadata: _metadata,
  59:             startDate: _startDate,
  60:             endDate: _endDate,
  61:             actions: _actions,
  62:             allowFailureMap: _allowFailureMap
  63:         });
  64:     }

Recommended Mitigation Steps

add msg.sender parameter in event-emit

[L-07] Keccak Constant values should used to immutable rather than constant

There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts.

While it doesn't save any gas because the compiler knows that developers often make this mistake, it's still best to use the right tool for the task at hand.

14 results 

src/core/dao/DAO.sol:
  40:     bytes32 public constant EXECUTE_PERMISSION_ID = keccak256("EXECUTE_PERMISSION");
  43:     bytes32 public constant UPGRADE_DAO_PERMISSION_ID = keccak256("UPGRADE_DAO_PERMISSION");
  46:     bytes32 public constant SET_METADATA_PERMISSION_ID = keccak256("SET_METADATA_PERMISSION");


src/core/permission/PermissionManager.sol:
  15:     bytes32 public constant ROOT_PERMISSION_ID = keccak256("ROOT_PERMISSION");

src/core/plugin/PluginUUPSUpgradeable.sol:
  35:     bytes32 public constant UPGRADE_PLUGIN_PERMISSION_ID = keccak256("UPGRADE_PLUGIN_PERMISSION");

src/framework/dao/DAORegistry.sol:
  15:     bytes32 public constant REGISTER_DAO_PERMISSION_ID = keccak256("REGISTER_DAO_PERMISSION");

src/framework/plugin/repo/PluginRepo.sol:
  49:     bytes32 public constant MAINTAINER_PERMISSION_ID = keccak256("MAINTAINER_PERMISSION");
  52:     bytes32 public constant UPGRADE_REPO_PERMISSION_ID = keccak256("UPGRADE_REPO_PERMISSION");

src/framework/plugin/setup/PluginSetupProcessor.sol:
  31:     bytes32 public constant APPLY_UPDATE_PERMISSION_ID = keccak256("APPLY_UPDATE_PERMISSION");

src/plugins/counter-example/MultiplyHelper.sol:
  12:     bytes32 public constant MULTIPLY_PERMISSION_ID = keccak256("MULTIPLY_PERMISSION");

src/plugins/counter-example/v1/CounterV1.sol:
  14:     bytes32 public constant MULTIPLY_PERMISSION_ID = keccak256("MULTIPLY_PERMISSION");

src/plugins/counter-example/v2/CounterV2.sol:
  14:     bytes32 public constant MULTIPLY_PERMISSION_ID = keccak256("MULTIPLY_PERMISSION");

src/plugins/token/MerkleMinter.sol:
  24:     bytes32 public constant MERKLE_MINT_PERMISSION_ID = keccak256("MERKLE_MINT_PERMISSION");

src/token/ERC20/governance/GovernanceERC20.sol:
  28:     bytes32 public constant MINT_PERMISSION_ID = keccak256("MINT_PERMISSION");

[L-08] Draft Openzeppelin Dependencies

The PluginUUPSUpgradeable.sol contracts utilised draft-IERC1822Upgradeable.sol an OpenZeppelin contract. This contract is still a draft and are not considered ready for mainnet use. OpenZeppelin contracts may be considered draft contracts if they have not received adequate security auditing or are liable to change with future development.

1 result - 1 file

src/core/plugin/PluginUUPSUpgradeable.sol:
  6: import {IERC1822ProxiableUpgradeable} from "@openzeppelin/contracts-upgradeable/interfaces/draft-IERC1822Upgradeable.sol";

[L-09] Missing Event for initialize

Context:

15 results - 15 files

src/core/dao/DAO.sol:
  104:     function initialize(

src/framework/dao/DAORegistry.sol:
  37:     function initialize(

src/framework/plugin/repo/PluginRepo.sol:
  120:     function initialize(address initialOwner) external initializer {

src/framework/plugin/repo/PluginRepoRegistry.sol:
  41:     function initialize(IDAO _dao, ENSSubdomainRegistrar _subdomainRegistrar) external initializer {

src/framework/utils/ens/ENSSubdomainRegistrar.sol:
  57:     function initialize(IDAO _managingDao, ENS _ens, bytes32 _node) external initializer {

src/plugins/counter-example/v1/CounterV1.sol:
  26:     function initialize(

src/plugins/counter-example/v2/CounterV2.sol:
  32:     function initialize(

src/plugins/governance/admin/Admin.sol:
  29:     function initialize(IDAO _dao) external initializer {

src/plugins/governance/majority-voting/addresslist/AddresslistVoting.sol:
  34:     function initialize(

src/plugins/governance/majority-voting/token/TokenVoting.sol:
  36:     function initialize(

src/plugins/governance/multisig/Multisig.sol:
  125:     function initialize(

src/plugins/token/MerkleDistributor.sol:
  46:     function initialize(

src/plugins/token/MerkleMinter.sol:
  41:     function initialize(

src/token/ERC20/governance/GovernanceERC20.sol:
  63:     function initialize(

src/token/ERC20/governance/GovernanceWrappedERC20.sol:
  46:     function initialize(

Description: Events help non-contract tools to track changes, and events prevent users from being surprised by changes Issuing event-emit during initialization is a detail that many projects skip

Recommendation: Add Event-Emit

[L-10] Exact number of days in a year

The true length of a year on Earth is 365.2422 days, or about 365.25 days We keep our calendar in sync with the seasons by having most years 365 days long but making just under 1/4 of all years 366-day "leap" years

2 results - 1 file

src/plugins/governance/majority-voting/MajorityVotingBase.sol:
  543  
- 544:         if (_votingSettings.minDuration > 365 days) {
+ 544:         if (_votingSettings.minDuration > 365.2422 days) {

- 545:             revert MinDurationOutOfBounds({limit: 365 days, actual: _votingSettings.minDuration});
+ 545:             revert MinDurationOutOfBounds({limit: 365.2422 days, actual: _votingSettings.minDuration});
  546          }

[N-01] Omissions in Events

Throughout the codebase, events are generally emitted when sensitive changes are made to the contracts. However, some events are missing important parameters

The events should include the new value and old value where possible:

src/core/dao/DAO.sol:
  325      /// @param newDaoURI The new DAO uri to be set.
  326:     function setDaoURI(string calldata newDaoURI) external auth(SET_METADATA_PERMISSION_ID) {
  327:         _setDaoURI(newDaoURI);
  328:     }
  329: 
  330:     /// @notice Sets the new DAO uri and emits the associated event.
  331:     /// @param daoURI_ The new DAO uri.
  332:     function _setDaoURI(string calldata daoURI_) internal {
  333:         _daoURI = daoURI_;
  334: 
  335:         emit NewURI(daoURI_);
  336:     }

[N-02] Function writing that does not comply with the Solidity Style Guide

Context: All Contracts

Description: Order of Functions; ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.

https://docs.soliditylang.org/en/v0.8.17/style-guide.html

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

[N-03] For modern and more readable code; update import usages

Context:

20 results - 7 files

src/core/dao/DAO.sol:
  
   5: import "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165StorageUpgradeable.sol";
   6: import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
   7: import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
   8: import "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";
   9: import "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol";
  10: import "@openzeppelin/contracts-upgradeable/token/ERC721/IERC721ReceiverUpgradeable.sol";
  11: import "@openzeppelin/contracts-upgradeable/token/ERC1155/IERC1155Upgradeable.sol";
  12: import "@openzeppelin/contracts-upgradeable/token/ERC1155/IERC1155ReceiverUpgradeable.sol";
  13: import "@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol";
  14: import "@openzeppelin/contracts/interfaces/IERC1271.sol";
  

src/core/permission/PermissionManager.sol:
  
  5: import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
  7: import "./IPermissionCondition.sol";
  8: import "./PermissionLib.sol";

src/core/plugin/proposal/Proposal.sol:
  8: import "./IProposal.sol";

src/core/plugin/proposal/ProposalUpgradeable.sol:
  8: import "./IProposal.sol";

src/framework/utils/ens/ENSMigration.sol:
  7: import "@ensdomains/ens-contracts/contracts/registry/ENSRegistry.sol";
  8: import "@ensdomains/ens-contracts/contracts/resolvers/PublicResolver.sol";

src/framework/utils/ens/ENSSubdomainRegistrar.sol:
  5: import "@ensdomains/ens-contracts/contracts/registry/ENS.sol";
  6: import "@ensdomains/ens-contracts/contracts/resolvers/Resolver.sol";
 
src/utils/Proxy.sol:
  5: import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";

Description: Solidity code is also cleaner in another way that might not be noticeable: the struct Point. We were importing it previously with global import but not using it. The Point struct polluted the source code with an unnecessary object we were not using because we did not need it. This was breaking the rule of modularity and modular programming: only import what you need Specific imports with curly braces allow us to apply this rule better.

Recommendation: import {contract1 , contract2} from "filename.sol";

A good example from the ArtGobblers project;

import {Owned} from "solmate/auth/Owned.sol";
import {ERC721} from "solmate/tokens/ERC721.sol";
import {LibString} from "solmate/utils/LibString.sol";
import {MerkleProofLib} from "solmate/utils/MerkleProofLib.sol";
import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";
import {ERC1155, ERC1155TokenReceiver} from "solmate/tokens/ERC1155.sol";
import {toWadUnsafe, toDaysWadUnsafe} from "solmate/utils/SignedWadMath.sol";

[N-04] Implement some type of version counter that will be incremented automatically for contract upgrades

As part of the upgradeability of Proxies , the contract can be upgraded multiple times, where it is a systematic approach to record the version of each upgrade.

5 results - 5 files

src/core/dao/DAO.sol:
  135      /// @dev The caller must have the `UPGRADE_DAO_PERMISSION_ID` permission.
  136:     function _authorizeUpgrade(address) internal virtual override auth(UPGRADE_DAO_PERMISSION_ID) {}
  137  

src/core/plugin/PluginUUPSUpgradeable.sol:
  60      /// @dev The caller must have the `UPGRADE_PLUGIN_PERMISSION_ID` permission.
  61:     function _authorizeUpgrade(
  62          address

src/framework/plugin/repo/PluginRepo.sol:
  256      /// @dev The caller must have the `UPGRADE_REPO_PERMISSION_ID` permission.
  257:     function _authorizeUpgrade(
  258          address

src/framework/utils/InterfaceBasedRegistry.sol:
  53      /// @dev The caller must have the `UPGRADE_REGISTRY_PERMISSION_ID` permission.
  54:     function _authorizeUpgrade(
  55          address

src/framework/utils/ens/ENSSubdomainRegistrar.sol:
  73      /// @dev The caller must have the `UPGRADE_REGISTRAR_PERMISSION_ID` permission.
  74:     function _authorizeUpgrade(
  75          address

I suggest implementing some kind of version counter that will be incremented automatically when you upgrade the contract.

Recommendation:

Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol:

+	uint256 public authorizeUpgradeCounter;

  189:     function _authorizeUpgrade(address) internal override {
  190:         _atLeastRole(DEFAULT_ADMIN_ROLE);
  191:         require(
  192:             upgradeProposalTime + UPGRADE_TIMELOCK < block.timestamp,
  193:             "Upgrade cooldown not initiated or still ongoing"
  194:         );
+	authorizeUpgradeCounter+=1;
  195:         clearUpgradeCooldown();
  196:     }

[N-05] Tokens accidentally sent to the contract cannot be recovered

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

It can't be recovered if the tokens accidentally arrive at the contract address, which has happened to many popular projects, so I recommend adding a recovery code to your critical contracts.

Recommended Mitigation Steps

Add this code:

 /**
  * @notice Sends ERC20 tokens trapped in contract to external address
  * @dev Onlyowner is allowed to make this function call
  * @param account is the receiving address
  * @param externalToken is the token being sent
  * @param amount is the quantity being sent
  * @return boolean value indicating whether the operation succeeded.
  *
 */
  function rescueERC20(address account, address externalToken, uint256 amount) public onlyOwner returns (bool) {
    IERC20(externalToken).transfer(account, amount);
    return true;
  }
}

[N-06] Repeated validation logic can be converted into a function modifier

If a query or logic is repeated over many lines, using a modifier improves the readability and reusability of the code (!= address(0))

6 results - 5 files

src/framework/utils/TokenFactory.sol:
  85          // deploy token
  86:         if (token != address(0)) {
  87              // Validate if token is ERC20

src/framework/utils/ens/ENSSubdomainRegistrar.sol:
  88  
  89:         if (currentOwner != address(0)) {
  90              revert AlreadyRegistered(subnode, currentOwner);

src/plugins/governance/majority-voting/token/TokenVotingSetup.sol:
   97  
   98:         if (token != address(0)) {
   99              if (!token.isContract()) {

  150              memory permissions = new PermissionLib.MultiTargetPermission[](
  151:                 tokenSettings.addr != address(0) ? 3 : 4
  152              );

src/token/ERC20/governance/GovernanceERC20.sol:
  115          // Automatically turn on delegation on mint/transfer but only for the first time.
  116:         if (to != address(0) && numCheckpoints(to) == 0 && delegates(to) == address(0)) {
  117              _delegate(to, to);

src/token/ERC20/governance/GovernanceWrappedERC20.sol:
  105          // Automatically turn on delegation on mint/transfer but only for the first time.
  106:         if (to != address(0) && numCheckpoints(to) == 0 && delegates(to) == address(0)) {
  107              _delegate(to, to);

[N-07] For functions, follow Solidity standard naming conventions (internal function style rule)

Context:

14 results - 6 files

src/core/dao/DAO.sol:
   61:     uint256 internal constant MAX_ACTIONS = 256;

src/core/permission/PermissionManager.sol:
   18:     address internal constant ANY_ADDR = address(type(uint160).max);
   21:     address internal constant UNSET_FLAG = address(0);
   24:     address internal constant ALLOW_FLAG = address(2);
   27:     mapping(bytes32 => address) internal permissionsHashed;

src/core/utils/CallbackHandler.sol:
  11:     mapping(bytes4 => bytes4) internal callbackMagicNumbers;
  14:     bytes4 internal constant UNREGISTERED_CALLBACK = bytes4(0);

src/framework/plugin/repo/PluginRepo.sol:
   55:     mapping(uint8 => uint16) internal buildsPerRelease;
   58:     mapping(bytes32 => Version) internal versions;
   61:     mapping(address => bytes32) internal latestTagHashForPluginSetup;
  251:     function tagHash(Tag memory _tag) internal pure returns (bytes32) {


src/plugins/governance/admin/Admin.sol:
  19:     bytes4 internal constant ADMIN_INTERFACE_ID =

src/plugins/governance/majority-voting/MajorityVotingBase.sol:
  173:     bytes4 internal constant MAJORITY_VOTING_BASE_INTERFACE_ID =
  187:     mapping(uint256 => Proposal) internal proposals;

Description: The above codes don't follow Solidity's standard naming convention,

internal and private functions : the mixedCase format starting with an underscore (_mixedCase starting with an underscore)

[N-08] Use descriptive names for Contracts and Libraries

This codebase will be difficult to navigate, as there are no descriptive naming conventions that specify which files should contain meaningful logic.

Prefixes should be added like this by filing:

Interface I_ absctract contracts Abs_ Libraries Lib_

We recommend that you implement this or a similar agreement.

[N-09] Use SMTChecker

The highest tier of smart contract behavior assurance is formal mathematical verification. All assertions that are made are guaranteed to be true across all inputs → The quality of your asserts is the quality of your verification.

https://twitter.com/0xOwenThurm/status/1614359896350425088?t=dbG9gHFigBX85Rv29lOjIQ&s=19

[N-10] Showing the actual values of numbers in NatSpec comments makes checking and reading code easier

2 results - 2 files

src/plugins/governance/majority-voting/MajorityVotingBase.sol:
- 540:         if (_votingSettings.minDuration < 60 minutes) {
+ 540:         if (_votingSettings.minDuration < 60 minutes) { // 40 minutes ( 40 * 60 ) = 2_400
  541:             revert MinDurationOutOfBounds({limit: 60 minutes, actual: _votingSettings.minDuration});
  542:         }
  543: 
- 544:         if (_votingSettings.minDuration > 365 days) {
+ 544:         if (_votingSettings.minDuration > 365 days) { // 365 days ( 365*24*60*60 ) = 31_536_000
  545:             revert MinDurationOutOfBounds({limit: 365 days, actual: _votingSettings.minDuration});
  546:         }

[N-11] Lines are too long

MajorityVotingBase.sol#L27

MajorityVotingBase.sol#L31

MajorityVotingBase.sol#L33-L34

MajorityVotingBase.sol#L38

MajorityVotingBase.sol#L62-L63

MajorityVotingBase.sol#L108

MajorityVotingBase.sol#L136

MajorityVotingBase.sol#L428

MajorityVotingBase.sol#L527

MajorityVotingBase.sol#L593

Multisig.sol#L33

Multisig.sol#L201

Multisig.sol#L304

PluginRepo.sol#L255

PermissionManager.sol#L26

Description: It is generally recommended that lines in the source code should not exceed 80-120 characters. Today's screens are much larger, so in some cases it makes sense to expand that.The lines above should be split when they reach that length, as the files will most likely be on GitHub and GitHub always uses a scrollbar when the length is more than 164 characters.

(why-is-80-characters-the-standard-limit-for-code-width)[https://softwareengineering.stackexchange.com/questions/148677/why-is-80-characters-the-standard-limit-for-code-width]

Recommendation: Multiline output parameters and return statements should follow the same style recommended for wrapping long lines found in the Maximum Line Length section.

https://docs.soliditylang.org/en/v0.8.17/style-guide.html#introduction

[N-12] Constants on the left are better

If you use the constant first you support structures that veil programming errors. And one should only produce code either to add functionality, fix an programming error or trying to support structures to avoid programming errors (like design patterns).

https://www.moserware.com/2008/01/constants-on-left-are-better-but-this.html

2 results - 2 files

src/token/ERC20/governance/GovernanceERC20.sol:
  116:         if (to != address(0) && numCheckpoints(to) == 0 && delegates(to) == address(0)) {

src/token/ERC20/governance/GovernanceWrappedERC20.sol:
  106:         if (to != address(0) && numCheckpoints(to) == 0 && delegates(to) == address(0)) {

[N-13] constants should be defined rather than using magic numbers

A magic number is a numeric literal that is used in the code without any explanation of its meaning. The use of magic numbers makes programs less readable and hence more difficult to maintain and update. Even assembly can benefit from using readable constants instead of hex/numeric literals

1 result - 1 file

src/plugins/utils/Ratio.sol:
  24:     result = _value / RATIO_BASE;

[N-14] Function Calls in Loop Could Lead to Denial of Service due to Array length not being checked

src/framework/dao/DAOFactory.sol#63:

 function createDao(
        DAOSettings calldata _daoSettings,
        PluginSettings[] calldata _pluginSettings
    ) external returns (DAO createdDao) {
        // Check if no plugin is provided.
        if (_pluginSettings.length == 0) {
            revert NoPluginProvided();
        }
+      if(_pluginSettings.length > maxArrayLength) {
+          revert maxArrayLengt();
+       }

        // Create DAO.
        createdDao = _createDAO(_daoSettings);

        // Register DAO.
        daoRegistry.register(createdDao, msg.sender, _daoSettings.subdomain);

        // Get Permission IDs
        bytes32 rootPermissionID = createdDao.ROOT_PERMISSION_ID();
        bytes32 applyInstallationPermissionID = pluginSetupProcessor
            .APPLY_INSTALLATION_PERMISSION_ID();

        // Grant the temporary permissions.
        // Grant Temporarly `ROOT_PERMISSION` to `pluginSetupProcessor`.
        createdDao.grant(address(createdDao), address(pluginSetupProcessor), rootPermissionID);

        // Grant Temporarly `APPLY_INSTALLATION_PERMISSION` on `pluginSetupProcessor` to this `DAOFactory`.
        createdDao.grant(
            address(pluginSetupProcessor),
            address(this),
            applyInstallationPermissionID
        );

        // Install plugins on the newly created DAO.
        for (uint256 i; i < _pluginSettings.length; ++i) {
            // Prepare plugin.
            (
                address plugin,
                IPluginSetup.PreparedSetupData memory preparedSetupData
            ) = pluginSetupProcessor.prepareInstallation(
                    address(createdDao),
                    PluginSetupProcessor.PrepareInstallationParams(
                        _pluginSettings[i].pluginSetupRef,
                        _pluginSettings[i].data
                    )
                );

            // Apply plugin.
            pluginSetupProcessor.applyInstallation(
                address(createdDao),
                PluginSetupProcessor.ApplyInstallationParams(
                    _pluginSettings[i].pluginSetupRef,
                    plugin,
                    preparedSetupData.permissions,
                    hashHelpers(preparedSetupData.helpers)
                )
            );
        }

        // Set the rest of DAO's permissions.
        _setDAOPermissions(createdDao);

        // Revoke the temporarly granted permissions.
        // Revoke Temporarly `ROOT_PERMISSION` from `pluginSetupProcessor`.
        createdDao.revoke(address(createdDao), address(pluginSetupProcessor), rootPermissionID);

        // Revoke `APPLY_INSTALLATION_PERMISSION` on `pluginSetupProcessor` from this `DAOFactory` .
        createdDao.revoke(
            address(pluginSetupProcessor),
            address(this),
            applyInstallationPermissionID
        );

        // Revoke Temporarly `ROOT_PERMISSION_ID` from `pluginSetupProcessor` that implecitly granted to this `DaoFactory`
        // at the create dao step `address(this)` being the initial owner of the new created DAO.
        createdDao.revoke(address(createdDao), address(this), rootPermissionID);
    }

Recommendation: Function calls made in unbounded loop are error-prone with potential resource exhaustion as it can trap the contract due to gas limitations or failed transactions. Consider bounding the loop length if the array is expected to be growing and/or handling a huge list of elements to avoid unnecessary gas wastage and denial of service.

[N-15] Use a single file for all system-wide constants

There are many addresses and constants used in the system. It is recommended to put the most used ones in one file (for example constants.sol, use inheritance to access these values)

This will help with readability and easier maintenance for future changes. This also helps with any issues, as some of these hard-coded values are admin addresses.

constants.sol Use and import this file in contracts that require access to these values. This is just a suggestion, in some use cases this may result in higher gas usage in the distribution.

29 results - 19 files

src/core/dao/DAO.sol:
  40:     bytes32 public constant EXECUTE_PERMISSION_ID = keccak256("EXECUTE_PERMISSION");
  43:     bytes32 public constant UPGRADE_DAO_PERMISSION_ID = keccak256("UPGRADE_DAO_PERMISSION");
  46:     bytes32 public constant SET_METADATA_PERMISSION_ID = keccak256("SET_METADATA_PERMISSION");
  49:     bytes32 public constant SET_TRUSTED_FORWARDER_PERMISSION_ID =
  53:     bytes32 public constant SET_SIGNATURE_VALIDATOR_PERMISSION_ID =
  57:     bytes32 public constant REGISTER_STANDARD_CALLBACK_PERMISSION_ID =

src/core/permission/PermissionLib.sol:
  10:     address public constant NO_CONDITION = address(0);

src/core/permission/PermissionManager.sol:
  15:     bytes32 public constant ROOT_PERMISSION_ID = keccak256("ROOT_PERMISSION");

src/core/plugin/PluginUUPSUpgradeable.sol:
  35:     bytes32 public constant UPGRADE_PLUGIN_PERMISSION_ID = keccak256("UPGRADE_PLUGIN_PERMISSION");

src/framework/dao/DAORegistry.sol:
  15:     bytes32 public constant REGISTER_DAO_PERMISSION_ID = keccak256("REGISTER_DAO_PERMISSION");

src/framework/plugin/repo/PluginRepo.sol:
  49:     bytes32 public constant MAINTAINER_PERMISSION_ID = keccak256("MAINTAINER_PERMISSION");
  52:     bytes32 public constant UPGRADE_REPO_PERMISSION_ID = keccak256("UPGRADE_REPO_PERMISSION");

src/framework/plugin/repo/PluginRepoRegistry.sol:
  16:     bytes32 public constant REGISTER_PLUGIN_REPO_PERMISSION_ID =

src/framework/plugin/setup/PluginSetupProcessor.sol:
  27:     bytes32 public constant APPLY_INSTALLATION_PERMISSION_ID =
  31:     bytes32 public constant APPLY_UPDATE_PERMISSION_ID = keccak256("APPLY_UPDATE_PERMISSION");
  34:     bytes32 public constant APPLY_UNINSTALLATION_PERMISSION_ID =

src/framework/utils/InterfaceBasedRegistry.sol:
  18:     bytes32 public constant UPGRADE_REGISTRY_PERMISSION_ID =

src/framework/utils/ens/ENSSubdomainRegistrar.sol:
  18:     bytes32 public constant UPGRADE_REGISTRAR_PERMISSION_ID =
  22:     bytes32 public constant REGISTER_ENS_SUBDOMAIN_PERMISSION_ID =

src/plugins/counter-example/MultiplyHelper.sol:
  12:     bytes32 public constant MULTIPLY_PERMISSION_ID = keccak256("MULTIPLY_PERMISSION");

src/plugins/counter-example/v1/CounterV1.sol:
  14:     bytes32 public constant MULTIPLY_PERMISSION_ID = keccak256("MULTIPLY_PERMISSION");

src/plugins/counter-example/v2/CounterV2.sol:
  14:     bytes32 public constant MULTIPLY_PERMISSION_ID = keccak256("MULTIPLY_PERMISSION");

src/plugins/governance/admin/Admin.sol:
  23:     bytes32 public constant EXECUTE_PROPOSAL_PERMISSION_ID =

src/plugins/governance/majority-voting/MajorityVotingBase.sol:
  183:     bytes32 public constant UPDATE_VOTING_SETTINGS_PERMISSION_ID =

src/plugins/governance/majority-voting/addresslist/AddresslistVoting.sol:
  27:     bytes32 public constant UPDATE_ADDRESSES_PERMISSION_ID =

src/plugins/governance/multisig/Multisig.sol:
  71:     bytes32 public constant UPDATE_MULTISIG_SETTINGS_PERMISSION_ID =

src/plugins/token/MerkleMinter.sol:
  24:     bytes32 public constant MERKLE_MINT_PERMISSION_ID = keccak256("MERKLE_MINT_PERMISSION");
  27:     bytes32 public constant CHANGE_DISTRIBUTOR_PERMISSION_ID =

src/token/ERC20/governance/GovernanceERC20.sol:
  28:     bytes32 public constant MINT_PERMISSION_ID = keccak256("MINT_PERMISSION");

[N-16] Highest risk must be specified in NatSpec comments and documentation

Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol:
  189:     function _authorizeUpgrade(address) internal override {
  190:         _atLeastRole(DEFAULT_ADMIN_ROLE);
  191:         require(
  192:             upgradeProposalTime + UPGRADE_TIMELOCK < block.timestamp,
  193:             "Upgrade cooldown not initiated or still ongoing"
  194:         );
  195:         clearUpgradeCooldown();
  196:     }

Description: Although the UUPS model has many advantages and the proposed proxy model is currently the UUPS model, there are some caveats we should be aware of before applying it to a real-world project.

One of the main attention: Since upgrades are done through the implementation contract with the help of the _authorizeUpgrade method, there is a high risk of new implementations such as excluding the _authorizeUpgrade method, which can permanently kill the smart contract's ability to upgrade. Also, this pattern is somewhat complicated to implement compared to other proxy patterns.

Recommendation: Therefore, this highest risk must be found in NatSpec comments and documents.

[N-17] Include proxy contracts to Audit

The Proxy contract could not be seen in the checklist because it is an upgradable contract, only the implementation contracts are visible, I recommend including the Proxy contract in the audit for integrity in the audit.

[N-18] Use of bytes.concat() instead of abi.encodePacked()

src/framework/utils/ens/ENSSubdomainRegistrar.sol:
  82:     function registerSubnode(
  83:         bytes32 _label,
  84:         address _targetAddress
  85:     ) external auth(REGISTER_ENS_SUBDOMAIN_PERMISSION_ID) {
  86:         bytes32 subnode = keccak256(abi.encodePacked(node, _label));

Rather than using abi.encodePacked for appending bytes, since version 0.8.4, bytes.concat() is enabled

Since version 0.8.4 for appending bytes, bytes.concat() can be used instead of abi.encodePacked(,)

[N-19] Don't use _msgSender() if not supporting EIP-2771

Use msg.sender if the code does not implement EIP-2771 trusted forwarder support.

Reference: https://eips.ethereum.org/EIPS/eip-2771

14 results - 7 files

src/core/plugin/dao-authorizable/DaoAuthorizable.sol:
  32:         _auth(dao_, address(this), _msgSender(), _permissionId, _msgData());

src/core/plugin/dao-authorizable/DaoAuthorizableUpgradeable.sol:
  33:         _auth(dao_, address(this), _msgSender(), _permissionId, _msgData());

src/plugins/governance/admin/Admin.sol:
  70:             _creator: _msgSender(),

src/plugins/governance/majority-voting/MajorityVotingBase.sol:
  270:         address account = _msgSender();

src/plugins/governance/majority-voting/addresslist/AddresslistVoting.sol:
   97:         if (minProposerVotingPower() != 0 && !isListedAtBlock(_msgSender(), snapshotBlock)) {
   98:             revert ProposalCreationForbidden(_msgSender());
  102:             _creator: _msgSender(),

src/plugins/governance/majority-voting/token/TokenVoting.sol:
  91:         if (votingToken.getPastVotes(_msgSender(), snapshotBlock) < minProposerVotingPower()) {
  92:             revert ProposalCreationForbidden(_msgSender());
  96:             _creator: _msgSender(),

src/plugins/governance/multisig/Multisig.sol:
  216:         if (multisigSettings.onlyListed && !isListedAtBlock(_msgSender(), snapshotBlock)) {
  217:             revert ProposalCreationForbidden(_msgSender());
  231:             _creator: _msgSender(),
  266:         address approver = _msgSender();

[N-20] Use parameters of custom Errors

Custom errors can be parameterized to better reflect their respective error messages if need be. For instance, the custom error instance below may be refactored as follows:

14 results - 7 files

src/core/dao/DAO.sol:
  73:     error TooManyActions();
  80:     error ZeroAmount();

src/core/permission/PermissionManager.sol:
  51:     error ConditionNotPresentForAnyAddress();
  54:     error PermissionsForAnyAddressDisallowed();
  57:     error AnyAddressDisallowedForWhoAndWhere();

src/framework/dao/DAOFactory.sol:
  48:     error NoPluginProvided();

src/framework/plugin/repo/PluginRepo.sol:
  72:     error InvalidPluginSetupInterface();
  75:     error ReleaseZeroNotAllowed();
  89:     error EmptyReleaseMetadata();
  92:     error ReleaseDoesNotExist();

src/framework/plugin/repo/PluginRepoRegistry.sol:
  31:     error EmptyPluginRepoSubdomain();

src/framework/plugin/setup/PluginSetupProcessor.sol:
  139:     error PluginNonupgradeable(address plugin);
  168:     error PluginAlreadyInstalled();

src/plugins/governance/majority-voting/token/TokenVoting.sol:
  29:     error NoVotingPower();

[S-01] Generate perfect code headers every time

Description: I recommend using header for Solidity code layout and readability

https://github.com/transmissions11/headers

/*//////////////////////////////////////////////////////////////
                           TESTING 123
//////////////////////////////////////////////////////////////*/

Gas Optimizations List

Total 22 issues

[G-01] Remove the initializer modifier

if we can just ensure that the initialize() function could only be called from within the constructor, we shouldn't need to worry about it getting called again.

14 results - 14 files:

src\core\dao\DAO.sol:

  109:     ) external initializer {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L109

src\framework\dao\DAORegistry.sol:

  40:     ) external initializer {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/dao/DAORegistry.sol#L40

src\framework\plugin\repo\PluginRepo.sol:

  120:     function initialize(address initialOwner) external initializer {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepo.sol#L120

src\framework\plugin\repo\PluginRepoRegistry.sol:

41:     function initialize(IDAO _dao, ENSSubdomainRegistrar _subdomainRegistrar) external initializer {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepoRegistry.sol#L41

src\framework\utils\ens\ENSSubdomainRegistrar.sol:

57:     function initialize(IDAO _managingDao, ENS _ens, bytes32 _node) external initializer {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/ens/ENSSubdomainRegistrar.sol#L57

src\plugins\counter-example\v1\CounterV1.sol:

  30:     ) external initializer {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/counter-example/v1/CounterV1.sol#L30

src\plugins\governance\admin\Admin.sol:

  29:     function initialize(IDAO _dao) external initializer {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/admin/Admin.sol#L29

src\plugins\governance\majority-voting\addresslist\AddresslistVoting.sol:

  38:     ) external initializer {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/addresslist/AddresslistVoting.sol#L38

src\plugins\governance\majority-voting\token\TokenVoting.sol:

  40:     ) external initializer {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/token/TokenVoting.sol#L40

src\plugins\governance\multisig\Multisig.sol:

  129:     ) external initializer {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/multisig/Multisig.sol#L129

src\plugins\token\MerkleDistributor.sol:

  50:     ) external initializer {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/token/MerkleDistributor.sol#L50

src\plugins\token\MerkleMinter.sol:

  45:     ) external initializer {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/token/MerkleMinter.sol#L45

src\token\ERC20\governance\GovernanceERC20.sol:

  68:     ) public initializer {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/token/ERC20/governance/GovernanceERC20.sol#L68

src\token\ERC20\governance\GovernanceWrappedERC20.sol:

  50:     ) public initializer {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/token/ERC20/governance/GovernanceWrappedERC20.sol#L50

In the EVM, the constructor's job is actually to return the bytecode that will live at the contract's address. So, while inside a constructor, your address (address(this)) will be the deployment address, but there will be no bytecode at that address! So if we check address(this).code.length before the constructor has finished, even from within a delegatecall, we will get 0. So now let's update our initialize() function to only run if we are inside a constructor:

src\token\ERC20\governance\GovernanceWrappedERC20.sol:

- 50:     ) public initializer {
+        require(address(this).code.length == 0, 'not in constructor’);

Now the Proxy contract's constructor can still delegatecall initialize(), but if anyone attempts to call it again (after deployment) through the Proxy instance, or tries to call it directly on the above instance, it will revert because address(this).code.length will be nonzero.

Also, because we no longer need to write to any state to track whether initialize() has been called, we can avoid the 20k storage gas cost. In fact, the cost for checking our own code size is only 2 gas, which means we have a 10,000x gas savings over the standard version. Pretty neat!

[G-02] Use hardcode address instead address(this)

Instead of address(this), it is more gas-efficient to pre-calculate and use the address with a hardcode. Foundry's script.sol and solmateLibRlp.sol contracts can do this.

Reference: https://book.getfoundry.sh/reference/forge-std/compute-create-address https://twitter.com/transmissions11/status/1518507047943245824

19 results - 12 files:

src\core\dao\DAO.sol:

  232:             IERC20Upgradeable(_token).safeTransferFrom(msg.sender, address(this), _amount);

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L232

src\core\permission\PermissionManager.sol:

  213:         _grant(address(this), _initialOwner, ROOT_PERMISSION_ID);

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/permission/PermissionManager.sol#L213

src\core\plugin\dao-authorizable\DaoAuthorizable.sol:

  32:         _auth(dao_, address(this), _msgSender(), _permissionId, _msgData());

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/plugin/dao-authorizable/DaoAuthorizable.sol#L32

src\core\plugin\dao-authorizable\DaoAuthorizableUpgradeable.sol:

  33:         _auth(dao_, address(this), _msgSender(), _permissionId, _msgData());

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/plugin/dao-authorizable/DaoAuthorizableUpgradeable.sol#L33

src\framework\dao\DAOFactory.sol:

   90:             address(this),

  130:             address(this),

  136:         createdDao.revoke(address(createdDao), address(this), rootPermissionID);

  148:             address(this),

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/dao/DAOFactory.sol#L90

src\framework\plugin\repo\PluginRepo.sol:

  123:         _grant(address(this), initialOwner, MAINTAINER_PERMISSION_ID);

  124:         _grant(address(this), initialOwner, UPGRADE_REPO_PERMISSION_ID);

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepo.sol#L123-L124

src\framework\plugin\repo\PluginRepoFactory.sol:

   53:         pluginRepo = _createPluginRepo(_subdomain, address(this));

   94:             address(this),

   99:             address(this),

  104:             address(this),

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepoFactory.sol#L53

src\framework\plugin\setup\PluginSetupProcessor.sol:

  702:             !DAO(payable(_dao)).hasPermission(address(this), msg.sender, _permissionId, bytes(""))

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/setup/PluginSetupProcessor.sol#L702

src\framework\utils\TokenFactory.sol:

  89:                 abi.encodeWithSelector(IERC20Upgradeable.balanceOf.selector, address(this))

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/TokenFactory.sol#L89

src\framework\utils\ens\ENSSubdomainRegistrar.sol:

  93:         ens.setSubnodeOwner(node, _label, address(this));

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/ens/ENSSubdomainRegistrar.sol#L93

src\plugins\governance\admin\Admin.sol:

  51:                 _where: address(this),

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/admin/Admin.sol#L51

src\plugins\governance\majority-voting\token\TokenVotingSetup.sol:

  278:             abi.encodeWithSelector(IERC20Upgradeable.balanceOf.selector, address(this))

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/token/TokenVotingSetup.sol#L278

[G-03] State variables only set in the constructor should be declared immutable

Avoids a Gsset (20000 gas) in the constructor, and replaces the first access in each transaction (Gcoldsload - 2100 gas) and each access thereafter (Gwarmacces - 100 gas) with a PUSH32 (3 gas).

2 results - 2 files:

contracts\src\framework\plugin\repo\PluginRepoFactory.sol:

  15:     PluginRepoRegistry public pluginRepoRegistry;

  22     constructor(PluginRepoRegistry _pluginRepoRegistry) {
  23:         pluginRepoRegistry = _pluginRepoRegistry;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepoFactory.sol#L23

contracts\src\framework\plugin\setup\PluginSetupProcessor.sol:
  
  128     PluginRepoRegistry public repoRegistry;

  277     constructor(PluginRepoRegistry _repoRegistry) {
  278:         repoRegistry = _repoRegistry;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/setup/PluginSetupProcessor.sol#L278

[G-04] Remove unused struct

In the TokenFactory contract, MintConfig struct is not used in any contract and outside the scope.

1 result - 1 file:

src\framework\utils\TokenFactory.sol:

  62:     struct MintConfig {
  63:         address[] receivers;
  64:         uint256[] amounts;
  65:     }

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/TokenFactory.sol#L62-L65

[G-05] Function Calls in a Loop Can Cause Denial of Service and out of gas due to not checking the Array length

Function calls made in an infinite loop are prone to gassing with potential resource exhaustion, as they can trap the contract due to gas limitations or failed transactions. Consider limiting the loop length if the array is expected to grow and/or handle a very large list of items to avoid unnecessary waste of gas and denial of service.

src/framework/dao/DAOFactory.sol#63:

 function createDao(
        DAOSettings calldata _daoSettings,
        PluginSettings[] calldata _pluginSettings
    ) external returns (DAO createdDao) {
        // Check if no plugin is provided.
-       if (_pluginSettings.length == 0) {
-           revert NoPluginProvided();
-       }
+      if(_pluginSettings.length > maxArrayLength) {
+          revert maxArrayLengt();
+       }

        // Create DAO.
        createdDao = _createDAO(_daoSettings);

        // Register DAO.
        daoRegistry.register(createdDao, msg.sender, _daoSettings.subdomain);

        // Get Permission IDs
        bytes32 rootPermissionID = createdDao.ROOT_PERMISSION_ID();
        bytes32 applyInstallationPermissionID = pluginSetupProcessor
            .APPLY_INSTALLATION_PERMISSION_ID();

        // Grant the temporary permissions.
        // Grant Temporarly `ROOT_PERMISSION` to `pluginSetupProcessor`.
        createdDao.grant(address(createdDao), address(pluginSetupProcessor), rootPermissionID);

        // Grant Temporarly `APPLY_INSTALLATION_PERMISSION` on `pluginSetupProcessor` to this `DAOFactory`.
        createdDao.grant(
            address(pluginSetupProcessor),
            address(this),
            applyInstallationPermissionID
        );

        // Install plugins on the newly created DAO.
        for (uint256 i; i < _pluginSettings.length; ++i) {
            // Prepare plugin.
            (
                address plugin,
                IPluginSetup.PreparedSetupData memory preparedSetupData
            ) = pluginSetupProcessor.prepareInstallation(
                    address(createdDao),
                    PluginSetupProcessor.PrepareInstallationParams(
                        _pluginSettings[i].pluginSetupRef,
                        _pluginSettings[i].data
                    )
                );

            // Apply plugin.
            pluginSetupProcessor.applyInstallation(
                address(createdDao),
                PluginSetupProcessor.ApplyInstallationParams(
                    _pluginSettings[i].pluginSetupRef,
                    plugin,
                    preparedSetupData.permissions,
                    hashHelpers(preparedSetupData.helpers)
                )
            );
        }

        // Set the rest of DAO's permissions.
        _setDAOPermissions(createdDao);

        // Revoke the temporarly granted permissions.
        // Revoke Temporarly `ROOT_PERMISSION` from `pluginSetupProcessor`.
        createdDao.revoke(address(createdDao), address(pluginSetupProcessor), rootPermissionID);

        // Revoke `APPLY_INSTALLATION_PERMISSION` on `pluginSetupProcessor` from this `DAOFactory` .
        createdDao.revoke(
            address(pluginSetupProcessor),
            address(this),
            applyInstallationPermissionID
        );

        // Revoke Temporarly `ROOT_PERMISSION_ID` from `pluginSetupProcessor` that implecitly granted to this `DaoFactory`
        // at the create dao step `address(this)` being the initial owner of the new created DAO.
        createdDao.revoke(address(createdDao), address(this), rootPermissionID);
    }

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/dao/DAOFactory.sol#L63-L137

[G-06] Using storage instead of memory for structs/arrays saves gas

When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct

8 results - 6 files:

src\core\permission\PermissionManager.sol:

  151:    PermissionLib.SingleTargetPermission memory item = items[i];

  171:    PermissionLib.MultiTargetPermission memory item = _items[i];

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/permission/PermissionManager.sol#L151

src\framework\dao\DAOFactory.sol:

  99:     IPluginSetup.PreparedSetupData memory preparedSetupData

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/dao/DAOFactory.sol#L99

src\framework\plugin\repo\PluginRepo.sol:

  167:    Tag memory tag = Tag(_release, build);

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepo.sol#L167

src\framework\plugin\setup\PluginSetupProcessor.sol:

  433:    PluginRepo.Version memory currentVersion = _params.pluginSetupRepo.getVersion(

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/setup/PluginSetupProcessor.sol#L433

src\framework\utils\TokenFactory.sol:

  88:     bytes memory data = token.functionStaticCall(

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/TokenFactory.sol#L88

src\plugins\governance\majority-voting\token\TokenVotingSetup.sol:

  108:    bool[] memory supportedIds = _getTokenInterfaceIds(token);

  212:    bool[] memory supportedIds = _getTokenInterfaceIds(token);

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/token/TokenVotingSetup.sol#L108

[G-07] Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

3 results - 1 file:

src\framework\plugin\repo\PluginRepo.sol:

  55:     mapping(uint8 => uint16) internal buildsPerRelease;
  58:     mapping(bytes32 => Version) internal versions;
  61:     mapping(address => bytes32) internal latestTagHashForPluginSetup;

[G-08] Multiple accesses of a mapping/array should use a local variable cache

The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations. Caching an array's struct avoids recalculating the array offsets into memory/calldata.

6 results - 2 files:

src\framework\plugin\setup\PluginSetupProcessor.sol:

  //@audit preparedSetupIdToBlockNumber[preparedSetupId]

  326:         if (pluginState.blockNumber < pluginState.preparedSetupIdToBlockNumber[preparedSetupId]) {

  330:         pluginState.preparedSetupIdToBlockNumber[preparedSetupId] = block.number;

  590:         if (pluginState.blockNumber < pluginState.preparedSetupIdToBlockNumber[preparedSetupId]) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/setup/PluginSetupProcessor.sol#L326

src\plugins\governance\majority-voting\MajorityVotingBase.sol:

  // @audit proposals[_proposalId]

  458:     proposals[_proposalId].executed = true;

  463:     proposals[_proposalId].actions,

  464:     proposals[_proposalId].allowFailureMap

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/MajorityVotingBase.sol#L458

[G-09] Empty blocks should be removed or emit something

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}}). Empty receive()/fallback() payable functions that are not used, can be removed to save deployment gas.

9 results - 9 files:

src\core\dao\DAO.sol:

  136:     function _authorizeUpgrade(address) internal virtual override auth(UPGRADE_DAO_PERMISSION_ID) {}

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L136

src\core\plugin\PluginUUPSUpgradeable.sol:

  63:     ) internal virtual override auth(UPGRADE_PLUGIN_PERMISSION_ID) {}

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/plugin/PluginUUPSUpgradeable.sol#L63

src\framework\plugin\repo\PluginRepo.sol:

  259:     ) internal virtual override auth(UPGRADE_REPO_PERMISSION_ID) {}

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepo.sol#L259

src\framework\plugin\setup\PluginSetup.sol:
  18:     function prepareUpdate(
  19:         address _dao,
  20:         uint16 _currentBuild,
  21:         SetupPayload calldata _payload
  22:     )
  23:         external
  24:         virtual
  25:         override
  26:         returns (bytes memory initData, PreparedSetupData memory preparedSetupData)
  27:     {}

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/setup/PluginSetup.sol#L18-L27

src\framework\utils\InterfaceBasedRegistry.sol:

  56:     ) internal virtual override auth(UPGRADE_REGISTRY_PERMISSION_ID) {}

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/InterfaceBasedRegistry.sol#L56

src\framework\utils\ens\ENSSubdomainRegistrar.sol:

  76:     ) internal virtual override auth(UPGRADE_REGISTRAR_PERMISSION_ID) {}

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/ens/ENSSubdomainRegistrar.sol#L76

src\plugins\counter-example\v1\CounterV1.sol:
  43      /// @notice Executes something on the DAO.
  44:     function execute() public {
  45:         // In order to do this, Count needs permission on the dao (EXEC_ROLE)
  46:         //dao.execute(...)
  47:     }

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/counter-example/v1/CounterV1.sol#L43-L47

src\plugins\counter-example\v2\CounterV2.sol:
  62      /// @notice Executes something on the DAO.
  63:     function execute() public {
  64:         // In order to do this, Count needs permission on the dao (EXEC_ROLE)
  65:         //dao.execute(...)
  66:     }

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/counter-example/v2/CounterV2.sol#L63-L66

[G-10] Don't use _msgSender() if not supporting EIP-2771

Use msg.sender if the code does not implement EIP-2771 trusted forwarder support.

Reference: https://eips.ethereum.org/EIPS/eip-2771

14 results - 7 files:

src\core\plugin\dao-authorizable\DaoAuthorizable.sol:

  32:    _auth(dao_, address(this), _msgSender(), _permissionId, _msgData());

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/plugin/dao-authorizable/DaoAuthorizable.sol#L32

src\core\plugin\dao-authorizable\DaoAuthorizableUpgradeable.sol:

  33:    _auth(dao_, address(this), _msgSender(), _permissionId, _msgData());

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/plugin/dao-authorizable/DaoAuthorizableUpgradeable.sol#L33

src\plugins\governance\admin\Admin.sol:

  70:    _creator: _msgSender(),

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/admin/Admin.sol#L70

src\plugins\governance\majority-voting\MajorityVotingBase.sol:

  270:   address account = _msgSender();

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/MajorityVotingBase.sol#L270

src\plugins\governance\majority-voting\addresslist\AddresslistVoting.sol:
 
   97:   if (minProposerVotingPower() != 0 && !isListedAtBlock(_msgSender(), snapshotBlock)) {
   98:      revert ProposalCreationForbidden(_msgSender());

  102:   _creator: _msgSender(),

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/addresslist/AddresslistVoting.sol#L97-L98

src\plugins\governance\majority-voting\token\TokenVoting.sol:
  
  91:    if (votingToken.getPastVotes(_msgSender(), snapshotBlock) < minProposerVotingPower()) {
  92:       revert ProposalCreationForbidden(_msgSender());

  96:    _creator: _msgSender(),

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/token/TokenVoting.sol#L91-L92

src\plugins\governance\multisig\Multisig.sol:
 
  216:   if (multisigSettings.onlyListed && !isListedAtBlock(_msgSender(), snapshotBlock)) {

  217:       revert ProposalCreationForbidden(_msgSender());

  231:   _creator: _msgSender(),

  266:   address approver = _msgSender();

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/multisig/Multisig.sol#L216-L217

[G-11] bytes constants are more eficient than string constans

If the data can fit in 32 bytes, the bytes32 data type can be used instead of bytes or strings, as it is less robust in terms of robustness.

10 results - 5 files:

src\core\dao\DAO.sol:

   70:     string private _daoURI;

   89:     event NewURI(string daoURI);

  320:     function daoURI() external view returns (string memory) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L70

src\core\dao\IEIP4824.sol:

  10:     function daoURI() external view returns (string memory _daoURI);

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/IEIP4824.sol#L10

src\framework\dao\DAOFactory.sol:

  34:         string daoURI;
  35:         string subdomain;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/dao/DAOFactory.sol#L34-L35

src\framework\utils\TokenFactory.sol:

  58:         string name;
  59:         string symbol;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/TokenFactory.sol#L58-L59

src\plugins\governance\majority-voting\token\TokenVotingSetup.sol:

  44:         string name;
  45:         string symbol;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/token/TokenVotingSetup.sol#L44-L45

[G-12] Change public function visibility to external

Since the following public functions are not called from within the contract, their visibility can be made external for gas optimization.

8 results - 7 files:

src\framework\plugin\repo\PluginRepo.sol:

  244:     function buildCount(uint8 _release) public view returns (uint256) {
  245:         return buildsPerRelease[_release];
  246:     }

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepo.sol#L244-L246

src\plugins\counter-example\v1\CounterV1.sol:

  44:     function execute() public {
  45:         // In order to do this, Count needs permission on the dao (EXEC_ROLE)
  46:         //dao.execute(...)
  47:     }

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/counter-example/v1/CounterV1.sol#L44-L47

src\plugins\counter-example\v2\CounterV2.sol:

  63:     function execute() public {
  64:         // In order to do this, Count needs permission on the dao (EXEC_ROLE)
  65:         //dao.execute(...)
  66:     }

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/counter-example/v2/CounterV2.sol#L63-L66

src\plugins\governance\majority-voting\MajorityVotingBase.sol:

  291:     function getVoteOption(
  292:         uint256 _proposalId,
  293:         address _voter
  294:     ) public view virtual returns (VoteOption) {
  295:         return proposals[_proposalId].voters[_voter];
  296:     }

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/MajorityVotingBase.sol#L291-L296

src\plugins\governance\multisig\Multisig.sol:

  328:     function hasApproved(uint256 _proposalId, address _account) public view returns (bool) {
  329:         return proposals[_proposalId].approvers[_account];
  330:     }

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/multisig/Multisig.sol#L328-L330

src\plugins\token\MerkleDistributor.sol:

  83:     function unclaimedBalance(
  84:         uint256 _index,
  85:         address _to,
  86:         uint256 _amount,
  87:         bytes32[] memory _proof
  88:     ) public view override returns (uint256) {
  89:         if (isClaimed(_index)) return 0;
  90:         return _verifyBalanceOnTree(_index, _to, _amount, _proof) ? _amount : 0;
  91:     }

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/token/MerkleDistributor.sol#L83-L91

src\token\ERC20\governance\GovernanceWrappedERC20.sol:

  81:     function depositFor(
  82:         address account,
  83:         uint256 amount
  84:     ) public override(IGovernanceWrappedERC20, ERC20WrapperUpgradeable) returns (bool) {
  85:         return ERC20WrapperUpgradeable.depositFor(account, amount);
  86:     }


  89:     function withdrawTo(
  90:         address account,
  91:         uint256 amount
  92:     ) public override(IGovernanceWrappedERC20, ERC20WrapperUpgradeable) returns (bool) {
  93:         return ERC20WrapperUpgradeable.withdrawTo(account, amount);
  94:     }

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/token/ERC20/governance/GovernanceWrappedERC20.sol#L81-L86

[G-13] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contracts 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

Use a larger size then downcast where needed.

3 results 3 files:

contracts\src\framework\plugin\repo\PluginRepo.sol:

  // @audit uint8 _release
  143:         if (_release - latestRelease > 1) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepo.sol#L143

contracts\src\plugins\governance\majority-voting\addresslist\AddresslistVoting.sol:

  // @audit uint64 snapshotBlock
  94:             snapshotBlock = block.number.toUint64() - 1;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/addresslist/AddresslistVoting.sol#L94

contracts\src\plugins\governance\multisig\Multisig.sol:

  // @audit uint64 _startDate
  221:             _startDate = block.timestamp.toUint64();

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/multisig/Multisig.sol#L221

[G-14] 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.

27 results - 16 files:

contracts\src\core\dao\DAO.sol:

  122     function isPermissionRestrictedForAnyAddr(
  123         bytes32 _permissionId
  124:     ) internal pure override returns (bool) {

  136:     function _authorizeUpgrade(address) internal virtual override auth(UPGRADE_DAO_PERMISSION_ID) {}

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L122-L132

contracts\src\core\permission\PermissionManager.sol:

  95:     function __PermissionManager_init(address _initialOwner) internal onlyInitializing {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/permission/PermissionManager.sol#L95

contracts\src\core\plugin\PluginCloneable.sol:

  22:     function __PluginCloneable_init(IDAO _dao) internal virtual onlyInitializing {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/plugin/PluginCloneable.sol#L22

contracts\src\core\plugin\PluginUUPSUpgradeable.sol:

  39:     function __PluginUUPSUpgradeable_init(IDAO _dao) internal virtual onlyInitializing {

  61     function _authorizeUpgrade(
  62         address
  63:     ) internal virtual override auth(UPGRADE_PLUGIN_PERMISSION_ID) {}

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/plugin/PluginUUPSUpgradeable.sol#L39

contracts\src\core\plugin\dao-authorizable\DaoAuthorizableUpgradeable.sol:

  20:     function __DaoAuthorizableUpgradeable_init(IDAO _dao) internal onlyInitializing {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/plugin/dao-authorizable/DaoAuthorizableUpgradeable.sol#L20

contracts\src\core\plugin\proposal\Proposal.sol:

  45     function _createProposal(
  46         address _creator,
  47         bytes calldata _metadata,
  48         uint64 _startDate,
  49         uint64 _endDate,
  50         IDAO.Action[] calldata _actions,
  51         uint256 _allowFailureMap
  52:     ) internal virtual returns (uint256 proposalId) {

  72     function _executeProposal(
  73         IDAO _dao,
  74         uint256 _proposalId,
  75         IDAO.Action[] memory _actions,
  76         uint256 _allowFailureMap
  77:     ) internal virtual returns (bytes[] memory execResults, uint256 failureMap) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/plugin/proposal/Proposal.sol#L45-L52

contracts\src\core\plugin\proposal\ProposalUpgradeable.sol:

  45     function _createProposal(
  46         address _creator,
  47         bytes calldata _metadata,
  48         uint64 _startDate,
  49         uint64 _endDate,
  50         IDAO.Action[] calldata _actions,
  51         uint256 _allowFailureMap
  52:     ) internal virtual returns (uint256 proposalId) {

  72     function _executeProposal(
  73         IDAO _dao,
  74         uint256 _proposalId,
  75         IDAO.Action[] memory _actions,
  76         uint256 _allowFailureMap
  77:     ) internal virtual returns (bytes[] memory execResults, uint256 failureMap) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/plugin/proposal/ProposalUpgradeable.sol#L45-L52

contracts\src\core\utils\CallbackHandler.sol:

  31     function _handleCallback(
  32         bytes4 _callbackSelector,
  33         bytes memory _data
  34:     ) internal virtual returns (bytes4) {

  48:     function _registerCallback(bytes4 _callbackSelector, bytes4 _magicNumber) internal virtual {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/utils/CallbackHandler.sol#L31-L34

contracts\src\framework\plugin\repo\PluginRepo.sol:

  257     function _authorizeUpgrade(
  258         address
  259:     ) internal virtual override auth(UPGRADE_REPO_PERMISSION_ID) {}

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepo.sol#L257-L259

contracts\src\framework\plugin\setup\PluginSetup.sol:

  33     function createERC1967Proxy(
  34         address _implementation,
  35         bytes memory _data
  36:     ) internal returns (address) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/setup/PluginSetup.sol#L33-L36

contracts\src\framework\utils\InterfaceBasedRegistry.sol:

  43     function __InterfaceBasedRegistry_init(
  44         IDAO _managingDao,
  45         bytes4 _targetInterfaceId
  46:     ) internal virtual onlyInitializing {

  54     function _authorizeUpgrade(
  55         address
  56:     ) internal virtual override auth(UPGRADE_REGISTRY_PERMISSION_ID) {}

  61:     function _register(address _registrant) internal {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/InterfaceBasedRegistry.sol#L43-L46

contracts\src\framework\utils\ens\ENSSubdomainRegistrar.sol:

  74     function _authorizeUpgrade(
  75         address
  76:     ) internal virtual override auth(UPGRADE_REGISTRAR_PERMISSION_ID) {}

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/ens/ENSSubdomainRegistrar.sol#L74-L76

contracts\src\plugins\governance\majority-voting\MajorityVotingBase.sol:

  238     function __MajorityVotingBase_init(
  239         IDAO _dao,
  240         VotingSettings calldata _votingSettings
  241:     ) internal onlyInitializing {

  564     function _validateProposalDates(
  565         uint64 _start,
  566         uint64 _end
  567:     ) internal view virtual returns (uint64 startDate, uint64 endDate) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/MajorityVotingBase.sol#L238-L241

contracts\src\plugins\governance\majority-voting\addresslist\AddresslistVoting.sol:

  148     function _vote(
  149         uint256 _proposalId,
  150         VoteOption _voteOption,
  151         address _voter,
  152         bool _tryEarlyExecution
  153:     ) internal override {

  191     function _canVote(
  192         uint256 _proposalId,
  193         address _account,
  194         VoteOption _voteOption
  195:     ) internal view override returns (bool) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/addresslist/AddresslistVoting.sol#L148-L153

contracts\src\plugins\governance\majority-voting\token\TokenVoting.sol:

  143     function _vote(
  144         uint256 _proposalId,
  145         VoteOption _voteOption,
  146         address _voter,
  147         bool _tryEarlyExecution
  148:     ) internal override {

  188     function _canVote(
  189         uint256 _proposalId,
  190         address _account,
  191         VoteOption _voteOption
  192:     ) internal view override returns (bool) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/token/TokenVoting.sol#L143-L148

contracts\src\plugins\utils\Addresslist.sol:

  59:     function _addAddresses(address[] calldata _newAddresses) internal virtual {

  77:     function _removeAddresses(address[] calldata _exitingAddresses) internal virtual {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/utils/Addresslist.sol#L59

[G-15] Do not calculate constants variables

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

34 results - 20 files:

contracts\src\core\dao\DAO.sol:

  40:     bytes32 public constant EXECUTE_PERMISSION_ID = keccak256("EXECUTE_PERMISSION");

  43:     bytes32 public constant UPGRADE_DAO_PERMISSION_ID = keccak256("UPGRADE_DAO_PERMISSION");

  46:     bytes32 public constant SET_METADATA_PERMISSION_ID = keccak256("SET_METADATA_PERMISSION");

  49:     bytes32 public constant SET_TRUSTED_FORWARDER_PERMISSION_ID =
  50          keccak256("SET_TRUSTED_FORWARDER_PERMISSION");

  53:     bytes32 public constant SET_SIGNATURE_VALIDATOR_PERMISSION_ID =
  54          keccak256("SET_SIGNATURE_VALIDATOR_PERMISSION");

  57:     bytes32 public constant REGISTER_STANDARD_CALLBACK_PERMISSION_ID =
  58          keccak256("REGISTER_STANDARD_CALLBACK_PERMISSION");

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L40-L58

contracts\src\core\permission\PermissionManager.sol:

  15:     bytes32 public constant ROOT_PERMISSION_ID = keccak256("ROOT_PERMISSION");

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/permission/PermissionManager.sol#L15

contracts\src\core\plugin\PluginUUPSUpgradeable.sol:

  35:     bytes32 public constant UPGRADE_PLUGIN_PERMISSION_ID = keccak256("UPGRADE_PLUGIN_PERMISSION");

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/plugin/PluginUUPSUpgradeable.sol#L35

contracts\src\core\utils\CallbackHandler.sol:

  14:     bytes4 internal constant UNREGISTERED_CALLBACK = bytes4(0);

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/utils/CallbackHandler.sol#L14

contracts\src\framework\dao\DAORegistry.sol:

  15:     bytes32 public constant REGISTER_DAO_PERMISSION_ID = keccak256("REGISTER_DAO_PERMISSION");

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/dao/DAORegistry.sol#L15

contracts\src\framework\plugin\repo\PluginRepo.sol:

  49:     bytes32 public constant MAINTAINER_PERMISSION_ID = keccak256("MAINTAINER_PERMISSION");

  52:     bytes32 public constant UPGRADE_REPO_PERMISSION_ID = keccak256("UPGRADE_REPO_PERMISSION");

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepo.sol#L49-L52

contracts\src\framework\plugin\repo\PluginRepoRegistry.sol:

  16:     bytes32 public constant REGISTER_PLUGIN_REPO_PERMISSION_ID =
  17          keccak256("REGISTER_PLUGIN_REPO_PERMISSION");

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepoRegistry.sol#L16-L17

contracts\src\framework\plugin\setup\PluginSetupProcessor.sol:

  27:     bytes32 public constant APPLY_INSTALLATION_PERMISSION_ID =
  28          keccak256("APPLY_INSTALLATION_PERMISSION");

  31:     bytes32 public constant APPLY_UPDATE_PERMISSION_ID = keccak256("APPLY_UPDATE_PERMISSION");

  34:     bytes32 public constant APPLY_UNINSTALLATION_PERMISSION_ID =
  35          keccak256("APPLY_UNINSTALLATION_PERMISSION");

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/setup/PluginSetupProcessor.sol#L27-L35

contracts\src\framework\utils\InterfaceBasedRegistry.sol:

  18:     bytes32 public constant UPGRADE_REGISTRY_PERMISSION_ID =
  19          keccak256("UPGRADE_REGISTRY_PERMISSION");

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/InterfaceBasedRegistry.sol#L18-L19

contracts\src\framework\utils\ens\ENSSubdomainRegistrar.sol:

  18:     bytes32 public constant UPGRADE_REGISTRAR_PERMISSION_ID =
  19          keccak256("UPGRADE_REGISTRAR_PERMISSION");

  22:     bytes32 public constant REGISTER_ENS_SUBDOMAIN_PERMISSION_ID =
  23          keccak256("REGISTER_ENS_SUBDOMAIN_PERMISSION");

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/ens/ENSSubdomainRegistrar.sol#L18-L23

contracts\src\plugins\counter-example\MultiplyHelper.sol:

  12:     bytes32 public constant MULTIPLY_PERMISSION_ID = keccak256("MULTIPLY_PERMISSION");

contracts\src\plugins\counter-example\v1\CounterV1.sol:

  14:     bytes32 public constant MULTIPLY_PERMISSION_ID = keccak256("MULTIPLY_PERMISSION");

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/counter-example/MultiplyHelper.sol#L12-L14

contracts\src\plugins\counter-example\v2\CounterV2.sol:

  14:     bytes32 public constant MULTIPLY_PERMISSION_ID = keccak256("MULTIPLY_PERMISSION");

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/counter-example/v2/CounterV2.sol#L14

contracts\src\plugins\governance\admin\Admin.sol:

  19:     bytes4 internal constant ADMIN_INTERFACE_ID =
  20          this.initialize.selector ^ this.executeProposal.selector;

  23:     bytes32 public constant EXECUTE_PROPOSAL_PERMISSION_ID =
  24          keccak256("EXECUTE_PROPOSAL_PERMISSION");

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/admin/Admin.sol#L19-L24

contracts\src\plugins\governance\majority-voting\MajorityVotingBase.sol:

  173:     bytes4 internal constant MAJORITY_VOTING_BASE_INTERFACE_ID =
  174          this.minDuration.selector ^
  175              this.minProposerVotingPower.selector ^
  176              this.votingMode.selector ^
  177              this.totalVotingPower.selector ^
  178              this.getProposal.selector ^
  179              this.updateVotingSettings.selector ^
  180              this.createProposal.selector;

  183:     bytes32 public constant UPDATE_VOTING_SETTINGS_PERMISSION_ID =
  184          keccak256("UPDATE_VOTING_SETTINGS_PERMISSION");

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/MajorityVotingBase.sol#L173-L184

contracts\src\plugins\governance\majority-voting\addresslist\AddresslistVoting.sol:

  23:     bytes4 internal constant ADDRESSLIST_VOTING_INTERFACE_ID =
  24          this.initialize.selector ^ this.addAddresses.selector ^ this.removeAddresses.selector;

  27:     bytes32 public constant UPDATE_ADDRESSES_PERMISSION_ID =
  28          keccak256("UPDATE_ADDRESSES_PERMISSION");

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/addresslist/AddresslistVoting.sol#L23-L28

contracts\src\plugins\governance\multisig\Multisig.sol:

  64:     bytes4 internal constant MULTISIG_INTERFACE_ID =
  65          this.initialize.selector ^
  66              this.updateMultisigSettings.selector ^
  67              this.createProposal.selector ^
  68              this.getProposal.selector;
   
  71:     bytes32 public constant UPDATE_MULTISIG_SETTINGS_PERMISSION_ID =
  72          keccak256("UPDATE_MULTISIG_SETTINGS_PERMISSION");

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/multisig/Multisig.sol#L64-L72

contracts\src\plugins\token\MerkleMinter.sol:

  24:     bytes32 public constant MERKLE_MINT_PERMISSION_ID = keccak256("MERKLE_MINT_PERMISSION");

  27:     bytes32 public constant CHANGE_DISTRIBUTOR_PERMISSION_ID =
  28          keccak256("CHANGE_DISTRIBUTOR_PERMISSION");

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/token/MerkleMinter.sol#L24-L28

contracts\src\token\ERC20\governance\GovernanceERC20.sol:
  28:     bytes32 public constant MINT_PERMISSION_ID = keccak256("MINT_PERMISSION");

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/token/ERC20/governance/GovernanceERC20.sol#L28

contracts\src\plugins\governance\majority-voting\token\TokenVoting.sol:

  22:     bytes4 internal constant TOKEN_VOTING_INTERFACE_ID =
  23          this.initialize.selector ^ this.getVotingToken.selector;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/token/TokenVoting.sol#L22-L23

Recommendation Code:

contracts\src\token\ERC20\governance\GovernanceERC20.sol:

- 28:     bytes32 public constant MINT_PERMISSION_ID = keccak256("MINT_PERMISSION");
             // MINT_PERMISSION_ID = keccak256("MINT_PERMISSION")
+ 28:     bytes32 public constant MINT_PERMISSION_ID = 0xb737b436e6cc542520cb79ec04245c720c38eebfa56d9e2d99b043979db20e4c;



contracts\src\plugins\governance\majority-voting\token\TokenVoting.sol:

- 22:     bytes4 internal constant TOKEN_VOTING_INTERFACE_ID =
- 23          this.initialize.selector ^ this.getVotingToken.selector;
              // TOKEN_VOTING_INTERFACE_ID = this.initialize.selector ^ this.getVotingToken.selector
+ 22:     bytes4 internal constant TOKEN_VOTING_INTERFACE_ID = 0x50eb001e;

[G-16] Use assembly to write address storage values

17 results - 13 files:

contracts\src\framework\plugin\repo\PluginRepoFactory.sol:

  22     constructor(PluginRepoRegistry _pluginRepoRegistry) {
  23:         pluginRepoRegistry = _pluginRepoRegistry;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepoFactory.sol#L23

contracts\src\framework\plugin\repo\PluginRepoRegistry.sol:

  41      function initialize(IDAO _dao, ENSSubdomainRegistrar _subdomainRegistrar) external initializer {
  45:         subdomainRegistrar = _subdomainRegistrar;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepoRegistry.sol#L45

contracts\src\framework\plugin\setup\PluginSetupProcessor.sol:
  277     constructor(PluginRepoRegistry _repoRegistry) {
  278:         repoRegistry = _repoRegistry;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/setup/PluginSetupProcessor.sol#L278

contracts\src\plugins\counter-example\v1\CounterV1.sol:

  26      function initialize(
  34:         multiplyHelper = _multiplyHelper;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/counter-example/v1/CounterV1.sol#L34

contracts\src\plugins\counter-example\v2\CounterV2.sol:

  32      function initialize(
  46:         multiplyHelper = _multiplyHelper;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/counter-example/v2/CounterV2.sol#L46

contracts\src\plugins\counter-example\v2\CounterV2PluginSetup.sol:

  24     constructor(MultiplyHelper _helper) {
  25:         multiplyHelperBase = _helper;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/counter-example/v2/CounterV2PluginSetup.sol#L25

contracts\src\core\dao\DAO.sol:

  283      function _setTrustedForwarder(address _trustedForwarder) internal {
  284:         trustedForwarder = _trustedForwarder;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L284

contracts\src\framework\dao\DAORegistry.sol:

  37      function initialize(
  42:         subdomainRegistrar = _subdomainRegistrar;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/dao/DAORegistry.sol#L42

contracts\src\core\plugin\dao-authorizable\DaoAuthorizableUpgradeable.sol:

  20      function __DaoAuthorizableUpgradeable_init(IDAO _dao) internal onlyInitializing {
  21:         dao_ = _dao;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/plugin/dao-authorizable/DaoAuthorizableUpgradeable.sol#L21

contracts\src\framework\utils\ens\ENSSubdomainRegistrar.sol:

  57     function initialize(IDAO _managingDao, ENS _ens, bytes32 _node) external initializer {
  60:         ens = _ens;
  69:         resolver = nodeResolver;

  100      function setDefaultResolver(
  107:         resolver = _resolver;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/ens/ENSSubdomainRegistrar.sol#L60

contracts\src\plugins\governance\majority-voting\token\TokenVoting.sol:

  36      function initialize(
  43:         votingToken = _token;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/token/TokenVoting.sol#L43

contracts\src\plugins\token\MerkleDistributor.sol:

  46      function initialize(
  52:         token = _token;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/token/MerkleDistributor.sol#L52

contracts\src\plugins\token\MerkleMinter.sol:

  41      function initialize(
  48:         token = _token;
  49:         distributorBase = _distributorBase;

  53     function changeDistributorBase(
  56:         distributorBase = _distributorBase;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/token/MerkleMinter.sol#L48-L49

Recommendation Code:

contracts\src\plugins\token\MerkleMinter.sol:

  53     function changeDistributorBase(
- 56:         distributorBase = _distributorBase;
+                  assembly {                      
+                      sstore(distributorBase.slot, _distributorBase)
+                  }                               
          

[G-17] Setting the constructor to payable

You can cut out 10 opcodes in the creation-time EVM bytecode if you declare a constructor payable. Making the constructor payable eliminates the need for an initial check of msg.value == 0 and saves 13 gas on deployment with no security risks.

21 results - 21 files:

src\core\dao\DAO.sol:
 
  92:     constructor() {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L92

src\core\plugin\Plugin.sol:

  17:     constructor(IDAO _dao) DaoAuthorizable(_dao) {}

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/plugin/Plugin.sol#L17

src\core\plugin\PluginCloneable.sol:

  16:     constructor() {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/plugin/PluginCloneable.sol#L16

src\core\plugin\PluginUUPSUpgradeable.sol:
 
  25:     constructor() {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/plugin/PluginUUPSUpgradeable.sol#L25

src\core\plugin\dao-authorizable\DaoAuthorizable.sol:

  19:     constructor(IDAO _dao) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/plugin/dao-authorizable/DaoAuthorizable.sol#L19

src\framework\dao\DAOFactory.sol:

  53:     constructor(DAORegistry _registry, PluginSetupProcessor _pluginSetupProcessor) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/dao/DAOFactory.sol#L53

src\framework\dao\DAORegistry.sol:
 
   30:     constructor() {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/dao/DAORegistry.sol#L30

src\framework\plugin\repo\PluginRepo.sol:
 
  112:     constructor() {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepo.sol#L112

src\framework\plugin\repo\PluginRepoFactory.sol:

  22:     constructor(PluginRepoRegistry _pluginRepoRegistry) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepoFactory.sol#L22

src\framework\plugin\repo\PluginRepoRegistry.sol:

  34:     constructor() {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepoRegistry.sol#L34

src\framework\plugin\setup\PluginSetupProcessor.sol:

  277:     constructor(PluginRepoRegistry _repoRegistry) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/setup/PluginSetupProcessor.sol#L277

src\framework\utils\TokenFactory.sol:
 
   68:     constructor() {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/TokenFactory.sol#L68

src\framework\utils\ens\ENSSubdomainRegistrar.sol:

  45:     constructor() {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/ens/ENSSubdomainRegistrar.sol#L45

src\plugins\counter-example\v1\CounterV1PluginSetup.sol:

  23:     constructor() {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/counter-example/v1/CounterV1PluginSetup.sol#L23

src\plugins\counter-example\v2\CounterV2PluginSetup.sol:

  24:     constructor(MultiplyHelper _helper) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/counter-example/v2/CounterV2PluginSetup.sol#L24

src\plugins\governance\admin\AdminSetup.sol:
 
  27:     constructor() {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/admin/AdminSetup.sol#L27

src\plugins\governance\majority-voting\addresslist\AddresslistVotingSetup.sol:

  20:     constructor() {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/addresslist/AddresslistVotingSetup.sol#L20

src\plugins\governance\majority-voting\token\TokenVotingSetup.sol:

  61:     constructor() {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/token/TokenVotingSetup.sol#L61

src\plugins\governance\multisig\MultisigSetup.sol:

  19:     constructor() {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/multisig/MultisigSetup.sol#L19

src\token\ERC20\governance\GovernanceERC20.sol:
 
  49:     constructor(

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/token/ERC20/governance/GovernanceERC20.sol#L49

src\token\ERC20\governance\GovernanceWrappedERC20.sol:
  
  38:     constructor(IERC20Upgradeable _token, string memory _name, string memory _symbol) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/token/ERC20/governance/GovernanceWrappedERC20.sol#L38

Recommendation: Set the constructor to payable

[G-18] Add unchecked {} for subtractions where the operands cannot underflow because of a previous require or if statement

require(a <= b); x = b - a => require(a <= b); unchecked { x = b - a } 
if(a <= b); x = b - a => if(a <= b); unchecked { x = b - a }

This will stop the check for overflow and underflow so it will save gas.

1 result - 1 file:

src\framework\plugin\repo\PluginRepo.sol:

  147:         if (_release > latestRelease) {
  148:             latestRelease = _release;

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepo.sol#L147-L148

[G-19] Use nested if and, avoid multiple check combinations

Using nested is cheaper than using && multiple check combinations. There are more advantages, such as easier to read code and better coverage reports.

11 results - 8 files:

src\core\permission\PermissionManager.sol:

  236:         if (_where == ANY_ADDR && _who == ANY_ADDR) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/permission/PermissionManager.sol#L236

src\framework\plugin\repo\PluginRepo.sol:

  157:         if (version.tag.release != 0 && version.tag.release != _release) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepo.sol#L157

src\framework\utils\RegistryUtils.sol:

  20:         if (char > 96 && char < 123) {

  25:         if (char > 47 && char < 58) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/RegistryUtils.sol#L20

src\plugins\governance\majority-voting\addresslist\AddresslistVoting.sol:

   97:         if (minProposerVotingPower() != 0 && !isListedAtBlock(_msgSender(), snapshotBlock)) {
  
  185:         if (_tryEarlyExecution && _canExecute(_proposalId)) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/addresslist/AddresslistVoting.sol#L97

src\plugins\governance\majority-voting\token\TokenVoting.sol:

  182:         if (_tryEarlyExecution && _canExecute(_proposalId)) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/token/TokenVoting.sol#L182

src\plugins\governance\multisig\Multisig.sol:

  216:         if (multisigSettings.onlyListed && !isListedAtBlock(_msgSender(), snapshotBlock)) {
  
  283:         if (_tryExecution && _canExecute(_proposalId)) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/multisig/Multisig.sol#L216

src\token\ERC20\governance\GovernanceERC20.sol:

  116:         if (to != address(0) && numCheckpoints(to) == 0 && delegates(to) == address(0)) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/token/ERC20/governance/GovernanceERC20.sol#L116

src\token\ERC20\governance\GovernanceWrappedERC20.sol:

  106:         if (to != address(0) && numCheckpoints(to) == 0 && delegates(to) == address(0)) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/token/ERC20/governance/GovernanceWrappedERC20.sol#L106

[G-20] Sort Solidity operations using short-circuit mode

Short-circuiting is a solidity contract development model that uses OR/AND logic to sequence different cost operations. It puts low gas cost operations in the front and high gas cost operations in the back, so that if the front is low If the cost operation is feasible, you can skip (short-circuit) the subsequent high-cost Ethereum virtual machine operation.

//f(x) is a low gas cost operation 
//g(y) is a high gas cost operation 

//Sort operations with different gas costs as follows 
f(x) || g(y) 
f(x) && g(y)

2 results - 1 file:

src\core\permission\PermissionManager.sol:

  240:    if (_where == ANY_ADDR || _who == ANY_ADDR) {

  242:    if (_permissionId == ROOT_PERMISSION_ID || isRestricted) {

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/permission/PermissionManager.sol#L240-L243

[G-21] Optimize names to save gas

Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions. 

Context:  All Contracts

Recommendation:  Find a lower method ID name for the most called functions for example Call() vs. Call1() is cheaper by 22 gas For example, the function IDs in the DAOl.sol contract will be the most used; A lower method ID may be given.

Proof of Consept: https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

DAO.sol function names can be named and sorted according to METHOD ID

Sighash   |   Function Signature
========================
eafb8b06  =>  initialize(bytes,address,address,string)
552b1863  =>  isPermissionRestrictedForAnyAddr(bytes32)
5ec29272  =>  _authorizeUpgrade(address)
da742228  =>  setTrustedForwarder(address)
ce1b815f  =>  getTrustedForwarder()
fdef9106  =>  hasPermission(address,address,bytes32,bytes)
ee57e36f  =>  setMetadata(bytes)
f01de670  =>  execute(bytes32,Action[],uint256)
bfe07da6  =>  deposit(address,uint256,string)
3e2ab0d9  =>  setSignatureValidator(address)
1626ba7e  =>  isValidSignature(bytes32,bytes)
4146c61e  =>  _setMetadata(bytes)
d292f98a  =>  _setTrustedForwarder(address)
f3e54f30  =>  _registerTokenInterfaces()
c4a50145  =>  registerStandardCallback(bytes4,bytes4,bytes4)
7034731b  =>  daoURI()
1080f99b  =>  setDaoURI(string)
f392bdc0  =>  _setDaoURI(string)

[G-22] Use a more recent version of solidity

In 0.8.15 the conditions necessary for inlining are relaxed. Benchmarks show that the change significantly decreases the bytecode size (which impacts the deployment cost) while the effect on the runtime gas usage is smaller.

In 0.8.17 prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call; Simplify the starting offset of zero-length operations to zero. More efficient overflow checks for multiplication.

All contracts in scope (62 files) are written in pragma solidity 0.8.17; and I recommend using the newer battle-tested version of Solidity 0.8.19.

Context: All contracts