Wrong input hash given to `decodeType` function in `CreateOffererHelpers` library

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/main/src/libraries/CreateOffererLib.sol#L199-L206 https://github.com/code-423n4/2023-09-delegate/blob/main/src/libraries/CreateOffererLib.sol#L292-L296 https://github.com/code-423n4/2023-09-delegate/blob/main/src/CreateOfferer.sol#L78 https://github.com/code-423n4/2023-09-delegate/blob/main/src/CreateOfferer.sol#L61

Vulnerability details

Impact

The RegistryHashes.decodeType function is supposed to take a specific bytes32 hash as input with an encoded token type in the last bytes to know token type is used, but when this function is called in the CreateOffererHelpers library it is given a simple token Id casted into bytes32 as input which will make the function return a wrong token type (if any) as the given input hasn't the encoded bytes specific to the token type.

This issue will cause both CreateOffererHelpers.updateTransientState and CreateOffererHelpers.verifyCreate functions to behave in an undesired manner (or even revert as token type will be wrong) which hinder the process of creating delegate token through the CreateOfferer contract.

Proof of Concept

First let's take a look at the RegistryHashes.decodeType here function below :

function decodeType(bytes32 inputHash) internal pure returns (IDelegateRegistry.DelegationType decodedType) {
    assembly {
        decodedType := and(inputHash, EXTRACT_LAST_BYTE)
    }
}

The function basically extract the token type from the last bytes of the given bytes32 input hash inputHash, this hash must be encoded with one of the functions : allHash, contractHash, erc721Hash, erc20Hash and erc1155Hash and this will give us the five available types (ALL_TYPE, CONTRACT_TYPE, ERC721_TYPE, ERC20_TYPE and ERC1155_TYPE) declared in the RegistryHashes library.

The issue now is that when the decodeType function is called in the functions CreateOffererHelpers.updateTransientState and CreateOffererHelpers.verifyCreate, it is not given a correctly encoded hash as explained above but instead the input to the function is a token id uint casted into bytes32 as highlighted below :

• Instance 1 : The updateTransientState function :

