ArbitraryCallsProposal: onERC1155Received callable

[code423n4]

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ArbitraryCallsProposal.sol#L194

Vulnerability details

Impact

In ArbitraryCallsProposal, any calls to onERC721Received are restricted to not trick contracts into thinking they received this token. However, calls to onERC1155Received can still be made. This is problematic because ERC1155 is backwards compatible with ERC721 (see Backwards Compatibility in https://eips.ethereum.org/EIPS/eip-1155) and there are hybrid ERC-1155/ERC-721 contracts (e.g., https://github.com/thesandboxgame/thesandbox-contracts/blob/master/src/Asset/ERC1155ERC721.sol)

Proof Of Concept

Because these hybrid tokens exist, a smart contract that receives ERC1155 & ERC721 tokens might have a logic like that:

function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes memory data
    ) external returns (bytes4) {
		_handleIncomingNFT(operator, from, tokenId, data);
		return IERC721Receiver.onERC721Received.selector;
}

function onERC1155Received(address operator, address from, uint256 tokenId, uint256 value, bytes calldata data) external returns(bytes4) {
		if (isNFT[msg.sender] && value == 1) # isNFT is a mapping that stores ERC1155 tokens where balanceOf(tokenId) <= 1
			_handleIncomingNFT(operator, from, tokenId, data);
		...
		return IERC1155Receiver.onERC1155Received.selector;
}

In such a scenario, they could still be tricked into thinking that they received a NFT.

Recommended Mitigation Steps

Also disallow calls to onERC1155Received.

[merklejerk]

Not an issue for our protocol specifically, but we can imagine some market or external protocol accepting these tokens in some unified 1155/721 solution that might get tripped up. Will also disallow calls to onERC1155Received in arbitrary calls.

[0xble]

Resolved: PartyDAO/party-protocol#128

[HardlyDifficult]

Interesting scenario. If a market did get tripped up by this, seems that's a bug for them to deal with, e.g. by confirming ownership. I'm not opposed to the recommendation - but the potential impact in terms of how it's relevant to the party doesn't seem to be explored. Downgrading to Low risk and merging with #101