Aragon Protocol contest - descharre's results

Summary

Low Risk

Non critical

Details

Low Risk

[L-01] Missing 0 address checks

The function _setTrustedForwarder() sets the trusted forwarder on the DAO. This is a single step process and has no checks. A 0 address check can be used to prevent that the trusted forwarder can be set to 0. Allowing zero-addresses will lead to contract reverts and force redeployments if there are no setters for such address variables.

More 0 address checks missing:

• DAO.sol#L239-L245
• DaoAuthorizable.sol#L20
• DAOFactory.sol#L54-L55

[L-02] Account existence check for low-level calls

Low-level calls call/delegatecall/staticcall return true even if the account called is non-existent (per EVM design). Account existence must be checked prior to calling if needed. If the to address is 0 by accident, the return value of success will be true.

• DAO.sol#L186

[L-03] Signature validater does not always return 0x1626ba7e

The function isValidSignature() forwards the call to the EIP 1271 signatureValidator. The natspec comment says the function returns the bytes4 magic value 0x1626ba7e if the signature is valid.

However as stated in EIP 1271, the function isValidSignature of signatureValidator can have two different return values. If the signature comes from the owner it will return 0x1626ba7e as stated in the natspec comments. Else the function will return 0xffffffff.

The return value should be checked and revert if value is not equal to 0x1626ba7e;

[L-04] Be consistent with checking the endDate

Multisig and MajorityVotingBase both have the function _isProposalOpen.

The function in MajorityVotingBase will return false if the endDate is equal to the current time. It can only be smaller. It's done better in Multisig. Here it will only return false if the endDate is past the current time. The same should be done in MajorityVotingBase. This way you can vote when the proposal is technically still open.

Multisig:

    return
        !proposal_.executed &&
        proposal_.parameters.startDate <= currentTimestamp64 &&
        proposal_.parameters.endDate >= currentTimestamp64;

MajorityBase:

    return
        proposal_.parameters.startDate <= currentTime &&
        currentTime < proposal_.parameters.endDate &&
        !proposal_.executed;

Non critical

[N-01] Constant values such as a call to keccak256(), should be immutable rather than constant

Constants are supposed to be used for literal values written into the code, and immutable variables should be used for expressions or values calculated in the constructor. While it doesn't save any gas, they should be used in their appropriate context.

• DAO.sol#L40
• DAO.sol#L43
• DAO.sol#L46
• DAO.sol#L49
• DAO.sol#L53
• DAO.sol#L57
• PermissionManager.sol#L15
• PluginUUPSUpgradeable.sol#L35
• DAORegistry.sol#L15
• PluginRepo.sol#L49
• PluginRepo.sol#L52
• PluginRepoRegistry.sol#L16
• PluginSetupProcessor.sol#L27
• PluginSetupProcessor.sol#L31
• PluginSetupProcessor.sol#L34
• InterfaceBasedRegistry.sol#L18
• ENSSubdomainRegistrar.sol#L18
• ENSSubdomainRegistrar.sol#L22
• MultiplyHelper.sol#L12
• CounterV1.sol#L14
• CounterV2.sol#L14
• Admin.sol#L23
• MajorityVotingBase.sol#L183
• AddresslistVoting.sol#L27
• Multisig.sol#L71

[N-02] Event is missing indexed fields

When an event is missing indexed fields it can be hard for off-chain scripts to efficiently filter those events.

• DAO.sol#L77
• IDAO.sol#L99
• IDAO.sol#L71-L75
• PluginRepo.sol#L109
• TokenFactory.sol#L42-L46
• MajorityVotingBase.sol#L226-L232

[N-03] Be consistent with setting events and errors in interfaces

If you specify events in the interface, be consistent with it and do it for every event. In IDAO interface, events are defined. But the DAO contract that implements the interface also has one event. It's better to also put this event in the interface.

[N-04] Timestamps don't need to be bigger than uint48

The maximum value of uint48 is year 8 million 921 thousand 556. Uint40, is probably okay too. It will be year 36,812 when it overflows. For block numbers, uint48 is big enough as well. Even with 10000 blocks per second, it would be enough until year 2861. In this project uint64 is mostly used for timestamps.

[N-05] Don't use access control on view functions

They can't change the state. And all state is public anyway

• MultiplyHelper.sol#L19
• CounterV1.sol#L39
• CounterV2.sol#L58

[N-06] Use scientific notation (1E6) rather than exponentation (10**6)

Scientific notation should be used for better code readability.

• Ratio.sol#L6

[N-07] Forgotton params in natspec comments

• Multisig.sol#L121-L136: _members is not specified in natspec
• Multisig.sol#L198-L262: _startDate and _endDate

Summary

Details

[G-01] Make up to 3 fields in an event indexed

Indexing all fields in an event saves gas. If the event has more than 3 fields, only index 3.

DAO.sol#L89: setDaoURI() gas saved 473

-   event NewURI(string daoURI);
+   event NewURI(string indexed daoURI);

This can be done for every event.

[G-02] Switch if statements in _isGranted()

The function _isGranted() is used in every function with the auth() modifier. The following if statements can be switched so when an authorized caller calls a function, it needs to do one less if statement because it will only check ALLOW_FLAG and return true. Where as previous it first checked the UNSET_FLAG and afterwards ALLOW_FLAG. This saves around 40 gas for every function that has access control (uses the auth modifier) and where the caller is authorized. It only dissadvantages unauthorized callers.

        if (accessFlagOrCondition == UNSET_FLAG) return false;
        if (accessFlagOrCondition == ALLOW_FLAG) return true;

