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: 10/42
Findings: 2
Award: $774.31
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: rbserver
Also found by: 0x6980, 0xAgro, 0xSmartContract, 0xmichalis, 0xnev, BRONZEDISC, DevABDee, IceBear, RaymondFam, Rolezn, SaeedAlipoor01988, Sathish9098, arialblack14, brgltd, chrisdior4, codeislight, descharre, imare, lukris02, luxartvinsec, matrix_0wl, tnevler, yongskiws
720.3467 USDC - $720.35
| Issue | Contexts | |
|---|---|---|
| LOW‑1 | Draft Import Dependencies | 3 |
| LOW‑2 | decimals() not part of ERC20 standard | 1 |
| LOW‑3 | Possible rounding issue | 5 |
| LOW‑4 | Minting tokens to the zero address should be avoided | 2 |
| LOW‑5 | Missing Contract-existence Checks Before Low-level Calls | 1 |
| LOW‑6 | Contracts are not using their OZ Upgradeable counterparts | 20 |
| LOW‑7 | No Storage Gap For Upgradeable Contracts | 4 |
Total: 36 contexts over 7 issues
| Issue | Contexts | |
|---|---|---|
| NC‑1 | Add a timelock to critical functions | 8 |
| NC‑2 | Constants Should Be Defined Rather Than Using Magic Numbers | 2 |
| NC‑3 | Constants in comparisons should appear on the left side | 3 |
| NC‑4 | Expressions for constant values such as a call to keccak256(), should use immutable rather than constant | 29 |
| NC‑5 | Critical Changes Should Use Two-step Procedure | 8 |
| NC‑6 | Duplicated require()/revert() Checks Should Be Refactored To A Modifier Or Function | 2 |
| NC‑7 | Large or complicated code bases should implement fuzzing tests | 1 |
| NC‑8 | Imports can be grouped together | 82 |
| NC‑9 | NatSpec return parameters should be included in contracts | 1 |
| NC‑10 | Initial value check is missing in Set Functions | 7 |
| NC‑11 | Contracts should have full test coverage | 1 |
| NC‑12 | Lines are too long | 173 |
| NC‑13 | Missing event for critical parameter change | 4 |
| NC‑14 | Implementation contract may not be initialized | 21 |
| NC‑15 | Non-usage of specific imports | 4 |
| NC‑16 | Use a more recent version of Solidity | 62 |
| NC‑17 | Omissions in Events | 1 |
| NC‑18 | Empty blocks should be removed or emit something | 11 |
| NC‑19 | Use bytes.concat() | 4 |
| NC‑20 | Implement some type of version counter that will be incremented automatically for contract upgrades | 5 |
Total: 429 contexts over 20 issues
Contracts are importing draft dependencies. These imported contracts are still a draft and are not considered ready for mainnet use and have not received adequate security auditing or are liable to change with future development.
6: import {IERC1822ProxiableUpgradeable} from "@openzeppelin/contracts-upgradeable/interfaces/draft-IERC1822Upgradeable.sol";
5: import {IERC20PermitUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/draft-IERC20PermitUpgradeable.sol";
8: import {IERC20PermitUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/draft-IERC20PermitUpgradeable.sol";
Stop using draft imported contracts and use their release version if it exists.
decimals() not part of ERC20 standarddecimals() is not part of the official ERC20 standard and might fail for tokens that do not implement it. While in practice it is very unlikely, as usually most of the tokens implement it, this should still be considered as a potential issue.
77: return ERC20WrapperUpgradeable.decimals()
Division by large numbers may result in the result being zero, due to solidity not supporting fractions. Consider requiring a minimum amount for the numerator to ensure that it is always larger than the denominator
The function _applyRatioCeiled states that "_ratio The ratio that must be in the interval [0, 10**6]." but there is no actual limitation to the input of _ratio and any uint can be set between 0 to 10**6.
24: result = _value / RATIO_BASE;
The core function mint is used by users to mint an option position by providing a token as collateral and borrowing the max amount of liquidity. Address(0) check is missing in both this function and the internal function _mint, which is triggered to mint the tokens to the to address. Consider applying a check in the function to ensure tokens aren't minted to the zero address.
function mint(address to, uint256 amount) external override auth(MINT_PERMISSION_ID) { _mint(to, amount); }
function _mint(address account, uint256 amount) internal virtual override { super._mint(account, amount); require(totalSupply() <= _maxSupply(), "ERC20Votes: total supply risks overflowing votes"); _writeCheckpoint(_totalSupplyCheckpoints, _add, amount); }
Low-level calls return success if there is no code present at the specified address.
186: (bool success, bytes memory response) = to.call{value: _actions[i].value}(
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L186
In addition to the zero-address checks, add a check to verify that <address>.code.length > 0
The non-upgradeable standard version of OpenZeppelin’s library are inherited / used by the contracts. It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour.
14: import "@openzeppelin/contracts/interfaces/IERC1271.sol";
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L14
5: import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol";
5: import {Context} from "@openzeppelin/contracts/utils/Context.sol";
5: import {Counters} from "@openzeppelin/contracts/utils/Counters.sol"; 6: import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol";
5: import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol"; 6: import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol"; 7: import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol";
5: import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol";
5: import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol"; 6: import {Address} from "@openzeppelin/contracts/utils/Address.sol";
5: import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol";
5: import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol";
5: import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol";
5: import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol"; 6: import {Address} from "@openzeppelin/contracts/utils/Address.sol"; 7: import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol";
10: import {MerkleProof} from "@openzeppelin/contracts/utils/cryptography/MerkleProof.sol";
5: import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol";
5: import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/utils/Proxy.sol#L5
Where applicable, use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts.
See https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/tree/master/contracts for list of available upgradeable contracts
For upgradeable contracts, there must be storage gap to "allow developers to freely add new state variables in the future without compromising the storage compatibility with existing deployments". Otherwise it may be very difficult to write new implementation code. Without storage gap, the variable in child contract might be overwritten by the upgraded base contract if new variables are added to the base contract. This could have unintended and very serious consequences to the child contracts.
Refer to the bottom part of this article: https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable
However, the contract doesn't contain a storage gap. The storage gap is essential for upgradeable contract because "It allows us to freely add new state variables in the future without compromising the storage compatibility with existing deployments". See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.
contract GovernanceERC20 is IERC20MintableUpgradeable, Initializable, ERC165Upgradeable, ERC20VotesUpgradeable, DaoAuthorizableUpgradeable {
contract MultiplyHelper is PluginUUPSUpgradeable {
contract Admin is IMembership, PluginCloneable, ProposalUpgradeable {
contract GovernanceWrappedERC20 is IGovernanceWrappedERC20, Initializable, ERC165Upgradeable, ERC20VotesUpgradeable, ERC20WrapperUpgradeable {
Recommend adding appropriate storage gap at the end of upgradeable contracts such as the below. Please reference OpenZeppelin upgradeable contract templates.
uint256[50] private __gap;
It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user). Consider adding a timelock to the following functions:
139: function setTrustedForwarder(
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L139
161: function setMetadata(
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L161
239: function setSignatureValidator(
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L239
326: function setDaoURI(string calldata newDaoURI) external auth(SET_METADATA_PERMISSION_ID) {
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L326
144: function setupBases() private {
100: function setDefaultResolver(
52: function setNewVariable(uint256 _newVariable) external reinitializer(2) {
53: function changeDistributorBase(
540: if (_votingSettings.minDuration < 60 minutes) {
544: if (_votingSettings.minDuration > 365 days) {
Doing so will prevent <a href="https://www.moserware.com/2008/01/constants-on-left-are-better-but-this.html">typo bugs</a>
30: if (char == 45) {
87: if (totalVotingPower_ == 0) {
220: if (_startDate == 0) {
keccak256(), should use immutable rather than constantWhile 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. There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts. constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.
40: bytes32 public constant EXECUTE_PERMISSION_ID = keccak256("EXECUTE_PERMISSION");
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L40
43: bytes32 public constant UPGRADE_DAO_PERMISSION_ID = keccak256("UPGRADE_DAO_PERMISSION");
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L43
46: bytes32 public constant SET_METADATA_PERMISSION_ID = keccak256("SET_METADATA_PERMISSION");
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L46
49: bytes32 public constant SET_TRUSTED_FORWARDER_PERMISSION_ID = keccak256("SET_TRUSTED_FORWARDER_PERMISSION");
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L49
53: bytes32 public constant SET_SIGNATURE_VALIDATOR_PERMISSION_ID = keccak256("SET_SIGNATURE_VALIDATOR_PERMISSION");
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L53
57: bytes32 public constant REGISTER_STANDARD_CALLBACK_PERMISSION_ID = keccak256("REGISTER_STANDARD_CALLBACK_PERMISSION");
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L57
15: bytes32 public constant ROOT_PERMISSION_ID = keccak256("ROOT_PERMISSION");
35: bytes32 public constant UPGRADE_PLUGIN_PERMISSION_ID = keccak256("UPGRADE_PLUGIN_PERMISSION");
15: bytes32 public constant REGISTER_DAO_PERMISSION_ID = keccak256("REGISTER_DAO_PERMISSION");
49: bytes32 public constant MAINTAINER_PERMISSION_ID = keccak256("MAINTAINER_PERMISSION");
52: bytes32 public constant UPGRADE_REPO_PERMISSION_ID = keccak256("UPGRADE_REPO_PERMISSION");
16: bytes32 public constant REGISTER_PLUGIN_REPO_PERMISSION_ID = keccak256("REGISTER_PLUGIN_REPO_PERMISSION");
27: bytes32 public constant APPLY_INSTALLATION_PERMISSION_ID = keccak256("APPLY_INSTALLATION_PERMISSION");
31: bytes32 public constant APPLY_UPDATE_PERMISSION_ID = keccak256("APPLY_UPDATE_PERMISSION");
34: bytes32 public constant APPLY_UNINSTALLATION_PERMISSION_ID = keccak256("APPLY_UNINSTALLATION_PERMISSION");
39: bytes32 private constant EMPTY_ARRAY_HASH = 0x569e75fc77c1a856f6daaf9e69d8a9566ca34aa47f9133711ce065a571af0cfd; /// @notice The hash obtained from the bytes-encoded zero value. /// @dev The hash is computed via `keccak256(abi.encode(0))`.
18: bytes32 public constant UPGRADE_REGISTRY_PERMISSION_ID = keccak256("UPGRADE_REGISTRY_PERMISSION");
18: bytes32 public constant UPGRADE_REGISTRAR_PERMISSION_ID = keccak256("UPGRADE_REGISTRAR_PERMISSION");
22: bytes32 public constant REGISTER_ENS_SUBDOMAIN_PERMISSION_ID = keccak256("REGISTER_ENS_SUBDOMAIN_PERMISSION");
12: bytes32 public constant MULTIPLY_PERMISSION_ID = keccak256("MULTIPLY_PERMISSION");
14: bytes32 public constant MULTIPLY_PERMISSION_ID = keccak256("MULTIPLY_PERMISSION");
14: bytes32 public constant MULTIPLY_PERMISSION_ID = keccak256("MULTIPLY_PERMISSION");
23: bytes32 public constant EXECUTE_PROPOSAL_PERMISSION_ID = keccak256("EXECUTE_PROPOSAL_PERMISSION");
183: bytes32 public constant UPDATE_VOTING_SETTINGS_PERMISSION_ID = keccak256("UPDATE_VOTING_SETTINGS_PERMISSION");
27: bytes32 public constant UPDATE_ADDRESSES_PERMISSION_ID = keccak256("UPDATE_ADDRESSES_PERMISSION");
71: bytes32 public constant UPDATE_MULTISIG_SETTINGS_PERMISSION_ID = keccak256("UPDATE_MULTISIG_SETTINGS_PERMISSION");
24: bytes32 public constant MERKLE_MINT_PERMISSION_ID = keccak256("MERKLE_MINT_PERMISSION");
27: bytes32 public constant CHANGE_DISTRIBUTOR_PERMISSION_ID = keccak256("CHANGE_DISTRIBUTOR_PERMISSION");
28: bytes32 public constant MINT_PERMISSION_ID = keccak256("MINT_PERMISSION");
The critical procedures should be two step process.
See similar findings in previous Code4rena contests for reference: https://code4rena.com/reports/2022-06-illuminate/#2-critical-changes-should-use-two-step-procedure
139: function setTrustedForwarder(
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L139
161: function setMetadata(
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L161
239: function setSignatureValidator(
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L239
326: function setDaoURI(string calldata newDaoURI) external auth(SET_METADATA_PERMISSION_ID) {
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L326
144: function setupBases() private {
100: function setDefaultResolver(
52: function setNewVariable(uint256 _newVariable) external reinitializer(2) {
53: function changeDistributorBase(
Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.
require()/revert() Checks Should Be Refactored To A Modifier Or FunctionSaves deployment costs
673: revert(reason); 685: revert(reason);
Large code bases, or code with lots of inline-assembly, complicated math, or complicated interactions between multiple contracts, should implement <a href="https://medium.com/coinmonks/smart-contract-fuzzing-d9b88e0b0a05">fuzzing tests</a>. Fuzzers such as Echidna require the test writer to come up with invariants which should not be violated under any circumstances, and the fuzzer tests various inputs and function calls to ensure that the invariants always hold. Even code with 100% code coverage can still have bugs due to the order of the operations a user performs, and fuzzers, with properly and extensively-written invariants, can close this testing gap significantly.
Various in-scope contract files.
Consider importing OZ first, then all interfaces, then all utils.
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"; 16: import {PermissionManager} from "../permission/PermissionManager.sol"; 17: import {CallbackHandler} from "../utils/CallbackHandler.sol"; 18: import {hasBit, flipBit} from "../utils/BitMap.sol"; 19: import {IEIP4824} from "./IEIP4824.sol"; 20: import {IDAO} from "./IDAO.sol";
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L5
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L6
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L7
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L8
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L9
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L10
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L11
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L12
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L13
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L14
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L16
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L17
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L18
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L19
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L20
5: import {DAO} from "../../core/dao/DAO.sol"; 6: import {PermissionLib} from "../../core/permission/PermissionLib.sol"; 7: import {createERC1967Proxy} from "../../utils/Proxy.sol"; 8: import {PluginRepo} from "../plugin/repo/PluginRepo.sol"; 9: import {PluginSetupProcessor} from "../plugin/setup/PluginSetupProcessor.sol"; 10: import {hashHelpers, PluginSetupRef} from "../plugin/setup/PluginSetupProcessorHelpers.sol"; 11: import {IPluginSetup} from "../plugin/setup/IPluginSetup.sol"; 12: import {DAORegistry} from "./DAORegistry.sol";
5: import {IDAO} from "../../core/dao/IDAO.sol"; 6: import {ENSSubdomainRegistrar} from "../utils/ens/ENSSubdomainRegistrar.sol"; 7: import {InterfaceBasedRegistry} from "../utils/InterfaceBasedRegistry.sol"; 8: import {isSubdomainValid} from "../utils/RegistryUtils.sol";
5: import {IDAO} from "../../../core/dao/IDAO.sol"; 6: import {ENSSubdomainRegistrar} from "../../utils/ens/ENSSubdomainRegistrar.sol"; 7: import {InterfaceBasedRegistry} from "../../utils/InterfaceBasedRegistry.sol"; 8: import {isSubdomainValid} from "../../utils/RegistryUtils.sol"; 9: import {IPluginRepo} from "./IPluginRepo.sol";
5: import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol"; 6: import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol"; 7: import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol"; 9: import {PermissionLib} from "../../../core/permission/PermissionLib.sol"; 10: import {createERC1967Proxy as createERC1967} from "../../../utils/Proxy.sol"; 11: import {IPluginSetup} from "./IPluginSetup.sol";
5: import "@ensdomains/ens-contracts/contracts/registry/ENS.sol"; 6: import "@ensdomains/ens-contracts/contracts/resolvers/Resolver.sol"; 8: import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; 10: import {DaoAuthorizableUpgradeable} from "../../../core/plugin/dao-authorizable/DaoAuthorizableUpgradeable.sol"; 11: import {IDAO} from "../../../core/dao/IDAO.sol";
5: import {ERC165Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol"; 6: import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; 7: import {SafeCastUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/math/SafeCastUpgradeable.sol"; 9: import {IProposal} from "../../../core/plugin/proposal/IProposal.sol"; 10: import {ProposalUpgradeable} from "../../../core/plugin/proposal/ProposalUpgradeable.sol"; 11: import {PluginUUPSUpgradeable} from "../../../core/plugin/PluginUUPSUpgradeable.sol"; 12: import {IDAO} from "../../../core/dao/IDAO.sol"; 13: import {RATIO_BASE, RatioOutOfBounds} from "../../utils/Ratio.sol"; 14: import {IMajorityVoting} from "./IMajorityVoting.sol";
5: import {SafeCastUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/math/SafeCastUpgradeable.sol"; 7: import {IDAO} from "../../../../core/dao/IDAO.sol"; 8: import {RATIO_BASE, _applyRatioCeiled} from "../../../utils/Ratio.sol"; 10: import {IMembership} from "../../../../core/plugin/membership/IMembership.sol"; 11: import {Addresslist} from "../../../utils/Addresslist.sol"; 12: import {IMajorityVoting} from "../IMajorityVoting.sol"; 13: import {MajorityVotingBase} from "../MajorityVotingBase.sol";
5: import {IVotesUpgradeable} from "@openzeppelin/contracts-upgradeable/governance/utils/IVotesUpgradeable.sol"; 6: import {SafeCastUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/math/SafeCastUpgradeable.sol"; 8: import {IMembership} from "../../../../core/plugin/membership/IMembership.sol"; 9: import {IDAO} from "../../../../core/dao/IDAO.sol"; 10: import {RATIO_BASE, _applyRatioCeiled} from "../../../utils/Ratio.sol"; 11: import {MajorityVotingBase} from "../MajorityVotingBase.sol"; 12: import {IMajorityVoting} from "../IMajorityVoting.sol";
5: import {SafeCastUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/math/SafeCastUpgradeable.sol"; 7: import {IDAO} from "../../../core/dao/IDAO.sol"; 8: import {IMembership} from "../../../core/plugin/membership/IMembership.sol"; 9: import {PluginUUPSUpgradeable} from "../../../core/plugin/PluginUUPSUpgradeable.sol"; 11: import {ProposalUpgradeable} from "../../../core/plugin/proposal/ProposalUpgradeable.sol"; 12: import {Addresslist} from "../../utils/Addresslist.sol"; 13: import {IMultisig} from "./IMultisig.sol";
5: import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol"; 6: import {ERC165Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol"; 7: import {IERC20Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol"; 9: import {IDAO} from "../../core/dao/IDAO.sol"; 10: import {PluginUUPSUpgradeable} from "../../core/plugin/PluginUUPSUpgradeable.sol"; 11: import {IERC20MintableUpgradeable} from "../../token/ERC20/IERC20MintableUpgradeable.sol"; 12: import {createERC1967Proxy} from "../../utils/Proxy.sol"; 13: import {IMerkleDistributor} from "./IMerkleDistributor.sol"; 14: import {MerkleDistributor} from "./MerkleDistributor.sol"; 15: import {IMerkleMinter} from "./IMerkleMinter.sol";
It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.
https://docs.soliditylang.org/en/v0.8.15/natspec-format.html
function _createDAO(DAOSettings calldata _daoSettings) internal returns (DAO dao) { // create dao dao = DAO(payable(createERC1967Proxy(daoBase, bytes("")))); // initialize the DAO and give the `ROOT_PERMISSION_ID` permission to this contract. dao.initialize( _daoSettings.metadata, address(this), _daoSettings.trustedForwarder, _daoSettings.daoURI ); }
Include return parameters in NatSpec comments
Recommendation Code Style: (from Uniswap3)
/// @notice Adds liquidity for the given recipient/tickLower/tickUpper position /// @dev The caller of this method receives a callback in the form of IUniswapV3MintCallback#uniswapV3MintCallback /// in which they must pay any token0 or token1 owed for the liquidity. The amount of token0/token1 due depends /// on tickLower, tickUpper, the amount of liquidity, and the current price. /// @param recipient The address for which the liquidity will be created /// @param tickLower The lower tick of the position in which to add liquidity /// @param tickUpper The upper tick of the position in which to add liquidity /// @param amount The amount of liquidity to mint /// @param data Any data that should be passed through to the callback /// @return amount0 The amount of token0 that was paid to mint the given amount of liquidity. Matches the value in the callback /// @return amount1 The amount of token1 that was paid to mint the given amount of liquidity. Matches the value in the callback function mint( address recipient, int24 tickLower, int24 tickUpper, uint128 amount, bytes calldata data ) external returns (uint256 amount0, uint256 amount1);
A check regarding whether the current value and the new value are the same should be added
139: function setTrustedForwarder( address _newTrustedForwarder ) external override auth(SET_TRUSTED_FORWARDER_PERMISSION_ID) { _setTrustedForwarder(_newTrustedForwarder); }
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L139
161: function setMetadata( bytes calldata _metadata ) external override auth(SET_METADATA_PERMISSION_ID) { _setMetadata(_metadata); }
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L161
239: function setSignatureValidator( address _signatureValidator ) external override auth(SET_SIGNATURE_VALIDATOR_PERMISSION_ID) { signatureValidator = IERC1271(_signatureValidator); emit SignatureValidatorSet({signatureValidator: _signatureValidator}); }
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L239
326: function setDaoURI(string calldata newDaoURI) external auth(SET_METADATA_PERMISSION_ID) { _setDaoURI(newDaoURI); }
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L326
144: function setupBases() private { distributorBase = new MerkleDistributor(); governanceERC20Base = address( new GovernanceERC20( IDAO(address(0)), "baseName", "baseSymbol", GovernanceERC20.MintSettings(new address[](0), new uint256[](0)) ) ); governanceWrappedERC20Base = address( new GovernanceWrappedERC20(IERC20Upgradeable(address(0)), "baseName", "baseSymbol") ); merkleMinterBase = address(new MerkleMinter()); } }
52: function setNewVariable(uint256 _newVariable) external reinitializer(2) { newVariable = _newVariable; }
53: function changeDistributorBase( IMerkleDistributor _distributorBase ) external override auth(CHANGE_DISTRIBUTOR_PERMISSION_ID) { distributorBase = _distributorBase; }
The test coverage rate of the project is 80%. Testing all functions is best practice in terms of security criteria. While 80% code coverage does not guarantee that there are no bugs, it often will catch easy-to-find bugs, and will ensure that there are fewer regressions when the code invariably has to be modified. Furthermore, in order to get full coverage, code authors will often have to re-organize their code so that it is more modular, so that each component can be tested separately, which reduces interdependencies between modules and layers, and makes for code that is easier to reason about and audit.
As stated in the documentation of the contest that it is 60% but in actuality it is 80% when running the tests:
- What is the overall line coverage percentage provided by your tests?: 60
Due to its capacity, test coverage is expected to be 100%.
Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length
134: /// @notice Internal method authorizing the upgrade of the contract via the [upgradeabilty mechanism for UUPS proxies](https://docs.openzeppelin.com/contracts/4.x/api/proxy#UUPSUpgradeable) (see [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822)).
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L134
262: /// Gas cost increases in future hard forks might break this function. As an alternative, EIP-2930-type transactions using access lists can be employed.
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L262
268: /// @param _input An alias being equivalent to `msg.data`. This feature of the fallback function was introduced with the [solidity compiler version 0.7.6](https://github.com/ethereum/solidity/releases/tag/v0.7.6)
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L268
338: /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)).
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L338
19: /// @notice Checks if an address has permission on a contract via a permission identifier and considers if `ANY_ADDRESS` was used in the granting process.
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/IDAO.sol#L19
40: /// @notice Executes a list of actions. If no failure map is provided, one failing action results in the entire excution to be reverted. If a non-zero failure map is provided, allowed actions can fail without the remaining actions being reverted.
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/IDAO.sol#L40
41: /// @param _callId The ID of the call. The definition of the value of `callId` is up to the calling contract and can be used, e.g., as a nonce.
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/IDAO.sol#L41
43: /// @param _allowFailureMap A bitmap allowing execution to succeed, even if individual actions might revert. If the bit at index `i` is 1, the execution succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert.
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/IDAO.sol#L43
58: /// @dev The value of `callId` is defined by the component/contract calling the execute function. A `Plugin` implementation can use it, for example, as a nonce.
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/IDAO.sol#L58
96: /// @dev This event is intended to be emitted in the `receive` function and is therefore bound by the gas limitations for `send`/`transfer` calls introduced by [ERC-2929](https://eips.ethereum.org/EIPS/eip-2929).
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/IDAO.sol#L96
121: /// @notice Checks whether a signature is valid for the provided hash by forwarding the call to the set [ERC-1271](https://eips.ethereum.org/EIPS/eip-1271) signature validator contract.
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/IDAO.sol#L121
127: /// @notice Registers an ERC standard having a callback by registering its [ERC-165](https://eips.ethereum.org/EIPS/eip-165) interface ID and callback function signature.
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/IDAO.sol#L127
9: /// @notice A distinct Uniform Resource Identifier (URI) pointing to a JSON object following the "EIP-4824 DAO JSON-LD Schema". This JSON file splits into four URIs: membersURI, proposalsURI, activityLogURI, and governanceURI. The membersURI should point to a JSON file that conforms to the "EIP-4824 Members JSON-LD Schema". The proposalsURI should point to a JSON file that conforms to the "EIP-4824 Proposals JSON-LD Schema". The activityLogURI should point to a JSON file that conforms to the "EIP-4824 Activity Log JSON-LD Schema". The governanceURI should point to a flatfile, normatively a .md file. Each of the JSON files named above can be statically-hosted or dynamically-generated.
7: /// @notice This interface can be implemented to support more customary permissions depending on on- or off-chain state, e.g., by querying token ownershop or a secondary condition, respectively.
36: /// @param condition The `PermissionCondition` that will be asked for authorization on calls connected to the specified permission identifier.
26: /// @notice A mapping storing permissions as hashes (i.e., `permissionHash(where, who, permissionId)`) and their status encoded by an address (unset, allowed, or redirecting to a `PermissionCondition`).
85: /// @notice A modifier to make functions on inheriting contracts authorized. Permissions to call the function are checked through this permission manager.
99: /// @notice Grants permission to an address to call methods in a contract guarded by an auth modifier with the specified permission identifier.
104: /// @dev Note, that granting permissions with `_who` or `_where` equal to `ANY_ADDR` does not replace other permissions with specific `_who` and `_where` addresses that exist in parallel.
119: /// @dev Note, that granting permissions with `_who` or `_where` equal to `ANY_ADDR` does not replace other permissions with specific `_who` and `_where` addresses that exist in parallel.
229: /// @dev Note, that granting permissions with `_who` or `_where` equal to `ANY_ADDR` does not replace other permissions with specific `_who` and `_where` addresses that exist in parallel.
113: /// @notice Grants permission to an address to call methods in a target contract guarded by an auth modifier with the specified permission identifier if the referenced condition permits it.
118: /// @param _condition The `PermissionCondition` that will be asked for authorization on calls connected to the specified permission identifier.
129: /// @notice Revokes permission from an address to call methods in a target contract guarded by an auth modifier with the specified permission identifier.
134: /// @dev Note, that revoking permissions with `_who` or `_where` equal to `ANY_ADDR` does not revoke other permissions with specific `_who` and `_where` addresses that exist in parallel.
192: /// @notice Checks if an address has permission on a contract via a permission identifier and considers if `ANY_ADDRESS` was used in the granting process.
228: /// @param _condition An address either resolving to a `PermissionCondition` contract address or being the `ALLOW_FLAG` address (`address(2)`).
281: /// @dev Note, that revoking permissions with `_who` or `_where` equal to `ANY_ADDR` does not revoke other permissions with specific `_who` and `_where` addresses that might have been granted in parallel.
291: /// @notice Checks if a caller is granted permissions on a target contract via a permission identifier and redirects the approval to a `PermissionCondition` if this was specified in the setup.
337: /// @notice Generates the hash for the `permissionsHashed` mapping obtained from the word "PERMISSION", the contract address, the address owning the permission, and the permission identifier.
353: /// @dev By default, every permission is unrestricted and it is the derived contract's responsibility to override it. Note, that the `ROOT_PERMISSION_ID` is included not required to be set it again.
361: /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)).
13: /// @notice An abstract, non-upgradeable contract to inherit from when creating a plugin being deployed via the minimal clones pattern (see [ERC-1167](https://eips.ethereum.org/EIPS/eip-1167)).
15: /// @notice An abstract, upgradeable contract to inherit from when creating a plugin being deployed via the UUPS pattern (see [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822)).
53: /// @notice Returns the address of the implementation contract in the [proxy storage slot](https://eips.ethereum.org/EIPS/eip-1967) slot the [UUPS proxy](https://eips.ethereum.org/EIPS/eip-1822) is pointing to.
59: /// @notice Internal method authorizing the upgrade of the contract via the [upgradeabilty mechanism for UUPS proxies](https://docs.openzeppelin.com/contracts/4.x/api/proxy#UUPSUpgradeable) (see [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822)).
65: /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)).
12: /// @notice An abstract contract providing a meta-transaction compatible modifier for non-upgradeable contracts instantiated via the `new` keyword to authorize function calls through an associated DAO.
29: /// @notice A modifier to make functions on inheriting contracts authorized. Permissions to call the function are checked through the associated DAO's permission manager.
12: /// @notice An abstract contract providing a meta-transaction compatible modifier for upgradeable or cloneable contracts to authorize function calls through an associated DAO.
30: /// @notice A modifier to make functions on inheriting contracts authorized. Permissions to call the function are checked through the associated DAO's permission manager.
37: /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)).
18: /// @param allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert.
12: /// @notice An abstract contract containing the traits and internal functionality to create and execute proposals that can be inherited by non-upgradeable DAO plugins.
42: /// @param _allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert.
69: /// @param _allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert.
12: /// @notice An abstract contract containing the traits and internal functionality to create and execute proposals that can be inherited by upgradeable DAO plugins.
42: /// @param _allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert.
69: /// @param _allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert.
82: /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)).
14: /// @notice A free function checking if a caller is granted permissions on a target contract via a permission identifier that redirects the approval to a `PermissionCondition` if this was specified in the setup.
7: /// @notice This contract handles callbacks by registering a magic number together with the callback function's selector. It provides the `_handleCallback` function that inherting have to call inside their `fallback()` function (`_handleCallback(msg.callbackSelector, msg.data)`). This allows to adaptively register ERC standards (e.g., [ERC-721](https://eips.ethereum.org/EIPS/eip-721), [ERC-1115](https://eips.ethereum.org/EIPS/eip-1155), or future versions of [ERC-165](https://eips.ethereum.org/EIPS/eip-165)) and returning the required magic numbers for the associated callback functions for the inheriting contract so that it doesn't need to be upgraded.
28: /// @dev This function is supposed to be called via `_handleCallback(msg.sig, msg.data)` in the `fallback()` function of the inheriting contract.
52: /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)).
41: /// @param data The bytes-encoded data containing the input parameters for the installation as specified in the plugin's build metadata JSON file.
72: /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)).
14: /// @notice Creates a new plugin version as the latest build for an existing release number or the first build for a new release number for the provided `PluginSetup` contract address and metadata.
255: /// @notice Internal method authorizing the upgrade of the contract via the [upgradeabilty mechanism for UUPS proxies](https://docs.openzeppelin.com/contracts/4.x/api/proxy#UUPSUpgradeable) (see [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822)).
41: /// @param _maintainer The maintainer of the plugin repo. This address has permission to update metadata, upgrade the repo logic, and manage the repo permissions.
44: /// @dev After the creation of the `PluginRepo` and release of the first version by the factory, ownership is transferred to the `_maintainer` address.
61: /// @notice Set the final permissions for the published plugin repository maintainer. All permissions are revoked from the plugin factory and granted to the specified plugin maintainer.
111: /// @notice Internal method creating a `PluginRepo` via the [ERC-1967](https://eips.ethereum.org/EIPS/eip-1967) proxy pattern from the provided base contract and registering it in the Aragon plugin registry.
71: /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)).
10: /// @notice The interface required for a plugin setup contract to be consumed by the `PluginSetupProcessor` for plugin installations, updates, and uninstallations.
14: /// @param permissions The array of multi-targeted permission operations to be applied by the `PluginSetupProcessor` to the installing or updating DAO.
20: /// @notice The payload for plugin updates and uninstallations containing the existing contracts as well as optional data to be consumed by the plugin setup.
23: /// @param data The bytes-encoded data containing the input parameters for the preparation of update/uninstall as specified in the corresponding ABI on the version's metadata.
32: /// @param _data The bytes-encoded data containing the input parameters for the installation as specified in the plugin's build metadata JSON file.
55: /// @return permissions The array of multi-targeted permission operations to be applied by the `PluginSetupProcessor` to the uninstalling DAO.
63: /// @dev The implementation can be instantiated via the `new` keyword, cloned via the minimal clones pattern (see [ERC-1167](https://eips.ethereum.org/EIPS/eip-1167)), or proxied via the UUPS pattern (see [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822)).
29: /// @notice A convenience function to create an [ERC-1967](https://eips.ethereum.org/EIPS/eip-1967) proxy contract pointing to an implementation and being associated to a DAO.
21: /// @notice This contract processes the preparation and application of plugin setups (installation, update, uninstallation) on behalf of a requesting DAO.
22: /// @dev This contract is temporarily granted the `ROOT_PERMISSION_ID` permission on the applying DAO and therefore is highly security critical.
37: /// @notice The hash obtained from the bytes-encoded empty array to be used for UI updates being required to submit an empty permission array.
49: /// @param currentAppliedSetupId The current setup id that plugin holds. Needed to confirm that `prepareUpdate` or `prepareUninstallation` happens for the plugin's current/valid dependencies.
50: /// @param preparedSetupIdToBlockNumber The mapping between prepared setup IDs and block numbers at which `prepareInstallation`, `prepareUpdate` or `prepareUninstallation` was executed.
63: /// @param data The bytes-encoded data containing the input parameters for the installation preparation as specified in the corresponding ABI on the version's metadata.
85: /// @param setupPayload The payload containing the plugin and helper contract addresses deployed in a preparation step as well as optional data to be consumed by the plugin setup.
110: /// @param setupPayload The payload containing the plugin and helper contract addresses deployed in a preparation step as well as optional data to be consumed by the plugin setup.
213: /// @param setupPayload The payload containing the plugin and helper contract addresses deployed in a preparation step as well as optional data to be consumed by the plugin setup.
245: /// @param setupPayload The payload containing the plugin and helper contract addresses deployed in a preparation step as well as optional data to be consumed by the plugin setup.
86: /// This includes the bytes-encoded data containing the input parameters for the update preparation as specified in the corresponding ABI on the version's metadata.
111: /// This includes the bytes-encoded data containing the input parameters for the uninstallation preparation as specified in the corresponding ABI on the version's metadata.
134: /// @dev This is thrown if the `APPLY_INSTALLATION_PERMISSION_ID`, `APPLY_UPDATE_PERMISSION_ID`, or APPLY_UNINSTALLATION_PERMISSION_ID is missing.
158: /// @notice Thrown if a prepared setup ID is not eligible to be applied. This can happen if another setup has been already applied or if the setup wasn't prepared in the first place.
181: /// @param data The bytes-encoded data containing the input parameters for the preparation as specified in the corresponding ABI on the version's metadata.
395: /// @dev The list of `_params.setupPayload.currentHelpers` has to be specified in the same order as they were returned from previous setups preparation steps (the latest `prepareInstallation` or `prepareUpdate` step that has happend) on which the update is prepared for.
551: /// @dev The list of `_params.setupPayload.currentHelpers` has to be specified in the same order as they were returned from previous setups preparation steps (the latest `prepareInstallation` or `prepareUpdate` step that has happend) on which the uninstallation was prepared for.
611: /// @dev The list of `_params.setupPayload.currentHelpers` has to be specified in the same order as they were returned from previous setups preparation steps (the latest `prepareInstallation` or `prepareUpdate` step that has happend) on which the uninstallation was prepared for.
649: /// @dev If the block number stored in `states[pluginInstallationId].blockNumber` exceeds the one stored in `pluginState.preparedSetupIdToBlockNumber[preparedSetupId]`, the prepared setup with `preparedSetupId` is outdated and not applicable anymore.
41: /// @param _preparationType The type of preparation the plugin is currently undergoing. Without this, it is possible to call `applyUpdate` even after `applyInstallation` is called.
52: /// @notice Internal method authorizing the upgrade of the contract via the [upgradeabilty mechanism for UUPS proxies](https://docs.openzeppelin.com/contracts/4.x/api/proxy#UUPSUpgradeable) (see [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822)).
74: /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)).
9: /// @dev This function allows empty (zero-length) subdomains. If this should not be allowed, make sure to add a respective check when using this function in your code.
72: /// @notice Creates a new `GovernanceERC20` token or a `GovernanceWrappedERC20` from an existing [ERC-20](https://eips.ethereum.org/EIPS/eip-20) token depending on the address used in the `TokenConfig` provided.
74: /// @param _tokenConfig The token configuration struct containing the name, and symbol of the token to be create, but also an address. For `address(0)`, a new governance token is created. For any other address pointing to an [ERC-20](https://eips.ethereum.org/EIPS/eip-20)-compatible contract, a wrapped governance token is created.
77: /// @return The created `MerkleMinter` contract used to mint the `ERC20VotesUpgradeable` tokens or `address(0)` if an existing token was provided.
15: /// @notice This contract registers ENS subdomains under a parent domain specified in the initialization process and maintains ownership of the subdomain since only the resolver address is set. This contract must either be the domain node owner or an approved operator of the node owner. The default resolver being used is the one specified in the parent domain.
72: /// @notice Internal method authorizing the upgrade of the contract via the [upgradeabilty mechanism for UUPS proxies](https://docs.openzeppelin.com/contracts/4.x/api/proxy#UUPSUpgradeable) (see [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822)).
110: /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)).
49: /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)).
31: /// @dev This only gets called for daos that install it for the first time. The initializer modifier protects it from being called a second time for old proxies.
51: /// @dev This gets called when a dao already has `CounterV1` installed and updates to this verison `CounterV2`. For these DAOs, this `setNewVariable` can only be called once which is achieved by `reinitializer(2)`.
68: /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)).
61: /// @param _allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert.
75: /// @dev Currently, there is no reliable way to revoke the `ADMIN_EXECUTE_PERMISSION_ID` from all addresses it has been granted to. Accordingly, only the `EXECUTE_PERMISSION_ID` is revoked for this uninstallation.
13: /// @param None The default option state of a voter indicating the absence of from the vote. This option neither influences support nor participation.
44: /// @notice Checks if the support value defined as $$/texttt{support} = /frac{N_/text{yes}}{N_/text{yes}+N_/text{no}}$$ for a proposal vote is greater than the support threshold.
49: /// @notice Checks if the worst-case support value defined as $$/texttt{worstCaseSupport} = /frac{N_/text{yes}}{ N_/text{total}-N_/text{abstain}}$$ for a proposal vote is greater than the support threshold.
54: /// @notice Checks if the participation value defined as $$/texttt{participation} = /frac{N_/text{yes}+N_/text{no}+N_/text{abstain}}{N_/text{total}}$$ for a proposal vote is greater or equal than the minimum participation value.
84: /// @param _tryEarlyExecution If `true`, early execution is tried after the vote cast. The call does not revert if early execution is not possible.
27: /// where $N_/text{yes}$, $N_/text{no}$, and $N_/text{abstain}$ are the yes, no, and abstain votes that have been cast and $N_/text{total}$ is the total voting power available at /// proposal creation time.
31: /// Two limit values are associated with these parameters and decide if a proposal execution should be possible: $/texttt{supportThreshold} /in [0,1]$ and $/texttt{minParticipation} /in /// [0,1]$.
33: /// For threshold values, $>$ comparison is used. This **does not** include the threshold value. E.g., for $/texttt{supportThreshold} = 50/%$, the criterion is fulfilled if there is at /// least one more yes than no votes ($N_/text{yes} = N_/text{no} + 1 $).
34: /// For minimum values, $/ge$ comparison is used. This **does** include the minimum participation value. E.g., for $/texttt{minParticipation} = 40/%$ and $N_/text{total} = 10$, the /// criterion is fulfilled if 4 out of 10 votes were casted.
38: /// However, this is not enforced by the contract code and developers can make unsafe parameters and only the frontend will warn about bad parameter settings.
62: /// This contract allows a proposal to be executed early, iff the vote outcome cannot change anymore by more people voting. Accordingly, vote replacement and early execution are /// mutually exclusive options.
63: /// The outcome cannot change anymore iff the support threshold is met even if all remaining votes are no votes. We call this number the worst-case number of no votes and define it as
108: /// @param EarlyExecution In early execution mode, a proposal can be executed early before the end date if the vote outcome cannot mathematically change by more voters voting.
136: /// @param allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert.
428: /// @param _allowFailureMap Allows proposal to succeed even if an action reverts. Uses bitmap representation. If the bit at index `x` is 1, the tx succeeds even if the action at `x` failed. Passing 0 will be treated as atomic execution.
432: /// @param _tryEarlyExecution If `true`, early execution is tried after the vote cast. The call does not revert if early execution is not possible.
447: /// @param _tryEarlyExecution If `true`, early execution is tried after the vote cast. The call does not revert if early execution is not possible.
527: // Require the support threshold value to be in the interval [0, 10^6-1], because `>` comparision is used in the support criterion and >100% could never be reached.
580: // Since `minDuration` is limited to 1 year, `startDate + minDuration` can only overflow if the `startDate` is after `type(uint64).max - minDuration`. In this case, the proposal creation will revert and another date can be picked.
593: /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)).
16: /// @notice The majority voting implementation using an [OpenZepplin `Votes`](https://docs.openzeppelin.com/contracts/4.x/api/governance#Votes) compatible governance token.
25: /// @notice An [OpenZepplin `Votes`](https://docs.openzeppelin.com/contracts/4.x/api/governance#Votes) compatible contract referencing the token being used for voting.
39: /// @param addr The token address. If this is `address(0)`, a new `GovernanceERC20` token is deployed. If not, the existing token is wrapped as an `GovernanceWrappedERC20`.
11: /// @notice Adds new members to the address list. Previously, it checks if the new addresslist length would be greater than `type(uint16).max`, the maximal number of approvals.
15: /// @notice Removes existing members from the address list. Previously, it checks if the new addresslist length is at least as long as the minimum approvals parameter requires. Note that `minApprovals` is must be at least 1 so the address list cannot become empty.
33: /// @param _allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert.
201: /// @param _allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert.
304: /// @param allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert.
128: /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)).
19: /// @notice A component minting [ERC-20](https://eips.ethereum.org/EIPS/eip-20) tokens and distributing them on merkle trees using `MerkleDistributor` clones.
97: /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZepplins guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)).
22: /// @notice Thrown when the address list update is invalid, which can be caused by the addition of an existing member or removal of a non-existing member.
19: /// @notice An [OpenZepplin `Votes`](https://docs.openzeppelin.com/contracts/4.x/api/governance#Votes) compatible [ERC-20](https://eips.ethereum.org/EIPS/eip-20) token that can be used for voting and is managed by a DAO.
21: /// @notice Wraps an existing [ERC-20](https://eips.ethereum.org/EIPS/eip-20) token by inheriting from `ERC20WrapperUpgradeable` and allows to use it for voting by inheriting from `ERC20VotesUpgradeable`. The latter is compatible with [OpenZepplin `Votes`](https://docs.openzeppelin.com/contracts/4.x/api/governance#Votes) interface.
24: /// 2. call `depositFor` to wrap them, which safely transfers the underlying [ERC-20](https://eips.ethereum.org/EIPS/eip-20) tokens to the contract and mints wrapped [ERC-20](https://eips.ethereum.org/EIPS/eip-20) tokens.
25: /// To get the [ERC-20](https://eips.ethereum.org/EIPS/eip-20) tokens back, the owner of the wrapped tokens can call `withdrawFor`, which burns the wrapped tokens [ERC-20](https://eips.ethereum.org/EIPS/eip-20) tokens and safely transfers the underlying tokens back to the owner.
26: /// @dev This contract intentionally has no public mint functionality because this is the responsibility of the underlying [ERC-20](https://eips.ethereum.org/EIPS/eip-20) token contract.
7: /// @notice Free function to create a [ERC-1967](https://eips.ethereum.org/EIPS/eip-1967) proxy contract based on the passed base contract address.
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/utils/Proxy.sol#L7
11: /// @dev Initializes the upgradeable proxy with an initial implementation specified by _logic. If _data is non-empty, it’s used as data in a delegate call to _logic. This will typically be an encoded function call, and allows initializing the storage of the proxy like a Solidity constructor (see [OpenZepplin ERC1967Proxy-constructor](https://docs.openzeppelin.com/contracts/4.x/api/proxy#ERC1967Proxy-constructor-address-bytes-)).
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/utils/Proxy.sol#L11
When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.
144: function setupBases() private {
100: function setDefaultResolver(
52: function setNewVariable(uint256 _newVariable) external reinitializer(2) {
53: function changeDistributorBase(
OpenZeppelin recommends that the initializer modifier be applied to constructors. Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits. https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/5
92: constructor()
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L92
17: constructor(IDAO _dao) DaoAuthorizable(_dao)
16: constructor()
25: constructor()
19: constructor(IDAO _dao)
53: constructor(DAORegistry _registry, PluginSetupProcessor _pluginSetupProcessor)
30: constructor()
112: constructor()
22: constructor(PluginRepoRegistry _pluginRepoRegistry)
34: constructor()
277: constructor(PluginRepoRegistry _repoRegistry)
68: constructor()
45: constructor()
23: constructor()
24: constructor(MultiplyHelper _helper)
27: constructor()
20: constructor()
61: constructor()
19: constructor()
49: constructor( IDAO _dao, string memory _name, string memory _symbol, MintSettings memory _mintSettings )
38: constructor(IERC20Upgradeable _token, string memory _name, string memory _symbol)
The current form of relative path import is not recommended for use because it can unpredictably pollute the namespace. Instead, the Solidity docs recommend specifying imported symbols explicitly. https://docs.soliditylang.org/en/v0.8.15/layout-of-source-files.html#importing-other-source-files
A good example:
import {OwnableUpgradeable} from "openzeppelin-contracts-upgradeable/contracts/access/OwnableUpgradeable.sol"; import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol"; import {SafeCastLib} from "solmate/utils/SafeCastLib.sol"; import {ERC20} from "solmate/tokens/ERC20.sol"; import {IProducer} from "src/interfaces/IProducer.sol"; import {GlobalState, UserState} from "src/Common.sol";
7: import "./IPermissionCondition.sol"; 8: import "./PermissionLib.sol";
8: import "./IProposal.sol";
8: import "./IProposal.sol";
Use specific imports syntax per solidity docs recommendation.
<a href="https://blog.soliditylang.org/2023/02/22/solidity-0.8.19-release-announcement/">0.8.19</a>: SMTChecker: New trusted mode that assumes that any compile-time available code is the actual used code, even in external calls. Bug Fixes:
pragma solidity 0.8.17;
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L3
pragma solidity 0.8.17;
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/IDAO.sol#L3
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/utils/auth.sol#L3
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/utils/Proxy.sol#L3
pragma solidity 0.8.17;
Consider updating to a more recent solidity version.
Throughout the codebase, events are generally emitted when sensitive changes are made to the contracts. However, some events are missing important parameters The following events should also add the previous original value in addition to the new value.
89: event NewURI(string daoURI);
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L89
The events should include the new value and old value where possible.
136: function _authorizeUpgrade(address) internal virtual override auth(UPGRADE_DAO_PERMISSION_ID) {}
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L136
320: } catch {}
17: constructor(IDAO _dao) DaoAuthorizable(_dao) {}
63: ) internal virtual override auth(UPGRADE_PLUGIN_PERMISSION_ID) {}
259: ) internal virtual override auth(UPGRADE_REPO_PERMISSION_ID) {}
27: {}
672: {} catch Error(string memory reason) { 682: try PluginUUPSUpgradeable(_proxy).upgradeTo(_implementation) {} catch Error(
56: ) internal virtual override auth(UPGRADE_REGISTRY_PERMISSION_ID) {}
10: contract Dummy {}
76: ) internal virtual override auth(UPGRADE_REGISTRAR_PERMISSION_ID) {}
bytes.concat()Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>))
347: return keccak256(abi.encodePacked("PERMISSION", _who, _where, _permissionId))
252: return keccak256(abi.encodePacked(_tag.release, _tag.build))
86: bytes32 subnode = keccak256(abi.encodePacked(node, _label))
105: bytes32 node = keccak256(abi.encodePacked(_index, _to, _amount))
Use bytes.concat() and upgrade to at least Solidity version 0.8.4 if required.
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.
136: function _authorizeUpgrade(address) internal virtual override auth(UPGRADE_DAO_PERMISSION_ID) {}
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L136
61: function _authorizeUpgrade(
257: function _authorizeUpgrade(
54: function _authorizeUpgrade(
74: function _authorizeUpgrade(
I suggest implementing some kind of version counter that will be incremented automatically when you upgrade the contract.
For example
uint256 public authorizeUpgradeCounter; function _authorizeUpgrade(address newImplementation) internal { authorizeUpgradeCounter+=1; }
#0 - c4-judge
2023-03-12T16:40:22Z
0xean marked the issue as grade-a
#1 - novaknole20
2023-03-22T09:52:05Z
L-3 Ceiling implies that we remove decimal places and round. Invalid
L-4 Is avoided in lower level of the contracts. Invalid
L-5 We want to allow to interact with EOA addresses. Invalid
#2 - c4-sponsor
2023-03-22T09:52:09Z
novaknole20 marked the issue as sponsor acknowledged
🌟 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 | Contexts | Estimated Gas Saved | |
|---|---|---|---|
| GAS‑1 | abi.encode() is less efficient than abi.encodepacked() | 6 | 600 |
| GAS‑2 | Setting the constructor to payable | 21 | 273 |
| GAS‑3 | Do not calculate constants | 1 | - |
| GAS‑4 | Don't use _msgSender() if not supporting EIP-2771 | 12 | - |
| GAS‑5 | Empty Blocks Should Be Removed Or Emit Something | 9 | - |
| GAS‑6 | Use assembly to write address storage values | 1 | - |
| GAS‑7 | internal functions only called once can be inlined to save gas | 4 | 88 |
| GAS‑8 | Optimize names to save gas | 33 | 726 |
| GAS‑9 | Public Functions To External | 64 | - |
| GAS‑10 | Save gas with the use of specific import statements | 4 | - |
| GAS‑11 | Shorten the array rather than copying to a new one | 5 | - |
| GAS‑12 | Help The Optimizer By Saving A Storage Variable’s Reference Instead Of Repeatedly Fetching It | 3 | - |
| GAS‑13 | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead | 35 | - |
| GAS‑14 | ++i/i++ Should Be unchecked{++i}/unchecked{i++} When It Is Not Possible For Them To Overflow, As Is The Case When Used In For- And While-loops | 2 | 70 |
| GAS‑15 | Use solidity version 0.8.19 to gain some gas boost | 62 | 5456 |
| GAS‑16 | Using storage instead of memory saves gas | 2 | 700 |
Total: 255 contexts over 16 issues
abi.encode() is less efficient than abi.encodepacked()See for more information: https://github.com/ConnorBlockchain/Solidity-Encode-Gas-Comparison
272: return abi.encode(magicNumber);
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L272
33: return keccak256(abi.encode(_dao, _plugin));
33: abi.encode(
73: abi.encode(_pluginSetupRef.versionTag, _pluginSetupRef.pluginSetupRepo, _helpersHash)
80: return keccak256(abi.encode(_helpers));
89: return keccak256(abi.encode(_permissions));
constructor to payableSaves ~13 gas per instance
92: constructor()
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L92
17: constructor(IDAO _dao) DaoAuthorizable(_dao)
16: constructor()
25: constructor()
19: constructor(IDAO _dao)
53: constructor(DAORegistry _registry, PluginSetupProcessor _pluginSetupProcessor)
30: constructor()
112: constructor()
22: constructor(PluginRepoRegistry _pluginRepoRegistry)
34: constructor()
277: constructor(PluginRepoRegistry _repoRegistry)
68: constructor()
45: constructor()
23: constructor()
24: constructor(MultiplyHelper _helper)
27: constructor()
20: constructor()
61: constructor()
19: constructor()
49: constructor( IDAO _dao, string memory _name, string memory _symbol, MintSettings memory _mintSettings )
38: constructor(IERC20Upgradeable _token, string memory _name, string memory _symbol)
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.
6: uint256 constant RATIO_BASE = 10 ** 6;
_msgSender() if not supporting EIP-2771Use msg.sender if the code does not implement <a href="https://eips.ethereum.org/EIPS/eip-2771">EIP-2771 trusted forwarder</a> support
70: _creator: _msgSender(),
270: address account = _msgSender();
97: if (minProposerVotingPower() != 0 && !isListedAtBlock(_msgSender(), snapshotBlock)) { 98: revert ProposalCreationForbidden(_msgSender()); 102: _creator: _msgSender(),
91: if (votingToken.getPastVotes(_msgSender(), snapshotBlock) < minProposerVotingPower()) { 92: revert ProposalCreationForbidden(_msgSender()); 96: _creator: _msgSender(),
216: if (multisigSettings.onlyListed && !isListedAtBlock(_msgSender(), snapshotBlock)) { 217: revert ProposalCreationForbidden(_msgSender()); 231: _creator: _msgSender(),
266: address approver = _msgSender();
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{...}})
136: function _authorizeUpgrade(address) internal virtual override auth(UPGRADE_DAO_PERMISSION_ID) {}
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L136
320: } catch {}
63: ) internal virtual override auth(UPGRADE_PLUGIN_PERMISSION_ID) {}
259: ) internal virtual override auth(UPGRADE_REPO_PERMISSION_ID) {}
27: {}
672: {} catch Error(string memory reason) {
682: try PluginUUPSUpgradeable(_proxy).upgradeTo(_implementation) {} catch Error(
56: ) internal virtual override auth(UPGRADE_REGISTRY_PERMISSION_ID) {}
76: ) internal virtual override auth(UPGRADE_REGISTRAR_PERMISSION_ID) {}
assembly to write address storage values333: _daoURI = daoURI_;
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L333
internal functions only called once can be inlined to save gas122: function isPermissionRestrictedForAnyAddr
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L122
212: function _initializePermissionManager
354: function isPermissionRestrictedForAnyAddr
141: function _createDAO
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 more <a href="https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92">here</a>
All in-scope contracts
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 Gauge.sol contract will be the most used; A lower method ID may be given.
The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.
function isGranted( address _where, address _who, bytes32 _permissionId, bytes memory _data ) public view virtual returns (bool) {
function pluginType() public pure override returns (PluginType) {
function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) {
function pluginType() public pure override returns (PluginType) {
function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) {
function pluginType() public pure override returns (PluginType) {
function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) {
function implementation() public view returns (address) {
function dao() public view returns (IDAO) {
function dao() public view returns (IDAO) {
function proposalCount() public view override returns (uint256) {
function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) {
function proposalCount() public view override returns (uint256) {
function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) {
function getLatestVersion(uint8 _release) public view returns (Version memory) {
function getLatestVersion(address _pluginSetup) public view returns (Version memory) {
function getVersion(Tag calldata _tag) public view returns (Version memory) {
function getVersion(bytes32 _tagHash) public view returns (Version memory) {
function buildCount(uint8 _release) public view returns (uint256) {
function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) {
function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) {
function validatePreparedSetupId( bytes32 pluginInstallationId, bytes32 preparedSetupId ) public view {
function multiply(uint256 _a) public view auth(MULTIPLY_PERMISSION_ID) returns (uint256) {
function execute() public {
function multiply(uint256 _a) public view auth(MULTIPLY_PERMISSION_ID) returns (uint256) {
function execute() public {
function supportsInterface( bytes4 _interfaceId ) public view override(PluginCloneable, ProposalUpgradeable) returns (bool) {
function vote( uint256 _proposalId, VoteOption _voteOption, bool _tryEarlyExecution ) public virtual {
function execute(uint256 _proposalId) public virtual {
function getVoteOption( uint256 _proposalId, address _voter ) public view virtual returns (VoteOption) {
function canVote( uint256 _proposalId, address _voter, VoteOption _voteOption ) public view virtual returns (bool) {
function canExecute(uint256 _proposalId) public view virtual returns (bool) {
function isSupportThresholdReached(uint256 _proposalId) public view virtual returns (bool) {
function isSupportThresholdReachedEarly( uint256 _proposalId ) public view virtual returns (bool) {
function isMinParticipationReached(uint256 _proposalId) public view virtual returns (bool) {
function supportThreshold() public view virtual returns (uint32) {
function minParticipation() public view virtual returns (uint32) {
function minDuration() public view virtual returns (uint64) {
function minProposerVotingPower() public view virtual returns (uint256) {
function votingMode() public view virtual returns (VotingMode) {
function totalVotingPower(uint256 _blockNumber) public view virtual returns (uint256);
function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) {
function totalVotingPower(uint256 _blockNumber) public view override returns (uint256) {
function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) {
function getVotingToken() public view returns (IVotesUpgradeable) {
function totalVotingPower(uint256 _blockNumber) public view override returns (uint256) {
function supportsInterface( bytes4 _interfaceId ) public view virtual override(PluginUUPSUpgradeable, ProposalUpgradeable) returns (bool) {
function approve(uint256 _proposalId, bool _tryExecution) public {
function hasApproved(uint256 _proposalId, address _account) public view returns (bool) {
function execute(uint256 _proposalId) public {
function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) {
function unclaimedBalance( uint256 _index, address _to, uint256 _amount, bytes32[] memory _proof ) public view override returns (uint256) {
function isClaimed(uint256 _index) public view override returns (bool) {
function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) {
function isListedAtBlock( address _account, uint256 _blockNumber ) public view virtual returns (bool) {
function isListed(address _account) public view virtual returns (bool) {
function addresslistLengthAtBlock(uint256 _blockNumber) public view virtual returns (uint256) {
function addresslistLength() public view virtual returns (uint256) {
function initialize( IDAO _dao, string memory _name, string memory _symbol, MintSettings memory _mintSettings ) public initializer {
function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) {
function initialize( IERC20Upgradeable _token, string memory _name, string memory _symbol ) public initializer {
function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) {
function depositFor( address account, uint256 amount ) public override(IGovernanceWrappedERC20, ERC20WrapperUpgradeable) returns (bool) {
function withdrawTo( address account, uint256 amount ) public override(IGovernanceWrappedERC20, ERC20WrapperUpgradeable) returns (bool) {
The current form of relative path import is not recommended for use because it can unpredictably pollute the namespace. Instead, the Solidity docs recommend specifying imported symbols explicitly. https://docs.soliditylang.org/en/v0.8.15/layout-of-source-files.html#importing-other-source-files
A good example:
import {OwnableUpgradeable} from "openzeppelin-contracts-upgradeable/contracts/access/OwnableUpgradeable.sol"; import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol"; import {SafeCastLib} from "solmate/utils/SafeCastLib.sol"; import {ERC20} from "solmate/tokens/ERC20.sol"; import {IProducer} from "src/interfaces/IProducer.sol"; import {GlobalState, UserState} from "src/Common.sol";
7: import "./IPermissionCondition.sol"; 8: import "./PermissionLib.sol";
8: import "./IProposal.sol";
8: import "./IProposal.sol";
Use specific imports syntax per solidity docs recommendation.
Inline-assembly can be used to shorten the array by changing the length slot, so that the entries don't have to be copied to a new, shorter array
58: address[] memory helpers = new address[](1);
59: address[] memory helpers = new address[](1);
132: address[] memory activeHelpers = new address[](1);
96: address[] memory helpers = new address[](1);
266: bytes4[] memory interfaceIds = new bytes4[](3);
To help the optimizer, declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array. The effect can be quite significant. As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex] and use it.
111: Proposal storage proposal_ = proposals[proposalId];
105: Proposal storage proposal_ = proposals[proposalId];
240: Proposal storage proposal_ = proposals[proposalId];
uints/ints smaller than 32 bytes (256 bits) incurs overheadWhen 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.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html
Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed
34: uint8 release;
35: uint16 build;
65: uint8 public latestRelease;
165: uint16 build = ++buildsPerRelease[_release];
210: uint16 latestBuild = uint16(buildsPerRelease[_release]);
17: uint8 char = uint8(nameBytes[i]);
67: uint64 currentTimestamp64 = block.timestamp.toUint64();
155: uint32 supportThreshold;
125: uint32 minParticipation;
126: uint64 minDuration;
156: uint64 startDate;
157: uint64 endDate;
158: uint64 snapshotBlock;
516: uint64 currentTime = block.timestamp.toUint64();
568: uint64 currentTimestamp = block.timestamp.toUint64();
580: uint64 earliestEndDate = startDate + votingSettings.minDuration;
92: uint64 snapshotBlock;
267: interfaceIds[0] = type(IERC20Upgradeable).interfaceId;
268: interfaceIds[1] = type(IVotesUpgradeable).interfaceId;
269: interfaceIds[2] = type(IGovernanceWrappedERC20).interfaceId;
36: uint16 approvals;
60: uint16 minApprovals;
50: uint64 snapshotBlock;
51: uint64 startDate;
52: uint64 endDate;
175: uint16 newAddresslistLength = uint16(addresslistLength() - _members.length);
214: uint64 snapshotBlock = block.number.toUint64() - 1;
404: uint64 currentTimestamp64 = block.timestamp.toUint64();
414: uint16 addresslistLength_ = uint16(addresslistLength());
++i/i++ Should Be unchecked{++i}/unchecked{i++} When It Is Not Possible For Them To Overflow, As Is The Case When Used In For- And While-loopsThe 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
95: for (uint256 i; i < _pluginSettings.length; ++i) {
16: for (uint256 i; i < nameLength; i++) {
Upgrade to the latest solidity version 0.8.19 to get additional gas savings. See latest release for reference: https://blog.soliditylang.org/2023/02/22/solidity-0.8.19-release-announcement/
pragma solidity 0.8.17;
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/DAO.sol#L3
pragma solidity 0.8.17;
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/dao/IDAO.sol#L3
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/core/utils/auth.sol#L3
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
pragma solidity 0.8.17;
https://github.com/code-423n4/2023-03-aragon/tree/main/packages/contracts/src/utils/Proxy.sol#L3
pragma solidity 0.8.17;
storage instead of memory saves gasWhen 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
151: PermissionLib.SingleTargetPermission memory item = items[i];
171: PermissionLib.MultiTargetPermission memory item = _items[i];
#0 - c4-judge
2023-03-12T18:24:05Z
0xean marked the issue as grade-b
#1 - c4-sponsor
2023-03-22T09:52:52Z
novaknole20 marked the issue as sponsor acknowledged