reNFT - Tendency's results

Lines of code

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

Vulnerability details

Impact

There are several instances in the codebase where the protocol implementation isn't eip712 compliant.

In this report, I will be tagging all of the observed quirks:

• 1. According to eip712, bytes and string should be the keccack256 of its content. As seen here

The dynamic values bytes and string are encoded as a keccak256 hash of their contents.

So, the Signer::_deriveHookHash function, defined here

    function _deriveHookHash(Hook memory hook) internal view returns (bytes32) {
        // Derive and return the hook as specified by EIP-712.
        return
            keccak256(
                abi.encode(_HOOK_TYPEHASH, hook.target, hook.itemIndex, hook.extraData)
            );
    }

Should be changed to:

    function _deriveHookHash(Hook memory hook) internal view returns (bytes32) {
        // Derive and return the hook as specified by EIP-712.
        return
            keccak256(
                abi.encode(_HOOK_TYPEHASH, hook.target, hook.itemIndex, keccak256(hook.extraData)) //<@
            );
    }

• 2. The hash returned at Signer::_deriveOrderMetadataHash excludes other metadata already defined in the _ORDER_METADATA_TYPEHASH

This:

    function _deriveOrderMetadataHash(
        OrderMetadata memory metadata
    ) internal view returns (bytes32) {
########
        return
            keccak256(
                abi.encode(
                    _ORDER_METADATA_TYPEHASH,
                    metadata.rentDuration,
                    keccak256(abi.encodePacked(hookHashes))
                )
            );
    }

should be changed to:

    function _deriveOrderMetadataHash(
        OrderMetadata memory metadata
    ) internal view returns (bytes32) {
########
        return
            keccak256(
                abi.encode(
                    _ORDER_METADATA_TYPEHASH,
                    metadata.orderType,
                    metadata.rentDuration,
                    keccak256(abi.encodePacked(hookHashes)),
                    keccak256(metadata.emittedExtraData)
                )
            );
    }

• 3. The wrong orderMetadataTypeHash is computed in Signer::_deriveRentalTypehashes function.

This is the struct whose type hash is to be defined:

struct OrderMetadata {
    // Type of order being created.
    OrderType orderType;
    // Duration of the rental in seconds.
    uint256 rentDuration;
    // Hooks that will act as middleware for the items in the order.
    Hook[] hooks;
    // Any extra data to be emitted upon order fulfillment.
    bytes emittedExtraData;
}

The EIP 712 guide on how to do this can be found here

orderMetadataTypeHash is the keccack256 of orderMetadataTypeString:

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

Referencing the eip, we can observe that, there is no definition for the Hook struct in the above string. The correct string should be:

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

With that fixed, rentPayloadTypeHash, which also uses the orderMetadataTypeString, should almost be okay, but there is still one issue.

According to the eip, here:

If the struct type references other struct types (and these in turn reference even more struct types), then the set of referenced struct types is collected, sorted by name and appended to the encoding.

the current definition:

            rentPayloadTypeHash = keccak256(
                abi.encodePacked(
                    rentPayloadTypeString,
                    orderMetadataTypeString,
                    orderFulfillmentTypeString
                )
            );

should be changed to:

            rentPayloadTypeHash = keccak256( 
                abi.encodePacked(
                    rentPayloadTypeString,
                    orderFulfillmentTypeString,
                    orderMetadataTypeString
                )
            );

this is because F comes before M when going down --> order

Proof of Concept

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

Tools Used

Manual Review & EIP712 specification

Recommended Mitigation Steps

Please observe the referenced fixes in the report

Assessed type

Other

[L - 01 ] - Safe's Fallback Handler Can be Changed

There are currently some checks in Guard that ensure certain delicate actions aren't allowed to be performed by owners of a safe, but the current implementation doesn't block calls that change the safe fallback handler. This could lead to compatibility issues and some other possible issues.

As discussed with a sponsor, this isn't an intended behaviour.

I have added a coded POC to show how this can be changed.

Add the below test to /test/integration/StopRent.t.sol, then import:

import {Enum} from "@safe-contracts/common/Enum.sol";

and then add this dummy contract:

contract DummyFallbackHandler {}

then run:

forge test -vvv --match-path test/integration/StopRent.t.sol --match-test test_ADDHANDLER

     function test_ADDHANDLER() public {
        //this will later be done in the setUp function
        DummyFallbackHandler handler = new DummyFallbackHandler();
        //we will be working with bob
        // speed up in time past the rental expiration
        // vm.warp(block.timestamp + 750);

        //get safe's current nonce
        uint256 nonce = alice.safe.nonce();
        bytes memory _calldata = abi.encodeWithSelector(
            bytes4(keccak256("setFallbackHandler(address)")),
            handler
        );
        // bytes memory _calldata = abi.encodeWithSelector(0x42966c68, 0);

        bytes32 dataHash = alice.safe.getTransactionHash(
            address(alice.safe),
            uint256(0),
            _calldata,
            Enum.Operation.Call,
            uint256(0),
            uint256(0),
            uint256(0),
            address(0),
            payable(0),
            nonce
        );

        (uint8 v, bytes32 r, bytes32 s) = vm.sign(alice.privateKey, dataHash);
        bytes memory signature = new bytes(65);
        assembly {
            mstore(add(signature, 32), r)
            mstore(add(signature, 64), s)
            mstore8(add(signature, 96), v)
        }

        alice.safe.execTransaction(
            address(alice.safe), //self call to the safe itself
            0,
            _calldata,
            Enum.Operation.Call, //Call
            0,
            0,
            0,
            address(0),
            payable(0),
            signature
        );
    }

Here are the logs:

[⠆] Compiling...
[⠘] Compiling 1 files with 0.8.22
[⠊] Solc 0.8.22 finished in 45.57sCompiler run successful!
[⠒] Solc 0.8.22 finished in 45.57s

Running 1 test for test/integration/StopRent.t.sol:TestStopRent
[PASS] test_ADDHANDLER() (gas: 143368)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 18.24ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

RECOMMENDATION

Add a check in the Guard::_checkTransaction function to block such requests