Platform: Code4rena
Start Date: 03/03/2023
Pot Size: $90,500 USDC
Total HM: 4
Participants: 42
Period: 7 days
Judge: 0xean
Total Solo HM: 2
Id: 219
League: ETH
Rank: 42/42
Findings: 1
Award: $53.96
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: JCN
Also found by: 0x6980, 0xSmartContract, 0xnev, Madalad, Phantasmagoria, Rageur, RaymondFam, ReyAdmirado, Rolezn, Sathish9098, atharvasama, descharre, hunter_w3b, matrix_0wl, saneryee, volodya, yongskiws
53.963 USDC - $53.96
| Issue | Total Gas Saved | |
|---|---|---|
| [G-01] | Usage of uint/int smaller than 32 bytes (256 bits) incurs overhead | - |
| [G-02] | ++i / i++ should be unchecked{++i}/unchecked{i++} | 20 |
| [G-03] | Not using the named return variables when a function returns, wastes deployment gas | - |
| [G-04] | Use constants instead of type(uintx).max | - |
| [G-05] | Setting the constructor to payable | 143 |
| [G-06] | Multiple accesses of a function should use a local variable cache | - |
| [G-07] | Multiple accesses of a mapping/array should use a local variable cache | 126 |
| [G-08] | Use nested if and, avoid multiple check combinations | 24 |
| [G-09] | Internal/Private functions only called once can be inlined to save gas | 160 |
| [G-10] | Internal functions not called by the contract should be removed to save deployment gas | - |
| [G-11] | Duplicated if checks should be refactored to a modifier or function | - |
| [G-12] | Using storage instead of memory for structs/arrays saves gas | 2100 |
| [G-13] | Using immutable on variables that are only set in the constructor and never after | 4194 |
| [G-14] | Expressions for constant values such as a call to keccak256(), should use immutable rather than constant | - |
| [G-15] | abi.encode() is less efficient than abi.encodePacked() | - |
| [G-16] | Functions guaranteed to revert when called by normal users can be marked payable | 315 |
| [G-17] | Unused imports | - |
| [G-18] | Optimize names to save gas | 220 |
When using elements that are smaller than 32 bytes, your contract’s 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. Use a larger size then downcast where needed. https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html
/src/core/plugin/proposal/Proposal.sol 48: uint64 _startDate, 49: uint64 _endDate,
/src/core/plugin/proposal/ProposalUpgradeable.sol 48: uint64 _startDate, 49: uint64 _endDate,
/src/framework/plugin/setup/PluginSetup.sol 20: uint16 _currentBuild,
++i / i++ should be unchecked{++i}/unchecked{i++} When It Is Not Possible For Them To OverflowThe unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas PER LOOP
/src/framework/dao/DAOFactory.sol 95: for (uint256 i; i < _pluginSettings.length; ++i) {
Do not use return at the end of the function
/src/framework/plugin/setup/PluginSetupProcessor.sol 289: ) external returns (address plugin, IPluginSetup.PreparedSetupData memory preparedSetupData) {
/src/framework/plugin/setup/PluginSetupProcessor.sol 401: returns (bytes memory initData, IPluginSetup.PreparedSetupData memory preparedSetupData)
type(uint160).max uses more gas in the distribution process and also for each transaction than constant usage.
/src/core/permission/PermissionManager.sol 18: address internal constant ANY_ADDR = address(type(uint160).max);
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.
/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/framework/dao/DAOFactory.sol 53: constructor(DAORegistry _registry, PluginSetupProcessor _pluginSetupProcessor) {
/src/framework/utils/TokenFactory.sol 68: constructor() {
/src/framework/plugin/repo/PluginRepoFactory.sol 22: constructor(PluginRepoRegistry _pluginRepoRegistry) {
/src/framework/plugin/repo/PluginRepoRegistry.sol 34: constructor() {
/src/framework/plugin/setup/PluginSetupProcessor.sol 277: constructor(PluginRepoRegistry _repoRegistry) {
/src/framework/utils/ens/ENSSubdomainRegistrar.sol 45: constructor() {
/src/core/plugin/dao-authorizable/DaoAuthorizable.sol 19: constructor(IDAO _dao) {
/src/core/plugin/Plugin.sol 17: constructor(IDAO _dao) DaoAuthorizable(_dao) {}
/src/core/plugin/PluginCloneable.sol 16: constructor() {
/src/core/plugin/PluginUUPSUpgradeable.sol 25: constructor() {
It is better to cache result of a function call rather than re-calling the function GovernanceWrappedERC20(token)
/src/framework/utils/TokenFactory.sol 100: GovernanceWrappedERC20(token).initialize(
/src/framework/utils/TokenFactory.sol 106: emit WrappedToken(GovernanceWrappedERC20(token));
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/TokenFactory.sol#L106 GovernanceERC20(token)
/src/framework/utils/TokenFactory.sol 112: GovernanceERC20(token).initialize(
/src/framework/utils/TokenFactory.sol 130: bytes32 tokenMintPermission = GovernanceERC20(token).MINT_PERMISSION_ID();
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/TokenFactory.sol#L130 MerkleMinter(merkleMinter)
/src/framework/utils/TokenFactory.sol 121: MerkleMinter(merkleMinter).initialize(
/src/framework/utils/TokenFactory.sol 128: emit TokenCreated(IERC20Upgradeable(token), MerkleMinter(merkleMinter), distributorBase);
/src/framework/utils/TokenFactory.sol 131: bytes32 merkleMintPermission = MerkleMinter(merkleMinter).MERKLE_MINT_PERMISSION_ID();
/src/framework/utils/TokenFactory.sol 140: return (ERC20VotesUpgradeable(token), MerkleMinter(merkleMinter));
implementation()
527: address currentImplementation = PluginUUPSUpgradeable(_params.plugin).implementation(); 528: address newImplementation = PluginSetup(version.pluginSetup).implementation();
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.
permissionsHashed[permHash]
/src/core/permission/PermissionManager.sol 284: if (permissionsHashed[permHash] != UNSET_FLAG) { 285: permissionsHashed[permHash] = UNSET_FLAG;
/src/core/permission/PermissionManager.sol 253: address currentCondition = permissionsHashed[permHash];
/src/core/permission/PermissionManager.sol 258: permissionsHashed[permHash] = newCondition;
latestTagHashForPluginSetup[_pluginSetup]
/src/framework/plugin/repo/PluginRepo.sol 156: Version storage version = versions[latestTagHashForPluginSetup[_pluginSetup]];
/src/framework/plugin/repo/PluginRepo.sol 172: latestTagHashForPluginSetup[_pluginSetup] = _tagHash;
entries[_registrant]
/src/framework/utils/InterfaceBasedRegistry.sol 62: if (entries[_registrant]) {
/src/framework/utils/InterfaceBasedRegistry.sol 71: entries[_registrant] = true;
Instead of using the && operator in a single if statement to check multiple conditions,using multiple require statements with 1 condition per require statement will save 8 GAS per && The gas difference would only be realized if the revert condition is realized(met).
/src/framework/plugin/setup/PluginSetupProcessor.sol 700: if ( 701: msg.sender != _dao && 702: !DAO(payable(_dao)).hasPermission(address(this), msg.sender, _permissionId, bytes("")) 703: ) {
/src/framework/plugin/repo/PluginRepo.sol 157: if (version.tag.release != 0 && version.tag.release != _release) {
/src/core/permission/PermissionManager.sol 236: if (_where == ANY_ADDR && _who == ANY_ADDR) {
Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls. When we define internal functions to perform computation:
When it does not affect readability, it is recommended to inline functions in order to save gas
/src/framework/utils/TokenFactory.sol 144: function setupBases() private {
/src/core/dao/DAO.sol 290: function _registerTokenInterfaces() private {
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L290
/src/framework/dao/DAOFactory.sol 141: function _createDAO(DAOSettings calldata _daoSettings) internal returns (DAO dao) {
/src/framework/dao/DAOFactory.sol 156: function _setDAOPermissions(DAO _dao) internal {
/src/framework/plugin/repo/PluginRepoFactory.sol 65: function _setPluginRepoPermissions(PluginRepo pluginRepo, address maintainer) internal {
/src/framework/plugin/setup/PluginSetupProcessor.sol 664: function _upgradeProxy( 665: address _proxy, 666: address _implementation, 667: bytes memory _initData 668: ) private {
/src/framework/plugin/setup/PluginSetupProcessor.sol 669: function _canApply(address _dao, bytes32 _permissionId) private view {
/src/framework/plugin/setup/PluginSetupProcessor.sol 719: function emitPrepareUpdateEvent(
Consider to remove these functions
/src/core/permission/PermissionManager.sol 95: function __PermissionManager_init(address _initialOwner) internal onlyInitializing {
/src/core/dao/DAO.sol 122: function isPermissionRestrictedForAnyAddr(
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L122
Saves deployment costs
/src/framework/plugin/setup/PluginSetupProcessor.sol 321: if (pluginState.currentAppliedSetupId != bytes32(0)) {
/src/framework/plugin/setup/PluginSetupProcessor.sol 326: if (pluginState.blockNumber < pluginState.preparedSetupIdToBlockNumber[preparedSetupId]) {
/src/framework/plugin/setup/PluginSetupProcessor.sol 378: if (_params.permissions.length > 0) {
/src/framework/plugin/setup/PluginSetupProcessor.sol 426: if (pluginState.currentAppliedSetupId != appliedSetupId) {
/src/framework/plugin/repo/PluginRepo.sol 150: if (_releaseMetadata.length == 0) {
/src/framework/plugin/repo/PluginRepo.sol 138: if (_release == 0) {
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
/src/framework/plugin/repo/PluginRepo.sol 167: Tag memory tag = Tag(_release, build);
Use immutable if you want to assign a permanent value at construction. Use constants if you already know the permanent value. Both get directly embedded in bytecode, saving SLOAD. Variables only set in the constructor and never edited afterwards should be marked as immutable, as it would avoid the expensive storage-writing operation in the constructor (around 20 000 gas per variable) and replace the expensive storage-reading operations (around 2100 gas per reading) to a less expensive value reading (3 gas)
/src/framework/plugin/repo/PluginRepoFactory.sol 25: pluginRepoBase = address(new PluginRepo());
/src/framework/plugin/setup/PluginSetupProcessor.sol 278: repoRegistry = _repoRegistry;
See this issue for a detail description of the issue
/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 =
/src/framework/utils/ens/ENSSubdomainRegistrar.sol 22: bytes32 public constant REGISTER_ENS_SUBDOMAIN_PERMISSION_ID =
/src/framework/plugin/setup/PluginSetupProcessor.sol 27: bytes32 public constant APPLY_INSTALLATION_PERMISSION_ID =
/src/framework/plugin/setup/PluginSetupProcessor.sol 31: bytes32 public constant APPLY_UPDATE_PERMISSION_ID = keccak256("APPLY_UPDATE_PERMISSION");
/src/framework/plugin/setup/PluginSetupProcessor.sol 34: bytes32 public constant APPLY_UNINSTALLATION_PERMISSION_ID =
/src/framework/plugin/repo/PluginRepoRegistry.sol 16: bytes32 public constant REGISTER_PLUGIN_REPO_PERMISSION_ID =
/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");
/src/framework/plugin/repo/PluginRepo.sol 52: bytes32 public constant UPGRADE_REPO_PERMISSION_ID = keccak256("UPGRADE_REPO_PERMISSION");
/src/core/dao/DAO.sol 40: bytes32 public constant EXECUTE_PERMISSION_ID = keccak256("EXECUTE_PERMISSION");
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L40
/src/core/dao/DAO.sol 43: bytes32 public constant UPGRADE_DAO_PERMISSION_ID = keccak256("UPGRADE_DAO_PERMISSION");
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L43
/src/core/dao/DAO.sol 46: bytes32 public constant SET_METADATA_PERMISSION_ID = keccak256("SET_METADATA_PERMISSION");
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L46
/src/core/dao/DAO.sol 50: bytes32 public constant SET_TRUSTED_FORWARDER_PERMISSION_ID =
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L50
/src/core/dao/DAO.sol 53: bytes32 public constant SET_SIGNATURE_VALIDATOR_PERMISSION_ID =
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L53
/src/core/dao/DAO.sol 57: bytes32 public constant REGISTER_STANDARD_CALLBACK_PERMISSION_ID =
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L57
/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");
Changing abi.encode function to abi.encodePacked can save gas since the abi.encode function pads extra null bytes at the end of the call data, which is unnecessary. Also, in general, abi.encodePacked is more gas-efficient (see Solidity-Encode-Gas-Comparison) Consider using abi.encodePacked() here:
/src/framework/plugin/setup/PluginSetupProcessorHelpers.sol 33: return keccak256(abi.encode(_dao, _plugin));
/src/framework/plugin/setup/PluginSetupProcessorHelpers.sol 52: abi.encode( 53: _pluginSetupRef.versionTag, 54: _pluginSetupRef.pluginSetupRepo, 55: _permissionsHash, 56: _helpersHash, 57: keccak256(_data), 58: _preparationType 59: )
/src/framework/plugin/setup/PluginSetupProcessorHelpers.sol 73: abi.encode(_pluginSetupRef.versionTag, _pluginSetupRef.pluginSetupRepo, _helpersHash)
/src/framework/plugin/setup/PluginSetupProcessorHelpers.sol 80: return keccak256(abi.encode(_helpers));
/src/framework/plugin/setup/PluginSetupProcessorHelpers.sol 89: return keccak256(abi.encode(_permissions));
If a function modifier such as auth() is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2) which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.
/src/framework/utils/ens/ENSSubdomainRegistrar.sol 82: function registerSubnode( 83: bytes32 _label, 84: address _targetAddress 85: ) external auth(REGISTER_ENS_SUBDOMAIN_PERMISSION_ID) {
/src/framework/utils/ens/ENSSubdomainRegistrar.sol 100: function setDefaultResolver( 101: address _resolver 102: ) external auth(REGISTER_ENS_SUBDOMAIN_PERMISSION_ID) {
/src/framework/plugin/repo/PluginRepoRegistry.sol 51: function registerPluginRepo( 52: string calldata subdomain, 53: address pluginRepo 54: ) external auth(REGISTER_PLUGIN_REPO_PERMISSION_ID) {
/src/framework/plugin/repo/PluginRepo.sol 128: function createVersion( 129: uint8 _release, 130: address _pluginSetup, 131: bytes calldata _buildMetadata, 132: bytes calldata _releaseMetadata 133: ) external auth(MAINTAINER_PERMISSION_ID) {
/src/framework/plugin/repo/PluginRepo.sol 187: function updateReleaseMetadata( 188: uint8 _release, 189: bytes calldata _releaseMetadata 190: ) external auth(MAINTAINER_PERMISSION_ID) {
/src/framework/dao/DAORegistry.sol 50: function register( 51: IDAO dao, 52: address creator, 53: string calldata subdomain 54: ) external auth(REGISTER_DAO_PERMISSION_ID) {
/src/core/dao/DAO.sol 139: function setTrustedForwarder( 140: address _newTrustedForwarder 141: ) external override auth(SET_TRUSTED_FORWARDER_PERMISSION_ID) {
/src/core/dao/DAO.sol 161: function setMetadata( 162: bytes calldata _metadata 163: ) external override auth(SET_METADATA_PERMISSION_ID) {
/src/core/dao/DAO.sol 168: function execute( 169: bytes32 _callId, 170: Action[] calldata _actions, 171: uint256 _allowFailureMap 172: ) 173: external 174: override 175: auth(EXECUTE_PERMISSION_ID) 176: returns (bytes[] memory execResults, uint256 failureMap)
/src/core/dao/DAO.sol 239: function setSignatureValidator( 240: address _signatureValidator 241: ) external override auth(SET_SIGNATURE_VALIDATOR_PERMISSION_ID) {
/src/core/dao/DAO.sol 309: function registerStandardCallback( 310: bytes4 _interfaceId, 311: bytes4 _callbackSelector, 312: bytes4 _magicNumber 313: ) external override auth(REGISTER_STANDARD_CALLBACK_PERMISSION_ID) {
/src/core/dao/DAO.sol 326: function setDaoURI(string calldata newDaoURI) external auth(SET_METADATA_PERMISSION_ID) {
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L326
/src/core/permission/PermissionManager.sol 105: function grant( 106: address _where, 107: address _who, 108: bytes32 _permissionId 109: ) external virtual auth(ROOT_PERMISSION_ID) {
/src/core/permission/PermissionManager.sol 120: function grantWithCondition( 121: address _where, 122: address _who, 123: bytes32 _permissionId, 124: IPermissionCondition _condition 125: ) external virtual auth(ROOT_PERMISSION_ID) {
/src/core/permission/PermissionManager.sol 167: function applyMultiTargetPermissions( 168: PermissionLib.MultiTargetPermission[] calldata _items 169: ) external virtual auth(ROOT_PERMISSION_ID) {
/src/core/dao/DAO.sol 11: import "@openzeppelin/contracts-upgradeable/token/ERC1155/IERC1155Upgradeable.sol";
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L11
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. See this for more details https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/plugin/PluginUUPSUpgradeable.sol supportsInterface(), implementation()
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/setup/PluginSetup.sol prepareUpdate()
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/permission/PermissionManager.sol isGranted(), grant(), grantWithCondition(), revoke(), applySingleTargetPermissions(), applyMultiTargetPermissions()
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol setTrustedForwarder(), hasPermission(), setMetadata(), execute(), deposit(), setSignatureValidator(), isValidSignature()
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/dao/DAOFactory.sol createDao()
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepo.sol createVersion(), updateReleaseMetadata(), getLatestVersion(), getVersion(), buildCount()
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepoFactory.sol createPluginRepo(), createPluginRepoWithFirstVersion()
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/repo/PluginRepoRegistry.sol registerPluginRepo()
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/plugin/setup/PluginSetupProcessor.sol validatePreparedSetupId(), prepareInstallation(), applyInstallation(), prepareUpdate(), applyUpdate(), prepareUninstallation(), applyUninstallation()
#0 - c4-judge
2023-03-12T18:29:33Z
0xean marked the issue as grade-b
#1 - c4-sponsor
2023-03-22T13:15:06Z
novaknole20 marked the issue as sponsor acknowledged