Unsafe abi.decode

[code423n4]

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/d129d647796369a18e30b336e74e7d1bfc779597/contracts/utils/LibSafeERC721.sol#L16 https://github.com/PartyDAO/party-contracts-c4/blob/d129d647796369a18e30b336e74e7d1bfc779597/contracts/utils/LibERC20Compat.sol#L12

Vulnerability details

Description

There is safeOwnerOf function in the LibSafeERC721 contract and compatTransfer function in the LibERC20Compat contract. Both of them are designed in a way to be compatiable with incorrect implementation of the contract to be called. safeOwnerOf even declared to proceed without reverting in any case. But both of them reverting on abi.decode(...) when the data returned from the call can't be correctly decoded into the specified type.

Proof of Concept

contract Hack
{
    // LibSafeERC721 hack

    function ownerOf(uint256) external pure returns (uint256) {
        return 2**160;
    }

    function hack1() external view returns (address) {
        return LibSafeERC721.safeOwnerOf(address(this), 0);
    }

    // LibERC20Compat hack

    function transfer(address, uint256) external pure returns (uint256) {
        return 2;
    }

    function hack2() external {
        LibERC20Compat.compatTransfer(address(this), address(0), 0);
    }
}

Recommended Mitigation Steps

Use assembly instructions for decoding the result or add special checks on the returned bytes array.

[merklejerk]

The system is designed to work only with compliant ERC721s. Our ERC20 solution already works with all major ERC20s that are both compliant and non-compliant. We don't think it's worth accommodating extremely broken ERC20s.

[HardlyDifficult]

Although this may not be required / relevant, and the report does not include concrete examples to justify the severity - it's a valid suggestion to consider to potentially be more robust. Lowering risk and merging with the warden's QA report #348