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

2 stars 0 forks source link

Missing Check When Attempting to Check if the List of Precious Tokens are Correct #245

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyGovernance.sol#L690 https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyGovernance.sol#L1094-L1103 https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyGovernance.sol#L1105-L1124

Vulnerability details

Impact

There exists an issue where a missing check to ensure that the preciousTokens and preciousToksnIds list is the same length on line 1102 of the PartyGovernance.sol contract. This may lead to an inaccurate hash when attempting to run the execute() function which in turn will execute a proposal. Note that the _hashPreciousList will hash both lists of tokens and ID's as is.

Proof of Concept

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

    function _isPreciousListCorrect(
        IERC721[] memory preciousTokens,
        uint256[] memory preciousTokenIds
    )
        private
        view
        returns (bool)
    {
        return preciousListHash == _hashPreciousList(preciousTokens, preciousTokenIds);
    }

Tools Used

Manual code inspection

Recommended Mitigation Steps

I recommend a similar approach to the _setPreciousList() function which is executed in the initialisation function - to check if both lists are the same length and if not, revert with a MismatchedPreciousListLengths() error.

merklejerk commented 2 years ago

Not an issue. Precious lists are checked to be the same length when the party is first created and can never be altered. If the lengths of the passed in precious lists differ, the hash will differ from the hash of the original.