Aragon Protocol contest - 0xnev's results

Issues Template

Non-Critical Template

Ordinary Issues Template

[N-01] Lack of zero-address checks in the constructor

Context:

6 results - 4 files

/DAOFactory.sol
53:    constructor(DAORegistry _registry, PluginSetupProcessor _pluginSetupProcessor) {
54:        daoRegistry = _registry;
55:        pluginSetupProcessor = _pluginSetupProcessor;

/PluginRepoFactory.sol
22:    constructor(PluginRepoRegistry _pluginRepoRegistry) {
23:        pluginRepoRegistry = _pluginRepoRegistry;


/PluginSetupProcessor.sol
277:    constructor(PluginRepoRegistry _repoRegistry) {
278:        repoRegistry = _repoRegistry;
279:    }

/CounterV2PluginSetup.sol
24:    constructor(MultiplyHelper _helper) {
25:        multiplyHelperBase = _helper;


/DaoAuthorizable.sol
19:    constructor(IDAO _dao) {
20:        dao_ = _dao;
21:    }

Description: Zero-address check should be implemented in constructors to avoid the risk of setting address(0) at deployment time for immutable address variables.

Reccomendation: Add a zero-address check for the immutable variables for the instances above using custom errors.

[N-02] Remove functions not used

1 result - 1 file
/TokenFactory.sol
8: function _uncheckedIncrement(uint256 i) pure returns (uint256)

Recommendation: Consider removing functions not used to save gas on deployment

[N-03] Omission of important parameters in events emitted

3 results - 3 files

/MajorityVotingBase.sol
526:    function _updateVotingSettings(VotingSettings calldata _votingSettings) internal virtual
550:        emit VotingSettingsUpdated({
551:            votingMode: _votingSettings.votingMode,
552:            supportThreshold: _votingSettings.supportThreshold,
553:            minParticipation: _votingSettings.minParticipation,
554:            minDuration: _votingSettings.minDuration,
555:            minProposerVotingPower: _votingSettings.minProposerVotingPower
556:        });

/Multisig.sol
413:    function _updateMultisigSettings(MultisigSettings calldata _multisigSettings) internal
429:        emit MultisigSettingsUpdated({
430:            onlyListed: _multisigSettings.onlyListed,
431:            minApprovals: _multisigSettings.minApprovals
432:        });

/PluginRepo.sol
187:    function updateReleaseMetadata
203:        emit ReleaseMetadataUpdated(_release, _releaseMetadata);

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

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

[N-04] Implement checks for input validation for setters

6 results - 5 files

/DAOFactory.sol
156:    function _setDAOPermissions(DAO _dao) internal

/PluginRepoFactory.sol
65:     function _setPluginRepoPermissions(PluginRepo pluginRepo, address maintainer) internal

/CounterV2.sol
52:    function setNewVariable(uint256 _newVariable) external reinitializer(2) {
53:        newVariable = _newVariable;
54:    }

/DAO.sol
283:    function _setTrustedForwarder(address _trustedForwarder) internal {
284:        trustedForwarder = _trustedForwarder;
285:
286:        emit TrustedForwarderSet(_trustedForwarder);
287:    }

239:    function setSignatureValidator(
240:        address _signatureValidator
241:    ) external override auth(SET_SIGNATURE_VALIDATOR_PERMISSION_ID) {
242:        signatureValidator = IERC1271(_signatureValidator);
243:
244:        emit SignatureValidatorSet({signatureValidator: _signatureValidator});
245:    }

/DaoAuthorizableUpgradeable.sol
20:    function __DaoAuthorizableUpgradeable_init(IDAO _dao) internal onlyInitializing {
21:        dao_ = _dao;
22:    }

Recommendation: zero-address checks can be added in the functions with address inputs to ensure the addresses set aren't address(0).

In the setNewVariable function, a check can be made to ensure the _newVariable is set as non-zero.

[O-01] Function naming suggestion of internal and private functions starts with _

7 results - 6 files

/TokenFactory.sol
144: function setupBases() private

/PluginRepo.sol
251: function tagHash(Tag memory _tag) internal pure returns (bytes32)

/PluginSetupProcessor.sol
719: function emitPrepareUpdateEvent

/PluginSetup.sol
33: function createERC1967Proxy

/PermissionManager.sol
342: function permissionHash
354: function isPermissionRestrictedForAnyAddr

/DAO.sol
122: function isPermissionRestrictedForAnyAddr

Proper use of _ as a function name prefix and a common pattern is to prefix internal and private function names with _. internal and private functions: the mixedCase format starting with an underscore (_mixedCase starting with an underscore)

[O-02] Solidity compiler optimizations can be problematic

/hardhat.config.ts
47: const config: HardhatUserConfig = {
48:   solidity: {
49:     version: '0.8.17',
50:     settings: {
51:       optimizer: {
52:         enabled: true,
53:         runs: 2000,

Description Protocol has enabled optional compiler optimizations in Solidity. There have been several optimization bugs with security implications. Moreover, optimizations are actively being developed. Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. It is likely that there are latent bugs related to optimization and that new bugs will be introduced due to future optimizations. Exploit Scenario: A latent or future bug in Solidity compiler optimizations—or in the Emscripten transpilation to solc-js—causes a security vulnerability in the contracts.

---

Recommendation: Short term, measure the gas savings from optimizations and carefully weigh them against the possibility of an optimization-related bug. Long term, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

Gas Optimization Template

[G-01] Use nested if statements to avoid multiple check combinations using &&

Context:

13 results - 9 files

/RegistryUtils.sol
20: if (char > 96 && char < 123) 
25: if (char > 47 && char < 58)

/AddresslistVoting.sol
97: if (minProposerVotingPower() != 0 && !isListedAtBlock(_msgSender(), snapshotBlock))
185: if (_tryEarlyExecution && _canExecute(_proposalId))

214:        if (
215:            proposal_.voters[_account] != VoteOption.None &&
216:            proposal_.parameters.votingMode != VotingMode.VoteReplacement
217:        )

/TokenVoting.sol
182: if (_tryEarlyExecution && _canExecute(_proposalId))

211:        if (
212:            proposal_.voters[_account] != VoteOption.None &&
213:            proposal_.parameters.votingMode != VotingMode.VoteReplacement
214:        )

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

/PluginRepo.sol
157: if (version.tag.release != 0 && version.tag.release != _release)

/PluginSetupProcessor.sol
700:        if (
701:            msg.sender != _dao &&
702:            !DAO(payable(_dao)).hasPermission(address(this), msg.sender, _permissionId, bytes(""))
703:        )

/PermissionManager.sol
236: if (_where == ANY_ADDR && _who == ANY_ADDR)

/GovernanceWrappedERC20.sol
106:if (to != address(0) && numCheckpoints(to) == 0 && delegates(to) == address(0))

/GovernanceERC20.sol
116: if (to != address(0) && numCheckpoints(to) == 0 && delegates(to) == address(0))

Recommendation: Replace && used within if and else if statements with nested if statements

[G-02] internal functions only called once can be inlined to save gas

Description: Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

Context: Consider inlining less complex functions for the internal functions instances below:

4 results - 4 files

/DAOFactory.sol
141: function _createDAO(DAOSettings calldata _daoSettings) internal returns (DAO dao)

/PermissionManager.sol
212: function _initializePermissionManager(address _initialOwner) internal

/ProposalUpgradeable.sol
33: function _createProposalId() internal returns (uint256 proposalId)

/Proposal.sol
33: function _createProposalId() internal returns (uint256 proposalId)

• _createDAO() only used in createDAO() within the same contract <br/>
• _initializePermissionManager() only used in __PermissionManager_init() within the same contract <br/>
• _createProposalId() only used in _createProposal() within the same contract <br/>
• _createProposalId() only used in _createProposal() within the same contract

For more complex functions listed below, consider inlining if it does not result in core functions being too complex:

2 results - 2 files

/DAOFactory.sol
156: function _setDAOPermissions(DAO _dao) internal

/PluginRepoFactory.sol
65: function _setPluginRepoPermissions

Similarly, consider inlining the following private function instances if core function does not get too complex:

6 results - 5 files

/TokenFactory.sol
144: function setupBases() private

/TokenVotingSetup.sol
276: function _isERC20(address token) private view returns (bool)

/PluginSetupProcessor.sol
699: function _canApply(address _dao, bytes32 _permissionId) private view
719: function emitPrepareUpdateEvent

/MerkleDistributor.sol
120: function _setClaimed(uint256 _index) private

/DAO.sol
290: function _registerTokenInterfaces() private

• setupBases() only used in constructor() within the same contract

  <br/>

• _isERC20() only used in prepareInstallation() within the same contract

  <br/>

• _canApply() only used in canApply() within the same contract

  <br/>

• emitPrepareUpdateEvent() only used in prepareUpdate within the same contract

  <br/>

• _setClaimed() only used in claim() within the same contract

  <br/>

• _registerTokenInterfaces() only used in initialize() within the same contract

• _setDAOPermissions() only used in createDAO() within the same contract

  <br/>

• _setPluginRepoPermissions only used in createPluginRepoWithFirstVersion() within the same contract

[G-03] <x> += <y> costs more gas than <x> = <x> + <y> for state variables (same with -=)

Context:

1 result - 1 file

/Multisig.sol
276: proposal_.approvals += 1;

Recommendation: Replace <x> += <y> with <x> = <x> + <y> for state variables (same for -=)

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

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

Recommendation: Cache variable like this SomeValue storage someValue = someMap[someIndex]

MajorityVotingBase.sol

Cache proposals[_proposalId] in local storage

1 result

457:    function _execute(uint256 _proposalId) internal virtual {
458:        proposals[_proposalId].executed = true;
459:
460:        _executeProposal(
461:            dao(),
462:            _proposalId,
463:            proposals[_proposalId].actions,
464:            proposals[_proposalId].allowFailureMap
465:        );
466:    }

MerkleDistributor.sol

Cache claimedBitMap[claimedWord_index] in local storage

1 result

120:    function _setClaimed(uint256 _index) private
123:        claimedBitMap[claimedWord_index] =
124:            claimedBitMap[claimedWord_index]

[G-05] Initialize stack variables outside loop

4 results - 3 files

/RegistryUtils.sol
16:    for (uint256 i; i < nameLength; i++) {
17:        uint8 char = uint8(nameBytes[i]);

/DAOFactory.sol
95:        for (uint256 i; i < _pluginSettings.length; ++i) {
96:            // Prepare plugin.
97:            (
98:                address plugin,
99:                IPluginSetup.PreparedSetupData memory preparedSetupData

/DAO.sol
184:        for (uint256 i = 0; i < _actions.length; ) {
185:            address to = _actions[i].to;

Description: Consider initializing the stack variables before the loop to avoid reinitialization on every loop

Recommendation: Consider initializing the stack variables before the loop to save gas

[G-06] Increments can be unchecked

2 results - 2 files

/RegistryUtils.sol
16: for (uint256 i; i < nameLength; i++)

/DAOFactory.sol
95: for (uint256 i; i < _pluginSettings.length; ++i)

Description: In Solidity 0.8.0+, there is a default overflow check on unsigned integers. It is possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

Recommendation: Code goes from:

for (uint256 i; i < numIterations; ++i) {  
 // ...  
}  

to

for (uint256 i; i < numIterations;) {  
 // ...  
 unchecked { ++i; }  
}  

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

5 results - 2 files

/TokenFactory.sol
27: address public governanceERC20Base;
30: address public governanceWrappedERC20Base;
33: address public merkleMinterBase;
36: MerkleDistributor public distributorBase;

/PluginSetupProcessor.sol
128: PluginRepoRegistry public repoRegistry;

Description: This avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).

Recommendation: Declare state variables only assigned in the constructor to be immutable

[G-08] Remove function not used

1 result - 1 file

/TokenFactory.sol
8: function _uncheckedIncrement(uint256 i) pure returns (uint256)

Recommendation: Consider removing functions not used to save gas on deployment