Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No hosts in PartyGovernance can block important governance functionalities #47

Closed
c4-submissions opened this issue Nov 4, 2023 · 9 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-429 edited-by-warden insufficient quality report This report is not of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-submissions
Copy link
Contributor

c4-submissions commented Nov 4, 2023

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L457-L472
https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L216-L220
https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L665-L681

Vulnerability details

Title

No hosts in PartyGovernance can block important governance functionalities

Links to affected code:

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L457-L472
https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L216-L220
https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L665-L681

Impact

In PartyGovernance.sol, a host can abdicate its role to another address.

PartyGovernance.sol#L455-L457

    /// @notice Transfer party host status to another.
    /// @param newPartyHost The address of the new host.
    function abdicateHost(address newPartyHost) external {

This function checks at the beginning if msg.sender is among the hosts, and, if not, reverts.
However, if newPartyHost is address(0x0), msg.sender will be removed in isHost mapping, and will not be a host anymore.
Furthermore, the counter numHosts is decreased by one.

We consider the case where a party is created with two hosts (like in tests).
If both of them abdicate to address(0x0), there will be no hosts for that party.

Despite it should be a desired behavior, we want to point out why it could be critical.
We start with contest's README:

Publicly Known Issues section

Hosts have unilateral veto power on any proposal. Hosts can block a governance party in this way.
There are emergency admin recovery functions on Parties and crowdfunds callable by the multisig that can execute arbitrary bytecode, though party hosts can disable them.

Additional Context

The protocol includes two primary trusted roles, each with unique capabilities and responsibilities:

Hosts:
    A role in the Party for trusted addresses that grants the ability to unilaterally veto proposals in the Party and configure Rage Quit. 
    Each Host may or may not be a member (i.e. have non-zero voting power in the Party).

Authorities: 
    A privileged role with root access to the Party.
    Can perform sensitive operations like minting and burning new memberships in a Party and updating the voting power of members.
    Authorities are expected to be trusted smart contracts, not EOAs.

So, the host is the unique role that can perform veto operation and that can configure Rage Quit.
Both operations are not indispensable, but are very important in the governance of a party, especially if wrong values were set by the last host and then it abdicate to address(0x0): those values will not be changed anymore.

In fact, there are two critical issues in these operations:

  1. only hosts can perform them. Authorities, the role with root access, can't perform them.
  2. hosts can only transfer its role or burn it. There is no way to create a new host after a party is created.

As stated in Severity categorization,

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value 
with a hypothetical attack path with stated assumptions, but external requirements.

We consider the lack of hosts a situation where part of governance protocol availability is impacted.
Furthermore, these functionalities were implemented in this way. This means that there is the possibility that a host abdicates to address(0x0), and so there is the possibility that a party with initial hosts, will not have hosts anymore.

Proof of Concept

There is a function that check host condition:

PartyGovernance.sol#L216-L220

PartyGovernance.sol#L216-L220

function _assertHost() internal view {
    if (!isHost[msg.sender]) {
        revert NotAuthorized();
    }
}

This check is based on isHost mapping:

PartyGovernance.sol#L205-L206

PartyGovernance.sol#L205-L206

/// @notice Whether an address is a party host.
mapping(address => bool) public isHost;

isHost is only manipulated in

PartyGovernance._initialize()

PartyGovernance.abdicateHost()

So, after initialization, only abdicateHost() can modify isHost mapping.
Let's go deeper into this function:

PartyGovernance.sol#L455-L472

PartyGovernance.sol#L455-L472

/// @notice Transfer party host status to another.
/// @param newPartyHost The address of the new host.
function abdicateHost(address newPartyHost) external {
    _assertHost();
    // 0 is a special case burn address.
    if (newPartyHost != address(0)) {
        // Cannot transfer host status to an existing host.
        if (isHost[newPartyHost]) {
            revert InvalidNewHostError();
        }
        isHost[newPartyHost] = true;
    } else {
        // Burned the host status
        --numHosts;
    }
    isHost[msg.sender] = false;
    emit HostStatusTransferred(msg.sender, newPartyHost);
}

When newPartyHost != address(0), newPartyHost is added into isHost, and msg.sender is removed. So, numHosts remains unchanged.
However, when newPartyHost == address(0), numHosts is decreased by one, msg.sender is removed and no host is added.

Now, what if the last host calls this function with newPartyHost == address(0)?
isHost will have only address -> false. This means that _assertHost() will revert forever, and there will be no way to create a new host or change this behavior.

Now, _assertHost() is used inside two functions:

PartyGovernance.veto()

    /// @notice As a party host, veto a proposal, unilaterally rejecting it.
    /// @dev The proposal will never be executable and cannot be voted on anymore.
    ///      A proposal that has been already executed at least once (in the `InProgress` status)
    ///      cannot be vetoed.
    /// @param proposalId The ID of the proposal to veto.
    function veto(uint256 proposalId) external {
        _assertHost();

    [...]

PartyGovernanceNFT.setRageQuit()

    /// @notice Set the timestamp until which ragequit is enabled.
    /// @param newRageQuitTimestamp The new ragequit timestamp.
    function setRageQuit(uint40 newRageQuitTimestamp) external {
        _assertHost();

    [...]

Both of these will be unavailable forever.

We implemented a test in order to remove all hosts:

function test_deleteAlleHosts() public {
    (
        Party party,
        IERC721[] memory preciousTokens,
        uint256[] memory preciousTokenIds
    ) = partyAdmin.createParty(
            partyImpl,
            PartyAdmin.PartyCreationMinimalOptions({
                host1: address(this),
                host2: address(steve),
                passThresholdBps: 5000,
                totalVotingPower: 43,
                preciousTokenAddress: address(toadz),
                preciousTokenId: 1,
                rageQuitTimestamp: 0,
                feeBps: 0,
                feeRecipient: payable(0)
            })
        );

    // Remove first host
    party.abdicateHost(address(0));
    assertEq(party.numHosts(), 1);

    // Remove secondo and last host
    vm.prank(address(steve));
    party.abdicateHost(address(0));

    // Now, there is no host.
    assertEq(party.numHosts(), 0);

}
forge test --match-test test_deleteAlleHosts

[PASS] test_deleteAlleHosts() (gas: 286960)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.62ms

Tools Used

Foundry and Visual ispection

Recommended Mitigation Steps

It should be better if Authority role has the ability of add new host.
Furthermore, abdicateHost() could be changed in order to remove an host only if it is not the last one:

/// @notice Transfer party host status to another.
/// @param newPartyHost The address of the new host.
function abdicateHost(address newPartyHost) external {
    _assertHost();
    // 0 is a special case burn address.
    if (newPartyHost != address(0)) {
        // Cannot transfer host status to an existing host.
        if (isHost[newPartyHost]) {
            revert InvalidNewHostError();
        }
        isHost[newPartyHost] = true;
    } else {
        if (numHosts == 1){
            revert AtLeastOneHost();
        }

        // Burned the host status
        --numHosts;
    }
    isHost[msg.sender] = false;
    emit HostStatusTransferred(msg.sender, newPartyHost);
}

Assessed type

Governance

@c4-submissions c4-submissions added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Nov 4, 2023
c4-submissions added a commit that referenced this issue Nov 4, 2023
@c4-pre-sort
Copy link

ydspa marked the issue as duplicate of #429

@c4-pre-sort c4-pre-sort added the insufficient quality report This report is not of sufficient quality label Nov 12, 2023
@c4-pre-sort
Copy link

ydspa marked the issue as insufficient quality report

@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Nov 19, 2023
@c4-judge
Copy link
Contributor

gzeon-c4 marked the issue as unsatisfactory:
Invalid

@niser93
Copy link

niser93 commented Nov 21, 2023

I don't understand why it is invalid. I supplied a working POC with test code that is able to remove all hosts.
I understand that this problem could not be considered as medium, but is valid for sure.
Similar issues were judged as QA:
#242 use similar approach to underlying the wrong management of hosts and numHost variable, but it doesn't supply a coded POC with tests
#511 use similar approach, but about authority, only underlying that there is no functionalities to abdicate. What if this functionality was implemented, but permit to delete all authorities? Why it is not the same about hosts, if even absence of hosts can block important functionalities of the contract?
Furthemore this issue doesn't have code POC nor coded mitigation
#543 report the absence of mechanism to remove authority (similar consideration of #511)

Moreover, i'm wondering if developers will change "hosts management" in future implementations. If they will change this check in order to avoid deleting of all hosts, for sure my report had given a value for them.

Thanks in advance

@niser93
Copy link

niser93 commented Nov 22, 2023

I would like to add that primary issue, #429 was judge invalid due to "user error".
However, hosts are not necessarily who has created party, i.e. they don't know overall number of hosts and they don't know if they are the last one.
I want underlying that the problem is this: management of hosts in a distributed way should have some kind of bondages, in order to avoid reaching irreversible state: num of hosts can only decrease, so we could assume that in a sufficient long time, every party will have no hosts.

@gzeon-c4
Copy link

Hosts are trusted addresses, host being able to abdicate themselves is an expected behavior.

@niser93
Copy link

niser93 commented Nov 23, 2023

But there is no way to add new hosts. As #511, that was judged as QA due to the lack of abdicate function for authorities, the lack of adding function for hosts should be QA, isn't it?
Thanks in advance

@gzeon-c4
Copy link

#511 (comment)

@niser93
Copy link

niser93 commented Nov 23, 2023

Ok, but according to #233, despite hosts are trusted address, sponsor accepted case where they are not fair behavior.
Furthermore #233 (comment)

However, i accept your judgement. Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-429 edited-by-warden insufficient quality report This report is not of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

6 participants