code-423n4 / 2022-09-party-findings

2 stars 0 forks source link

Accidental `newPartyHost` in `PartyGovernance.sol` Could Result in Irreversible Changes #226

Closed code423n4 closed 1 year ago

code423n4 commented 2 years ago

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyGovernance.sol#L458-L469

Vulnerability details

Description

There exists a functionality in the PartyGovernance.sol contract where a user has the ability to nominate and transfer the partyHost status to another address. This change of address is implemented instantaneously which is generally discouraged due to an error in the addresses.

Impact

Whilst the contract does check for a zero address, if a partyHost were to transfer their status to another user and accidentally makes an error in the address they wish to assign, the action is irreversible.

Proof of Concept

Source: https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyGovernance.sol#L458-L469

    /// @notice Transfer party host status to another.
    /// @param newPartyHost The address of the new host.
    function abdicate(address newPartyHost) external onlyHost onlyDelegateCall { 
        // 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;
        }
        isHost[msg.sender] = false;
        emit HostStatusTransferred(msg.sender, newPartyHost);
    }

Tools Used

Manual code inspection

Recommended Mitigation Steps

Since this is a critical function of the contract, I recommend implementing this in the form of a two step change. This can be done by storing the new partyHost in a temporary variable in the form of a mapping along side the previous user (mapping(address => address)). A confirmAbdicate() function can then be implemented to confirm these changes are correct.

merklejerk commented 1 year ago

OK with risk. Prefer to guard against user error on FE. Host could also easily transfer to a dead address, that isn't nonzero.

HardlyDifficult commented 1 year ago

This is a valid suggestion to help prevent user error. Downgrading to QA and merging with #250