can be changed to:

        if (accessFlagOrCondition == ALLOW_FLAG) return true;
        if (accessFlagOrCondition == UNSET_FLAG) return false;

[G-03] Make own counter instead of using OpenZeppelin Counter

Both Proposal.sol and ProposalUpgradeable.sol use the Counter from OpenZeppelin. It's more gas efficiënt if you create your own counter.

Proposal.sol

L14:
-   using CountersUpgradeable for CountersUpgradeable.Counter;

    /// @notice The incremental ID for proposals and executions.
-   CountersUpgradeable.Counter private proposalCounter;
+   uint256 private counter;

L20:
    function proposalCount() public view override returns (uint256) {
-       return proposalCounter.current();
+       return counter;
    }

L33:
    function _createProposalId() internal returns (uint256 proposalId) {
-       proposalId = proposalCount();
+       proposalId = counter++;
-       proposalCounter.increment();
    }

[G-04] Using double if/require statement instead of && consumes less gas

Saves around 15 gas for require statements and 40 for if statements

-       if (_where == ANY_ADDR && _who == ANY_ADDR) {
+       if (_where == ANY_ADDR) {
+         if (_who == ANY_ADDR) {
            revert AnyAddressDisallowedForWhoAndWhere();
          }    
+       }

• PermissionManager.sol#L236-L238
• PluginSetupProcessor.sol#L700-L710
• RegistryUtils.sol#L20-L27
• AddresslistVoting.sol#L97-L99
• AddresslistVoting.sol#L185-L187
• AddresslistVoting.sol#L214-L219
• TokenVoting.sol#L182-L185
• TokenVoting.sol#L211-L216
• Multisig.sol#L216-L218
• Multisig.sol#L283-L285
• GovernanceERC20.sol#L116-L118
• GovernanceWrappedERC20.sol#L106-L108

[G-05] Remove return values when they are never used

The function execute() has two return variables, execResults and failureMap. Right now the function is only called in Proposal.sol in _executeProposal(). This function saves the return variables but does nothing with them and also returns it.

The _executeProposal() is called in Admin.sol, Multisig.sol and MajorityVotingBase.sol. All those functions only call the _executeProposal() function but don't even save the return variables.

Long story, short. The functions execute() and _executeProposal() don't need to return anything. When you remove the return statement and values, 12805 gas is saved for the execute() function.

The function createProposal() in AddresslistVoting and TokenVoting returns proposalId that is used nowhere else. This return statement can also be removed.

[G-06] Make for loops unchecked

The risk of for loops getting overflowed is extremely low. Because it always increments by 1 and is limited to the arrays length. Even if the arrays are extremely long, it will take a massive amount of time and gas to let the for loop overflow. The longer the array, the more gas you will save.

-       for (uint256 i; i < _pluginSettings.length; ++i) {
+       for (uint256 i; i < _pluginSettings.length;) {
            // Prepare plugin.
            (
                address plugin,
                IPluginSetup.PreparedSetupData memory preparedSetupData
            ) = pluginSetupProcessor.prepareInstallation(
                    address(createdDao),
                    PluginSetupProcessor.PrepareInstallationParams(
                        _pluginSettings[i].pluginSetupRef,
                        _pluginSettings[i].data
                    )
                );

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

• DAOFactory.sol#L95-L117
• RegistryUtils.sol#L16-L36

[G-07] Use msg.sender directly instead of the function _msgSender()

The project use _msgSender() from OpenZeppelin Context contract. This function returns msg.sender so it does exactly the same as msg.sender Saves around 30 gas for every instance

• DaoAuthorizable.sol#L32
• DaoAuthorizableUpgradeable.sol#L33
• Admin.sol#L70
• MajorityVotingBase.sol#L270
• AddresslistVoting.sol#L97
• AddresslistVoting.sol#L102
• AddresslistVoting.sol#L98
• TokenVoting.sol#L91-L92
• TokenVoting.sol#L96
• Multisig.sol#L216-L217
• Multisig.sol#L231
• Multisig.sol#L266

[G-08] Don't save block and transaction properties to a memory variable

Most of the block and transaction properties only use 2 gas. While reading memory uses the MLOAD opcode which is 3 gas. And you also save some gas because you don't have to write it to a variable.

• MajorityVotingBase.sol#L270

[G-09] Save state variables in memory when they are used more than once

Reading from storage costs 2100 gas for the first time and 100 for the second time. Reading from memory only costs 3 gas. When you save the variable to memory first you have the 2100 gas cost. But when a variable is used more than once you save 97 gas for each extra read.

Example:

        return
            (RATIO_BASE - proposal_.parameters.supportThreshold) * proposal_.tally.yes >
            proposal_.parameters.supportThreshold * proposal_.tally.no;

Can be changed to

        uint32 treshold =  proposal_.parameters.supportThreshold;
        return
            (RATIO_BASE - treshold) * proposal_.tally.yes >
            treshold * proposal_.tally.no;

[G-10] Place if statement before assignement to a variable

When you do an if check before you assign the value to a variable and it fails you save the cost of assigning to a variable first.

• MajorityVotingBase.sol#L573-L577
• MajorityVotingBase.sol#L587-L589