Aragon Protocol contest
Findings & Analysis Report

2023-12-12

Overview
- About C4
- Wardens
Summary
Scope
Severity Criteria
Medium Risk Findings (4)
Low Risk and Non-Critical Issues
Gas Optimizations
Disclosures

Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the Aragon Protocol smart contract system written in Solidity. The audit took place between March 3—March 10 2023.

Wardens

43 Wardens contributed reports to Aragon Protocol:

0x52
0x6980
0xAgro
0xSmartContract
0xWeiss
0xmichalis
0xnev
0xsomeone
AkshaySrivastav
BRONZEDISC
DevABDee
IceBear
JCN
Madalad
Phantasmagoria
Rageur
RaymondFam
ReyAdmirado
Rolezn
SaeedAlipoor01988
Sathish9098
V_B (Barichek and vlad_bochok)
adriro
arialblack14
atharvasama
banky
brgltd
carlitox477
chrisdior4
codeislight
descharre
hunter_w3b
imare
lukris02
luxartvinsec
matrix_0wl
rbserver
sakshamguruji
saneryee
tnevler
volodya
yongskiws

This audit was judged by 0xean.

Final report assembled by liveactionllama.

Summary

The C4 analysis yielded an aggregated total of 4 unique vulnerabilities. Of these vulnerabilities, 0 received a risk rating in the category of HIGH severity and 4 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 29 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 18 reports recommending gas optimizations.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 Aragon Protocol repository, and is composed of 62 smart contracts written in the Solidity programming language and includes 7,152 lines of Solidity code.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

Malicious Input Handling
Escalation of privileges
Arithmetic
Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

Medium Risk Findings (4)

[M-01] User may force fail the action from the `DAO:execute`

Submitted by V_B, also found by 0xWeiss

DAO.sol#L186
MajorityVotingBase.sol#L286
MajorityVotingBase.sol#L459

The execute function from the DAO.sol contract allow to execution of any call to any address if the caller has appropriate permission. Some calls are expected to be always successfully executed, and some may revert and execute will continue the execution.

The following code may call and handle call status.

address to = _actions[i].to;
(bool success, bytes memory response) = to.call{value: _actions[i].value}(
    _actions[i].data
);

if (!success) {
    // If the call failed and wasn't allowed in allowFailureMap, revert.
    if (!hasBit(_allowFailureMap, uint8(i))) {
        revert ActionFailed(i);
    }

    // If the call failed, but was allowed in allowFailureMap, store that
    // this specific action has actually failed.
    failureMap = flipBit(failureMap, uint8(i));
}

Also, the function is expected to be used in a different scenario, where the caller may be a user, voter, etc. (See MajorityVotingBase). So the caller is not a trusted entity and that means any manipulation of the DAO call should be avoided.

The problem is that caller may choose the gas with which the code is executed. If the child call execution spends enough gas then the user may choose that amount of gas, that child call frame fails, but the left gas is enough to successfully finish DAO:execute function.

Please note, even though the execute pass all gas to the child call, actually only 63/64 gas is passed and 1/64 of gas is left on the parent call (EIP-150).

https://medium.com/iovlabs-innovation-stories/the-dark-side-of-ethereum-1-64th-call-gas-reduction-ba661778568c

Attack scenario

The DAO starts majority voting, and users who have DAO tokens may vote for the proposal. The proposal is to call one target protocol, which may fail in case of an inner reason. So the DAO set that the call may fail. The approximate gas that is needed to finish the call to the target contract is 700k. A malicious voter call execute function with 711.1k of gas. Since 63/64 * 711.1 < 700, the requested call will fail. And the remaining gas is still sufficient to end the execute function logic.

Impact

The user may forcefully fail the inner call from the execute function. Also, anyone who will use the usual eth_estimateGas for the gas estimation for the execute function will accidentally calculate the amount of gas that will fail the call.

Since majority voting is hard to process with many users involved, creating another proposal may create a lot of pain.

Recommended Mitigation Steps

Add the require that gas after the call is bigger than gas before / 64.

uint256 gasBefore;
// Do call...
require(gasleft() > gasBefore/64);

novaknole20 (Aragon) confirmed

[M-02] `MerkleMinter` created through `TokenFactory` cannot be upgraded