function updateTransientState(
    CreateOffererStructs.TransientState storage transientState,
    address fulfiller,
    SpentItem calldata minimumReceived,
    SpentItem calldata maximumSpent,
    CreateOffererStructs.Context memory decodedContext
) internal {
    //@audit minimumReceived.identifier is a simple token id
    IDelegateRegistry.DelegationType tokenType = RegistryHashes.decodeType(
        bytes32(minimumReceived.identifier)
    );
    if (tokenType == IDelegateRegistry.DelegationType.ERC721) {
    ...
}

The decodeType function is given bytes32(minimumReceived.identifier) as input even though minimumReceived.identifier is just a simple token id that was given by seaport when generateOrder function was called :

function generateOrder(
    address fulfiller,
    SpentItem[] calldata minimumReceived,
    SpentItem[] calldata maximumSpent,
    bytes calldata context
)
    external
    checkStage(Enums.Stage.generate, Enums.Stage.transfer)
    onlySeaport(msg.sender)
    returns (SpentItem[] memory offer, ReceivedItem[] memory consideration)
{
    if (context.length != 160) revert Errors.InvalidContextLength();
    Structs.Context memory decodedContext = abi.decode(
        context,
        (Structs.Context)
    );
    (offer, consideration) = Helpers.processSpentItems(
        minimumReceived,
        maximumSpent
    );
    Helpers.updateTransientState(
        transientState,
        fulfiller,
        minimumReceived[0],
        maximumSpent[0],
        decodedContext
    );
}

This is even explained in the seaport code comments where they indicate that the identifier parameter is a token Id (which will be zero in case of ERC20) :

/**
 * @dev An offer item has five components: an item type (ETH or other native
 *      tokens, ERC20, ERC721, and ERC1155, as well as criteria-based ERC721 and
 *      ERC1155), a token address, a dual-purpose "identifierOrCriteria"
 *      component that will either represent a tokenId or a merkle root
 *      depending on the item type, and a start and end amount that support
 *      increasing or decreasing amounts over the duration of the respective
 *      order.
 */
struct OfferItem {
    ItemType itemType;
    address token;
    uint256 identifierOrCriteria; //@audit simple token id, not encoded like in `RegistryHashes`
    uint256 startAmount;
    uint256 endAmount;
}

/**
 * @dev A consideration item has the same five components as an offer item and
 *      an additional sixth component designating the required recipient of the
 *      item.
 */
struct ConsiderationItem {
    ItemType itemType;
    address token;
    uint256 identifierOrCriteria; //@audit simple token id, not encoded like in `RegistryHashes`
    uint256 startAmount;
    uint256 endAmount;
    address payable recipient;
}

/**
 * @dev A spent item is translated from a utilized offer item and has four
 *      components: an item type (ETH or other native tokens, ERC20, ERC721, and
 *      ERC1155), a token address, a tokenId, and an amount.
 */
struct SpentItem {
    ItemType itemType;
    address token; //@audit simple token id, not encoded like in `RegistryHashes`
    uint256 identifier;
    uint256 amount;
}

• Instance 2 : The verifyCreate function :

The same happens in the verifyCreate function where the decodeType function is given bytes32 casted identifier as input but this parameter is also a simple token id coming from the SpentItem struct when the ratifyOrder function is called :

function verifyCreate(
    address delegateToken,
    uint256 identifier,
    CreateOffererStructs.Receivers storage receivers,
    ReceivedItem calldata consideration,
    bytes calldata context
) internal view {
    IDelegateRegistry.DelegationType tokenType = RegistryHashes.decodeType(
        bytes32(identifier)
    );
    ...
}

function ratifyOrder(
    SpentItem[] calldata offer,
    ReceivedItem[] calldata consideration,
    bytes calldata context,
    bytes32[] calldata,
    uint256 contractNonce
)
    external
    checkStage(Enums.Stage.ratify, Enums.Stage.generate)
    onlySeaport(msg.sender)
    returns (bytes4)
{
    Helpers.processNonce(nonce, contractNonce);
    Helpers.verifyCreate(
        delegateToken,
        offer[0].identifier,
        transientState.receivers,
        consideration[0],
        context
    );
    return this.ratifyOrder.selector;
}

So in both instances mentioned above the decodeType function is not given the correct hash encoded by one of the hashing functions (allHash, contractHash, erc721Hash, erc20Hash, erc1155Hash) instead the input is a simple hash that will not contain the token type encoded in its last bytes and thus the decodeType function will not return the correct/expected token type in both instances.

This will cause problems in both functions as this issue can make the whole process of creating delegate token through CreateOfferer revert (DOS) because the decoded token type will be wrong.

Tools Used

Manual review

Recommended Mitigation Steps

The decodeType function must be given the correctly encoded hash as input in both CreateOffererHelpers.updateTransientState and CreateOffererHelpers.verifyCreate functions.

Assessed type

en/de-code

[0xfoobar]

We are doing a bit of stretching here to use the RegistryHashes lib to extract the last byte of a bytes32 even when the last byte isn't strictly a registry type but a seaport type, but should still function as desired

[c4-sponsor]

0xfoobar (sponsor) disputed

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof

[GalloDaSballo]

Have asked Warden for POC, will allow until end of Post-Judging QA

[thebrittfactor]

For transparency, the judge requested PoC from the warden has been added below:

As explained the decodeType function, expect a special hash encoded with one of the encoding functions in RegistryHashes (allHash, contractHash, erc721Hash, erc20Hash and erc1155Hash), those functions will add the correct type at the end of the hash (ALL_TYPE, CONTRACT_TYPE, ERC721_TYPE, ERC20_TYPE and ERC1155_TYPE).

But in the instances I mentioned in my submission the hash given as input to decodeType are simple token id returned from seaport (transformed into hashes using bytes32()), you can check that in the Seaport Docs here.

In my understanding, hashing a simple token id (uint going from 0 to uint256.max) into bytes32 will not give the same information that are encoded by the RegistryHashes library hashing functions (mentioned above), and thus in the mentioned instances, the returned hash will not contain the relevant type data in the last bytes that are needed in the logic.

[0xfoobar]

Seaport does allow arbitrary identifierOrCriteria parameters, but we plan to populate this as a (spoofed CreateOfferer) tokenId, not a hash. So should preserve the trailing byte that we submit. Is there a PoC or modified test case showing that this gets hashed incorrectly to break something? Our test suite is working fine across all types: https://github.com/delegatexyz/delegate-market/blob/main/test/CreateOfferer.t.sol

[GalloDaSballo]

bytes32 to uint256 doesn't cause a loss of data So to dispute this finding we just have to prove the integrity of the data

We also have empirical proof via the test suite that the code works as intended

[GalloDaSballo]

I don't believe I have sufficient information to assert that the finding is valid

I have had staff follow up with the Warden and asked them to Produce a Coded POC

When the Warden sent back a few lines, I had staff tell them that they should produce a Coded POC as a necessary requirement to validate finding

I have done my best effort to understand the finding and believe there is insufficient proof to validate it.

• Per the tests all Token Identifying data is provided to the contract

• This data is "shrunk" into 32bytes for the identified, which is address with one byte replaced with the Token Identifier

• Fulfilling of orders is not denied as demonstrated by the Tests as well as POCs I had written for judging other issues

• What may be at risk is the possibility of a clash

The clash would fail here due to incorrect create data

Due to this, I must close the finding as invalid due to lack of proof

This is an example of a finding complex enough where a POC is needed

I would recommend other wardens to consider this idea and follow up with the Sponsor