reNFT - ABAIKUNANBAEV's results

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L162 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L218

Vulnerability details

Impact

In Signer.sol, there are some functions where constructor of EIP-712 typehash is not made properly when generating hash. This makes the signature verification process not work as expected resulting in functions reverts due to generation of different signatures and non-compliance to EIP-712

Proof of Concept

First, it's _deviateRentalOrderHash() that constructs the typehash like this:

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L248-262

  function _deriveRentalOrderHash(
        RentalOrder memory order
    ) internal view returns (bytes32) {
        // Create arrays for items and hooks.
        bytes32[] memory itemHashes = new bytes32[](order.items.length);
        bytes32[] memory hookHashes = new bytes32[](order.hooks.length);

        // Iterate over each item.
        for (uint256 i = 0; i < order.items.length; ++i) {
            // Hash the item.
            itemHashes[i] = _deriveItemHash(order.items[i]);
        }

        // Iterate over each hook.
        for (uint256 i = 0; i < order.hooks.length; ++i) {
            // Hash the hook.
            hookHashes[i] = _deriveHookHash(order.hooks[i]);
        }

        return
            keccak256(
                abi.encode(
                    _RENTAL_ORDER_TYPEHASH,
                    order.seaportOrderHash,
                    keccak256(abi.encodePacked(itemHashes)),
                    keccak256(abi.encodePacked(hookHashes)),
                    order.orderType,
                    order.lender,
                    order.renter,
                    order.startTimestamp,
                    order.endTimestamp
                )
            );
    }

However, the rentalOrderTypeString is declared like that:

   bytes memory rentalOrderTypeString = abi.encodePacked(
            "RentalOrder(bytes32 seaportOrderHash,Item[] items,Hook[] hooks,uint8 orderType,address lender,address renter,address rentalWallet,uint256 startTimestamp,uint256 endTimestamp)"
        );

The problem is that the rentalWallet param is missing in this case.

This problem is also present _deriveOrderMetadataHash() function where orderType and emittedExtraData params are missing:

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L218-239

 function _deriveOrderMetadataHash(
        OrderMetadata memory metadata
    ) internal view returns (bytes32) {
        // Create array for hooks.
        bytes32[] memory hookHashes = new bytes32[](metadata.hooks.length);

        // Iterate over each hook.
        for (uint256 i = 0; i < metadata.hooks.length; ++i) {
            // Hash the hook
            hookHashes[i] = _deriveHookHash(metadata.hooks[i]);
        }

        // Derive and return the metadata hash as specified by EIP-712.
        return
            keccak256(
                abi.encode(
                    _ORDER_METADATA_TYPEHASH,
                    metadata.rentDuration,
                    keccak256(abi.encodePacked(hookHashes))
                )
            );
    }

orderMetadataTypeString:

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L384-386

            bytes memory orderMetadataTypeString = abi.encodePacked(
                "OrderMetadata(uint8 orderType,uint256 rentDuration,Hook[] hooks,bytes emittedExtraData)"
            );

Tools Used

Manual review.

Recommended Mitigation Steps

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L248-262

             

keccak256(
                abi.encode(
                    _RENTAL_ORDER_TYPEHASH,
                    order.seaportOrderHash,
                    keccak256(abi.encodePacked(itemHashes)),
                    keccak256(abi.encodePacked(hookHashes)),
                    order.orderType,
                    order.lender,
                    order.renter,
+                   order.rentalWallet,
                    order.startTimestamp,
                    order.endTimestamp
                )
            );

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L218-239

 keccak256(
                abi.encode(
                    _ORDER_METADATA_TYPEHASH,
+                   metadata.orderType,
                    metadata.rentDuration,
                    keccak256(abi.encodePacked(hookHashes)),   
+                   metadata.emittedExtraData
  
));

Assessed type

Other

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L385 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L363

Vulnerability details

Impact

In Signer.sol, when creating typehashes in _deriveRentalTypehashes(), orderType parameter is assigned to an incorrect type in itemTypeString, rentalOrderTypeString and orderMetadataTypeString. This will make the contract non-compliant to EIP-712 and the functions that use EIP-712 signatures will effectively revert.

Proof of Concept

In _deriveRentalTypehashes(), itemTypeString is created this way:

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L352-354

 bytes memory itemTypeString = abi.encodePacked(
            "Item(uint8 itemType,uint8 settleTo,address token,uint256 amount,uint256 identifier)"
        );

However, itemType and settleTo are of type Item and SettleTo and not uint8.

For example, rentalPayloadTypeString is created correctly:

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L389-391

   bytes memory rentPayloadTypeString = abi.encodePacked(
                "RentPayload(OrderFulfillment fulfillment,OrderMetadata metadata,uint256 expiration,address intendedFulfiller)"
            );

It uses OrderFullfillment type for fulfillment param instead of uint8, unlike in the previous example.

The next strings are orderMetadataTypeString and rentalOrderTypeString where also uint8 type used instead of custom types for orderType parameter:

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L362-364

  bytes memory rentalOrderTypeString = abi.encodePacked(
            "RentalOrder(bytes32 seaportOrderHash,Item[] items,Hook[] hooks,uint8 orderType,address lender,address renter,address rentalWallet,uint256 startTimestamp,uint256 endTimestamp)"
        );

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L384-386

  bytes memory orderMetadataTypeString = abi.encodePacked(
                "OrderMetadata(uint8 orderType,uint256 rentDuration,Hook[] hooks,bytes emittedExtraData)"
            );

Tools Used

Manual review.

Recommended Mitigation Steps

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L362-364

 bytes memory rentalOrderTypeString = abi.encodePacked(
        
-     "RentalOrder(bytes32 seaportOrderHash,Item[] items,Hook[] hooks,uint8 orderType,address lender,address renter,address rentalWallet,uint256 startTimestamp,uint256 endTimestamp)"
                 
+     "RentalOrder(bytes32 seaportOrderHash,Item[] items,Hook[] hooks,OrderType orderType,address lender,address renter,address rentalWallet,uint256 startTimestamp,uint256 endTimestamp)"

        );        

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L384-386

bytes memory orderMetadataTypeString = abi.encodePacked(                
        
-     "OrderMetadata(uint8 orderType,uint256 rentDuration,Hook[] hooks,bytes emittedExtraData)"
            );   
               
+       "OrderMetadata(OrderType orderType,uint256 rentDuration,Hook[] hooks,bytes emittedExtraData)"
            );

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L352-354

bytes memory itemTypeString = abi.encodePacked(
        
-       "Item(uint8 itemType,uint8 settleTo,address token,uint256 amount,uint256 identifier)"
        );      
+       "Item(ItemType itemType,SettleTo settleTo,address token,uint256 amount,uint256 identifier)"
        );

Assessed type

Other