PartyGovernanceNFT advertises but does not honor the ERC-4906 standard

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernanceNFT.sol#L208
https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernanceNFT.sol#L236
https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernanceNFT.sol#L247
https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernanceNFT.sol#L255

Vulnerability details

The PartyGovernanceNFT contract inherits from PartyGovernance, and through it, it advertises support for the ERC-4906 standard:

    // PartyGovernance.sol:333

    function supportsInterface(bytes4 interfaceId) public pure virtual returns (bool) {
        return
            interfaceId == type(IERC721Receiver).interfaceId ||
            interfaceId == type(ERC1155TokenReceiverBase).interfaceId ||
            // ERC4906 interface ID
            interfaceId == 0x49064906;
    }

because of this, consumers like NFT marketplaces or block explorer expect updates from PartyGovernanceNFT in the form of MetadataUpdate or BatchMetadataUpdate events whenever the metadata of its NFTs changes.

The protocol has a default implementation of Party metadata, which, among other information, includes voting power:

                // PartyNFTRenderer.sol:292
                string.concat(
                    "This membership represents ",
                    generateVotingPowerPercentage(tokenId),
                    "% voting power in ",
                    partyName,
                    ". Head to ",
                    generateExternalURL(),
                    " to view the Party's latest activity."
                );

Consequently, the metadata is expected to change whenever a single NFT's voting power, or the contract's total voting power are updated.
However, when this happens, no MetadataUpdate or BatchMetadataUpdate event is raised.

The following vote-share (and consequently metadata) changing functions have been identified, and none emits the required events:

    // PartyGovernanceNFT.sol:208
    function increaseVotingPower(uint256 tokenId, uint96 votingPower) external {
        // [...]
        emit PartyCardIntrinsicVotingPowerSet(tokenId, newIntrinsicVotingPower);

        _adjustVotingPower(ownerOf(tokenId), votingPower.safeCastUint96ToInt192(), address(0));
    }

    // PartyGovernanceNFT.sol:236
    function decreaseVotingPower(uint256 tokenId, uint96 votingPower) external {
        _assertAuthority();
        mintedVotingPower -= votingPower;
        votingPowerByTokenId[tokenId] -= votingPower;

        _adjustVotingPower(ownerOf(tokenId), -votingPower.safeCastUint96ToInt192(), address(0));
    }

    // PartyGovernanceNFT.sol:247
    function increaseTotalVotingPower(uint96 votingPower) external {
        _assertAuthority();
        _getSharedProposalStorage().governanceValues.totalVotingPower += votingPower;
    }

    // PartyGovernanceNFT.sol:255
    function decreaseTotalVotingPower(uint96 votingPower) external {
        _assertAuthority();
        _getSharedProposalStorage().governanceValues.totalVotingPower -= votingPower;
    }

As a consequence, off-chain platforms like NFT marketplaces or block explorers may show stale metadata for the NFTs, and token holders can use this stale data to their advantage.

To add context, openness to having PartyGovernanceNFT tokens traded on a marketplace seems a reasonable use case since the team opted for implementing the ERC-2981 standard for PartyGovernanceNFT tokens.

Impact

PartyGovernanceNFT tokens may be exchanged for inflated prices on platforms showing stale data.

Proof of Concept

Starting with a PartyGovernanceNFT (after crowdfunding is finalized) that delegates its tokenURI to a PartyNFTRenderer contract:

• create an NFT, and hash its tokenURI
• call increaseVotingPower or decreaseVotingPower for the given NFT:
  • hash again the NFT tokenURI and observe how it's changed
  • however no MetadataUpdate was called
• call increaseTotalVotingPower or decreaseTotalVotingPower
  • hash again the NFT tokenURI and observe how it's changed
  • however no BatchMetadataUpdate was called

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Consider updating:

• PartyGovernanceNFT.increaseVotingPower to emit a MetadataUpdate event
• PartyGovernanceNFT.decreaseVotingPower to emit a MetadataUpdate event
• PartyGovernanceNFT.increaseTotalVotingPower to emit a BatchMetadataUpdate event
• PartyGovernanceNFT.decreaseTotalVotingPower to emit a BatchMetadataUpdate event

Assessed type

ERC721

[ydspa]

QA: L

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[c4-judge]

gzeon-c4 marked the issue as primary issue

[c4-judge]

gzeon-c4 marked the issue as selected for report

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[gzeon-c4]

Judging as Med due to broken support of ERC4960

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/README.md?plain=1#L120

• PartyGovernance: Should comply with ERC4906

[c4-sponsor]

0xble (sponsor) confirmed

[3docSec]

Hi @gzeon-c4 can you please review the tags of this issue? "Selected for report", "sponsor confirmed" and "insufficient quality report" seem to somewhat clash between each other 😅

[gzeon-c4]

Hi @gzeon-c4 can you please review the tags of this issue? "Selected for report", "sponsor confirmed" and "insufficient quality report" seem to somewhat clash between each other 😅

"insufficient quality report" is tag only used for presort