Submitted by adriro

During the token creation process in the TokenFactory contract, the function creates a MerkleMinter contract to setup and handle token initial token distribution.

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

...

// Clone and initialize a `MerkleMinter`
address merkleMinter = merkleMinterBase.clone();
MerkleMinter(merkleMinter).initialize(
    _managingDao,
    IERC20MintableUpgradeable(token),
    distributorBase
);

...

The MerkleMinter contract is an upgradeable contract, as it inherits from PluginUUPSUpgradeable:

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

contract MerkleMinter is IMerkleMinter, PluginUUPSUpgradeable {

However, as we can see in the first code snippet, the MerkleMinter instance created in createToken is a cloned instance (using OpenZeppelin Clones library). This is incompatible with upgradeable contracts, which require the use of a proxy.

This issue will cause the MerkleMinter instance created through TokenFactory to fail to be upgraded. The MerkleMinter contract will contain all the required logic to be upgraded, but the action will fail as there is no proxy to change to a new potential implementation.

Proof of Concept

The following test illustrates the issue. We call createToken to get an instance of MerkleMinter. We then simulate a new version of the contract to upgrade to (merkleMinterV2Impl) and try to upgrade the MerkleMinter instance to this new implementation. The call fails with a “Function must be called through active proxy” error (error is defined in OpenZeppelin base UUPSUpgradeable contract).

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_TokenFactory_createToken_MerkleMinterNotUpgradeable() public {
    DAO dao = createDao();
    TokenFactory tokenFactory = new TokenFactory();
    grantRootPermission(dao, address(tokenFactory));

    TokenFactory.TokenConfig memory tokenConfig = TokenFactory.TokenConfig({
        addr: address(0),
        name: "DAO Token",
        symbol: "DAOT"
    });

    address[] memory receivers = new address[](0);
    uint256[] memory amounts = new uint256[](0);
    GovernanceERC20.MintSettings memory mintSettings = GovernanceERC20.MintSettings({
        receivers: receivers,
        amounts: amounts
    });

    (, MerkleMinter merkleMinter) = tokenFactory.createToken(dao, tokenConfig, mintSettings);

    // Assume we have a new V2 implementation...
    MerkleMinter merkleMinterV2Impl = new MerkleMinter();

    // The following will fail when the UUPS checks if the upgrade came from the proxy (since there's no proxy)
    vm.expectRevert("Function must be called through active proxy");
    PluginUUPSUpgradeable(merkleMinter).upgradeTo(address(merkleMinterV2Impl));
}

Recommendation

The MerkleMinter instance should be created using a proxy over the base implementation (createERC1967Proxy) instead of cloning the implementation:

diff --git a/src/framework/utils/TokenFactory.sol b/src/framework/utils/TokenFactory.sol
index 381e745..91441e5 100644
--- a/src/framework/utils/TokenFactory.sol
+++ b/src/framework/utils/TokenFactory.sol
@@ -15,6 +15,7 @@ import {GovernanceWrappedERC20} from "../../token/ERC20/governance/GovernanceWra
 import {IERC20MintableUpgradeable} from "../../token/ERC20/IERC20MintableUpgradeable.sol";
 import {DAO} from "../../core/dao/DAO.sol";
 import {IDAO} from "../../core/dao/IDAO.sol";
+import {createERC1967Proxy} from "../../utils/Proxy.sol";
 
 /// @title TokenFactory
 /// @author Aragon Association - 2022-2023
@@ -116,12 +117,15 @@ contract TokenFactory {
             _mintSettings
         );
 
-        // Clone and initialize a `MerkleMinter`
-        address merkleMinter = merkleMinterBase.clone();
-        MerkleMinter(merkleMinter).initialize(
-            _managingDao,
-            IERC20MintableUpgradeable(token),
-            distributorBase
+        // Create proxy and initialize a `MerkleMinter`
+        address merkleMinter = createERC1967Proxy(
+            merkleMinterBase,
+            abi.encodeWithSelector(
+                MerkleMinter.initialize.selector,
+                _managingDao,
+                token,
+                distributorBase
+            )
         );
 
         // Emit the event

	Issue
[01]	`DAO.execute` FUNCTION DOES NOT CONSIDER TOKEN’S `transfer` OR `transferFrom` FUNCTION CALL THAT DOES NOT REVERT BUT RETURNS `false` AS A FAILURE
[02]	`DAO.execute` FUNCTION DOES NOT CHECK IF `_actions[i].to` HAS ANY CONTRACT CODE WHEN `_actions[i].data` IS NOT EMPTY
[03]	WHEN `_actions[i].data` IS NOT EMPTY, `DAO.execute` FUNCTION DOES NOT CHECK IF SUCH `_actions[i].data`‘S FUNCTION EXISTS IN `_actions[i].to` CONTRACT
[04]	`PermissionManager.revoke` TRANSACTION CAN BE FRONTRUN
[05]	WHETHER `PermissionManager._grantWithCondition` FUNCTION SHOULD REVERT WHEN `_where == ANY_ADDR` OR `_who == ANY_ADDR` IS TRUE NEEDS TO BE RESOLVED
[06]	`DAO` CONTRACT’S `receive()` CAN BE UPDATED TO CALL `DAO.deposit` FUNCTION WITH `_token` INPUT BEING `address(0)`
[07]	UNLIKE `DAO.deposit`, `DAO` CONTRACT HAS NO FUNCTIONS FOR DEPOSITING ERC721 AND ERC1155 TO DAO
[08]	`PermissionManager.applyMultiTargetPermissions` FUNCTION ALREADY COVERS USE CASES OF `PermissionManager.applySingleTargetPermissions` FUNCTION
[09]	MISSING `address(0)` CHECKS FOR CRITICAL ADDRESS INPUTS
[10]	REDUNDANT RETURN STATEMENTS FOR FUNCTIONS WITH NAMED RETURNS CAN BE REMOVED
[11]	VULNERABILITIES IN VERSION 4.8.1 OF `@openzeppelin/contracts` AND `@openzeppelin/contracts-upgradeable`
[12]	SOLIDITY VERSION `0.8.19` CAN BE USED
[13]	DEFINITIONS OF `UNSET_FLAG` AND `ALLOW_FLAG` ARE INCORRECT IN DOCUMENTATION
[14]	`bytes4(0)` CAN BE REPLACED WITH A CONSTANT
[15]	`NewURI` EVENT CAN BE MOVED TO `IDAO` INTERFACE
[16]	INPUT VARIABLE CAN BE NAMED WITH LEADING UNDERSCORE
[17]	WORD TYPING TYPOS
[18]	INCOMPLETE NATSPEC COMMENTS

Number	Issue	Instances	Total Gas Saved
G-01	State variables only set in the constructor should be declared immutable	11	23100
G-02	State variables can be cached instead of re-reading them from storage	16	1600
G-03	Multiple accesses of a mapping/array should use a storage pointer	4	227
G-04	Avoid calling the same internal function multiple times	-	-

	Min	Max	Avg	# calls
Before	-	-	531312	4
After	-	-	527106	4

	Min	Max	Avg	# calls
Before	121713	178154	161829	40
After	121606	178047	161722	40

	Min	Max	Avg	# calls
Before	140546	142827	141691	14
After	140152	142433	141297	14

	Min	Max	Avg	# calls
Before	70489	72989	71991	10
After	70342	72842	71844	10

Aragon Protocol contest Findings & Analysis Report

2023-12-12

Table of contents

Overview

About C4

Wardens

Summary

Scope

Severity Criteria

Medium Risk Findings (4)

[M-01] User may force fail the action from the DAO:execute

Impact

Recommended Mitigation Steps

[M-02] MerkleMinter created through TokenFactory cannot be upgraded

Proof of Concept

Recommendation

[M-03] createProposal snapshot block can temporarily desync with minApproval / minVotingPower

Proof of Concept

Recommended Mitigation Steps

[M-04] DAO.execute(bytes32, Action[], uint256) is vulnerable to re-entrancy attacks

Impact

Proof of Concept

Recommended Mitigation Steps

Low Risk and Non-Critical Issues

Summary

[01] DAO.execute FUNCTION DOES NOT CONSIDER TOKEN’S transfer OR transferFrom FUNCTION CALL THAT DOES NOT REVERT BUT RETURNS false AS A FAILURE

[02] DAO.execute FUNCTION DOES NOT CHECK IF _actions[i].to HAS ANY CONTRACT CODE WHEN _actions[i].data IS NOT EMPTY

[03] WHEN _actions[i].data IS NOT EMPTY, DAO.execute FUNCTION DOES NOT CHECK IF SUCH _actions[i].data‘S FUNCTION EXISTS IN _actions[i].to CONTRACT

[04] PermissionManager.revoke TRANSACTION CAN BE FRONTRUN

[05] WHETHER PermissionManager._grantWithCondition FUNCTION SHOULD REVERT WHEN _where == ANY_ADDR OR _who == ANY_ADDR IS TRUE NEEDS TO BE RESOLVED

[06] DAO CONTRACT’S receive() CAN BE UPDATED TO CALL DAO.deposit FUNCTION WITH _token INPUT BEING address(0)

[07] UNLIKE DAO.deposit, DAO CONTRACT HAS NO FUNCTIONS FOR DEPOSITING ERC721 AND ERC1155 TO DAO

[08] PermissionManager.applyMultiTargetPermissions FUNCTION ALREADY COVERS USE CASES OF PermissionManager.applySingleTargetPermissions FUNCTION

[09] MISSING address(0) CHECKS FOR CRITICAL ADDRESS INPUTS

[10] REDUNDANT RETURN STATEMENTS FOR FUNCTIONS WITH NAMED RETURNS CAN BE REMOVED

[11] VULNERABILITIES IN VERSION 4.8.1 OF @openzeppelin/contracts AND @openzeppelin/contracts-upgradeable

[12] SOLIDITY VERSION 0.8.19 CAN BE USED

[13] DEFINITIONS OF UNSET_FLAG AND ALLOW_FLAG ARE INCORRECT IN DOCUMENTATION

[14] bytes4(0) CAN BE REPLACED WITH A CONSTANT

[15] NewURI EVENT CAN BE MOVED TO IDAO INTERFACE

[16] INPUT VARIABLE CAN BE NAMED WITH LEADING UNDERSCORE

[17] WORD TYPING TYPOS

[18] INCOMPLETE NATSPEC COMMENTS

Gas Optimizations

Summary

Gas Optimizations

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

Gas Savings for createPluginRepo(), obtained via protocol’s tests: Avg 4206 gas | Set pluginRepoRegistry and pluginRepoBase to immutable to save ~4200 gas.

Set repoRegistry as immutable to save ~2100 gas

Move setupBases() logic into the constructor and set governanceERC20Base, governanceWrappedERC20Base, merkleMinterBase, and distributorBase to immutable to save ~8400 gas.

Set multiplyHelperBase and counterBase to immutable to save ~4200 gas.

Set multiplyHelperBase and counterBase to immutable to save ~4200 gas.

[G-02] State variables can be cached instead of re-reading them from storage

Gas Savings for createVersion(), obtained via protocol’s tests: Avg 107 gas | Cache latestRelease as a stack variable to save 1 SLOAD

Gas Savings for createVersion(), obtained via protocol’s tests: Avg 394 gas | Cache node, ens, and resolver to save 4 SLOADs

Cache token as a stack variable to save 1 SLOAD

Cache signatureValidator as a stack variable to save 1 SLOAD

Cache version.tag.release as a stack variable to save 2 SLOADs

Cache counterBase and multiplyHelperBase as stack variables to save 2 SLOADs

Cache counterBase and multiplyHelperBase as stack variables to save 2 SLOADs (Identical to instance above)

Cache proposal_.parameters.supportThreshold as a stack variable to save 1 SLOAD

Cache proposal_.tally.yes and proposal_.parameters.supportThreshold as stack variables to save 2 SLOADs

[G-03] Multiple accesses of a mapping/array should use a storage pointer

Gas Savings for execute(), obtained via protocol’s tests: Avg 147 gas | Use storage pointer proposal_ instead of re-accessing the mapping.

proposals[_proposalId] can be cached as a storage pointer

[G-04] Avoid calling the same internal function multiple times

Cache value from _msgSender()

Disclosures

Aragon Protocol contest
Findings & Analysis Report

[M-01] User may force fail the action from the `DAO:execute`

[M-02] `MerkleMinter` created through `TokenFactory` cannot be upgraded

[M-03] `createProposal` snapshot block can temporarily desync with `minApproval` / `minVotingPower`

[M-04] `DAO.execute`(`bytes32`, `Action[]`, `uint256`) is vulnerable to re-entrancy attacks

[01] `DAO.execute` FUNCTION DOES NOT CONSIDER TOKEN’S `transfer` OR `transferFrom` FUNCTION CALL THAT DOES NOT REVERT BUT RETURNS `false` AS A FAILURE

[02] `DAO.execute` FUNCTION DOES NOT CHECK IF `_actions[i].to` HAS ANY CONTRACT CODE WHEN `_actions[i].data` IS NOT EMPTY

[03] WHEN `_actions[i].data` IS NOT EMPTY, `DAO.execute` FUNCTION DOES NOT CHECK IF SUCH `_actions[i].data`‘S FUNCTION EXISTS IN `_actions[i].to` CONTRACT

[04] `PermissionManager.revoke` TRANSACTION CAN BE FRONTRUN

[05] WHETHER `PermissionManager._grantWithCondition` FUNCTION SHOULD REVERT WHEN `_where == ANY_ADDR` OR `_who == ANY_ADDR` IS TRUE NEEDS TO BE RESOLVED

[06] `DAO` CONTRACT’S `receive()` CAN BE UPDATED TO CALL `DAO.deposit` FUNCTION WITH `_token` INPUT BEING `address(0)`

[07] UNLIKE `DAO.deposit`, `DAO` CONTRACT HAS NO FUNCTIONS FOR DEPOSITING ERC721 AND ERC1155 TO DAO

[08] `PermissionManager.applyMultiTargetPermissions` FUNCTION ALREADY COVERS USE CASES OF `PermissionManager.applySingleTargetPermissions` FUNCTION

[09] MISSING `address(0)` CHECKS FOR CRITICAL ADDRESS INPUTS

[11] VULNERABILITIES IN VERSION 4.8.1 OF `@openzeppelin/contracts` AND `@openzeppelin/contracts-upgradeable`

[12] SOLIDITY VERSION `0.8.19` CAN BE USED

[13] DEFINITIONS OF `UNSET_FLAG` AND `ALLOW_FLAG` ARE INCORRECT IN DOCUMENTATION

[14] `bytes4(0)` CAN BE REPLACED WITH A CONSTANT

[15] `NewURI` EVENT CAN BE MOVED TO `IDAO` INTERFACE

Gas Savings for `createPluginRepo()`, obtained via protocol’s tests: Avg 4206 gas | Set `pluginRepoRegistry` and `pluginRepoBase` to `immutable` to save ~4200 gas.

Set `repoRegistry` as `immutable` to save ~2100 gas

Move `setupBases()` logic into the `constructor` and set `governanceERC20Base`, `governanceWrappedERC20Base`, `merkleMinterBase`, and `distributorBase` to `immutable` to save ~8400 gas.

Set `multiplyHelperBase` and `counterBase` to `immutable` to save ~4200 gas.

Set `multiplyHelperBase` and `counterBase` to `immutable` to save ~4200 gas.

Gas Savings for `createVersion()`, obtained via protocol’s tests: Avg 107 gas | Cache `latestRelease` as a stack variable to save 1 SLOAD

Gas Savings for `createVersion()`, obtained via protocol’s tests: Avg 394 gas | Cache `node`, `ens`, and `resolver` to save 4 SLOADs

Cache `token` as a stack variable to save 1 SLOAD

Cache `signatureValidator` as a stack variable to save 1 SLOAD

Cache `version.tag.release` as a stack variable to save 2 SLOADs

Cache `counterBase` and `multiplyHelperBase` as stack variables to save 2 SLOADs

Cache `counterBase` and `multiplyHelperBase` as stack variables to save 2 SLOADs (Identical to instance above)

Cache `proposal_.parameters.supportThreshold` as a stack variable to save 1 SLOAD

Cache `proposal_.tally.yes` and `proposal_.parameters.supportThreshold` as stack variables to save 2 SLOADs

Gas Savings for `execute()`, obtained via protocol’s tests: Avg 147 gas | Use storage pointer `proposal_` instead of re-accessing the mapping.

`proposals[_proposalId]` can be cached as a storage pointer

Cache value from `_msgSender()`