reNFT - evmboi32's results

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L379-L400 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L352-L375

Vulnerability details

Impact

The Signer package doesn't follow the EIP-712 standard, which will result in issues with integrators.

Proof of Concept

If we take a look at how rentalOrderTypeHash is calculated we can see that it is done incorrectly.

bytes memory itemTypeString = abi.encodePacked(
    "Item(uint8 itemType,uint8 settleTo,address token,uint256 amount,uint256 identifier)"
);

// Construct the Hook type string.
bytes memory hookTypeString = abi.encodePacked(
    "Hook(address target,uint256 itemIndex,bytes extraData)"
);

// Construct the RentalOrder type string.
bytes memory rentalOrderTypeString = abi.encodePacked(
    "RentalOrder(bytes32 seaportOrderHash,Item[] items,Hook[] hooks,uint8 orderType,address lender,address renter,address rentalWallet,uint256 startTimestamp,uint256 endTimestamp)"
);

rentalOrderTypeHash = keccak256(
    abi.encode(rentalOrderTypeString, hookTypeString, itemTypeString)
);

According to the "RentalOrder(bytes32 seaportOrderHash,Item[] items,Hook[] hooks,uint8 orderType,address lender,address renter,address rentalWallet,uint256 startTimestamp,uint256 endTimestamp)" the order of strings is set incorrectly when calculating the rentalOrderTypeHash

It should be instead as follows:

rentalOrderTypeHash = keccak256(
    abi.encode(rentalOrderTypeString, itemTypeString, hookTypeString)
);

The same problem arises when calculating the rentPayloadTypeHash. The orderMetadataTypeString and orderFulfillmentTypeString are switched. Furthermore the orderMetadataTypeString is incorrect as it doesn't add the Hook hash at the end.

bytes memory orderFulfillmentTypeString = abi.encodePacked(
    "OrderFulfillment(address recipient)"
);

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

// Construct the RentPayload type string.
bytes memory rentPayloadTypeString = abi.encodePacked(
    "RentPayload(OrderFulfillment fulfillment,OrderMetadata metadata,uint256 expiration,address intendedFulfiller)"
);

            
// Derive RentPayload type hash via combination of relevant type strings.
rentPayloadTypeHash = keccak256(
    abi.encodePacked(
        rentPayloadTypeString,
        orderMetadataTypeString,
        orderFulfillmentTypeString
    )
);

Tools Used

Manual Review

Recommended Mitigation Steps

Modify the hashes to follow the EIP-712 standard

rentalOrderTypeHash = keccak256(
    abi.encode(rentalOrderTypeString, itemTypeString, hookTypeString)
);

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

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

Assessed type

Other

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L280-L284 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L133-L135 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L124

Vulnerability details

Impact

In the case of ERC1155 tokens users can have their own (non-rented) tokens stuck in the safe for the duration of the rental.

Proof of Concept

The ERC1155 standard allows for multiple tokens with the same tokenId.

Consider this scenario:

• Lender wants to lend his ERC1155 token with tokenId = 0
• He creates a PAY order and decides to pay someone in order to play games with his token
• Renter sees this order and accepts it
• Doing that the ERC1155 token from the lender is sent to the safe that the renter owns.
• Now let's imagine that the game the renter is playing gives you multipliers, advantages, etc... if you have multiple of the same tokens (2 or more tokens with tokenId = 0 in this case).
• Renter obtains another ERC1155 token with tokenId = 0 from the same collection from opensea or another nft marketplace.
• He decides to send the token to the safe so he can earn double the rewards as he has two tokens with tokenId = 0
• This works fine, but the problem arises when the renter wants to transfer his token from the safe. Note that one token in the safe is a rented and should not be withdrawn until the rental is over, and the other token was manually transferred to the safe by the renter.
• Trying to withdraw his own token fails as the rental is active.
• The contract fails to check the amount of rented ERC1155 tokens with the same ID and reverts if there are> 0 rentals with that tokenId.
• If we take a look at the function below we can see that the isRentedOut reverts if there is a rented token with tokenId in the safe.

    function _revertSelectorOnActiveRental(
        bytes4 selector,
        address safe,
        address token,
        uint256 tokenId
    ) private view {
        // Check if the selector is allowed.
        if (STORE.isRentedOut(safe, token, tokenId)) {
            revert Errors.GuardPolicy_UnauthorizedSelector(selector);
        }
    }

• This will prevent the renter from withdrawing his own token while the original rental is active
• The renter should be able to withdraw the token that was manually transferred to the safe and was owned by him as this token is not rented.

Coded POC

Add this function, test, and imports to the StopRent.t.sol file and run with forge test --match-path ./test/integration/StopRent.t.sol -vvv

import {ISafe} from "@src/interfaces/ISafe.sol";
import {Enum} from "@safe-contracts/common/Enum.sol";
import {MockERC1155} from "@test/mocks/tokens/standard/MockERC1155.sol";
import {Errors} from "@src/libraries/Errors.sol";

// Helper function
function transferNFTOut(address nftToken, uint256 tokenId, bool _revertExpected) internal {
    ISafe safe = ISafe(address(bob.safe));

    bytes memory data = abi.encodeWithSelector(
        erc1155s[0].safeTransferFrom.selector, 
        address(bob.safe),
        address(bob.addr),
        tokenId,
        1,
        ""
    );

    bytes32 txHash = ISafe(safe).getTransactionHash(
        nftToken,
        0, // 0 value
        data,
        Enum.Operation.Call,
        0,
        0,
        0,
        address(0),
        payable(address(0)),
        0                // safe.nonce() its 0 since it is a first tx
    );

    // Bob (renter) signs the tx
    (uint8 v, bytes32 r, bytes32 s) = vm.sign(bob.privateKey, txHash);
    bytes memory signatures = abi.encodePacked(r, s, v);

    if (_revertExpected) {
        vm.expectRevert(
            abi.encodeWithSelector(
                Errors.GuardPolicy_UnauthorizedSelector.selector, 
                erc1155s[0].safeTransferFrom.selector
            )
        );
    }
    safe.execTransaction(nftToken, 0, data, Enum.Operation.Call, 0, 0, 0, address(0), payable(address(0)), signatures);
}

function testCannotTransferNonRentedNFTs() public {
    uint256 tokenId = 0;
    MockERC1155 nftToken = erc1155s[0];

    // Create a BASE order
    createOrder({
        offerer: alice,
        orderType: OrderType.BASE,
        erc721Offers: 0,
        erc1155Offers: 1,
        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
    });

    // Deal 1 token with tokenId of 0 to Bob (renter)
    dealERC1155(address(nftToken), bob.addr, tokenId, 1);

    // Finalize the base order fulfillment
    // Alice lends 100 ERC1155 tokens with tokenId of 0 to Bob (to his safe)
    RentalOrder memory rentalOrder = finalizeBaseOrderFulfillment();
    assertEq(STORE.isRentedOut(address(bob.safe), address(nftToken), tokenId), true);
    assertEq(nftToken.balanceOf(address(bob.safe), tokenId), 100);

    // Bob manually transfers his NFT to his safe
    vm.prank(bob.addr);
    nftToken.safeTransferFrom(bob.addr, address(bob.safe), 0, 1, "");

    // Assert that the balance increased
    assertEq(nftToken.balanceOf(address(bob.safe), tokenId), 101);

    // This call will now revert as the rental is active
    transferNFTOut(address(nftToken), tokenId, true);

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

    // Stop the rental order
    vm.prank(alice.addr);
    stop.stopRent(rentalOrder);

    // Assert that the token is no longer rented out in storage
    assertEq(STORE.isRentedOut(address(bob.safe), address(nftToken), tokenId), false);

    // Bob can now transfer his token out of his safe
    transferNFTOut(address(nftToken), tokenId, false);
}

[PASS] testCannotTransferNonRentedNFTs() (gas: 1970424)

Tools Used

Manual Review

Recommended Mitigation Steps

Since the safe prevents batch transfers, modifying the _revertSelectorOnActiveRental in such a way should be sufficient. This will now allow the renter to transfer the tokens he transferred to the safe during the time of an active rental (the rented token still cannot be transferred)

function _revertSelectorOnActiveRental(
    bytes4 selector,
    address safe,
    address token,
    uint256 tokenId
) private view {
    // Check if the selector is allowed.
    if (STORE.isRentedOut(safe, token, tokenId)) {
+       // Get rentalId
+       RentalId rentalId = RentalUtils.getItemPointer(safe, token, tokenId);
+       // More than one ERC1155 token with the same tokenId in the safe
+       if(STORE.rentedAssets(rentalId) > 1) {
+           // ERC1155
+           require(IERC1155(token).balanceOf(safe, tokenId) > STORE.rentedAssets(rentalId), "Guard: cannot transfer rented asset");
+           return;
+       }
        revert Errors.GuardPolicy_UnauthorizedSelector(selector);
    }
}

Assessed type

Invalid Validation

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L159-L162 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L169-L172 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L480-L482 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L210-L212 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L289 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L607

Vulnerability details

Impact

The checks for hooks are done incorrectly, which can cause the rental to not be stopped by anyone.

Proof of Concept

The hookStatus mapping should indicate if the hook in question is enabled for any of the three actions onTransaction, onStart, or onStop. If the hookStatus[hook] > 0 this indicates that the hook is whitelisted by the admin and should be invoked on actions that are specified inside the hookStatus[hook] accordingly.

Since only the admin can change the hookStatus using the updateHookStatus function, the value hookStatus[hook] > 0 would indicate that the hook is whitelisted by the protocol and can be specified as part of the orderMetadata. The contract should differentiate between whitelisted hooks and hook actions which it clearly does not.

If we take a look at the _addHooks function invoked during rental creation we can see that if the hook is whitelisted eg. hookStatus[hook] > 0 but the onStart is not enabled the rental creation will revert

function _addHooks(
    Hook[] memory hooks,
    SpentItem[] memory offerItems,
    address rentalWallet
) internal {
    // Define hook target, offer item index, and an offer item.
    address target;
    uint256 itemIndex;
    SpentItem memory offer;

    // Loop through each hook in the payload.
    for (uint256 i = 0; i < hooks.length; ++i) {
        // Get the hook's target address.
        target = hooks[i].target;

        ...
        if (!STORE.hookOnStart(target)) {          <=
            revert Errors.Shared_DisabledHook(target);
        }
        ...
    }
}

This doesn't seem to be the desired functionality, as the WhitelistedFulfillment.t.sol test file indicates that we can only have some of the hook functionality enabled.

guard.updateHookStatus(address(hook), uint8(2));

Let's consider the following scenario:

• Owner sets hookStatus[hookA] = 2 which indicates the hook should only be invoked onStart

function hookOnStart(address hook) external view returns (bool) {
    // 2 is 0x00000010. Determines if the masked bit is enabled.
    return uint8(2) & hookStatus[hook] != 0;
}

• Lender specifies a hookA as part of the rental metadata. The onStop and onTransaction functions will do nothing as they are not supported by the hook and the onStart will process data accordingly.
• Now rental is successfully created and the onStart function on the hook is invoked. Perfect
• Now let's try to stop the rental.
• When stopping a rental the _removeHooks function is invoked which implements similar logic to the _addHooks function. The problem here is that the function reverts if onStop is not activated (the owner only activated the onStart hook) for the hook (which is incorrect). Rental now cannot be stopped as this will always revert.

function _removeHooks(
    Hook[] calldata hooks,
    Item[] calldata rentalItems,
    address rentalWallet
) internal {
    // Define hook target, item index, and item.
    address target;
    uint256 itemIndex;
    Item memory item;

    // Loop through each hook in the payload.
    for (uint256 i = 0; i < hooks.length; ++i) {
        // Get the hook address.
        target = hooks[i].target;

        ...
        if (!STORE.hookOnStop(target)) {
            revert Errors.Shared_DisabledHook(target);
        }
        ...
    }
}

• hookStatus[hook] = 2 so this will fail

function hookOnStop(address hook) external view returns (bool) {
    // 4 is 0x00000100. Determines if the masked bit is enabled.
    return uint8(4) & hookStatus[hook] != 0;
}

Coded POC

Add the imports and the test to the StopRent.t.sol file and run with forge test --match-path ./test/integration/StopRent.t.sol -vvv.

import {
    WhitelistedFulfillmentHook
} from "@src/examples/whitelisted-fulfillment/WhitelistedFulfillmentHook.sol";
import {IHook} from "@src/interfaces/IHook.sol";
import { Hook } from "@src/libraries/RentalStructs.sol";

function testHookNotEnabledOnStop() public {
    WhitelistedFulfillmentHook hook;
    hook = new WhitelistedFulfillmentHook();

    // Owner sets the hookStatus to be only invoked onStart
    vm.prank(deployer.addr);
    guard.updateHookStatus(address(hook), uint8(2));

    // Assert that hook set up was successful
    assertEq(STORE.hookOnStart(address(hook)), true);
    assertEq(STORE.hookOnStop(address(hook)), false);
    assertEq(STORE.hookOnTransaction(address(hook)), false);

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

    address[] memory whitelist = new address[](2);
    whitelist[0] = address(bob.safe);
    whitelist[1] = address(carol.safe);

    Hook[] memory hooks = new Hook[](1);
    hooks[0] = Hook({
        // the hook contract to target
        target: address(hook),
        // index of the item in the order to apply the hook to
        itemIndex: 0,
        // any extra data that the hook will need.
        extraData: abi.encode(whitelist)
    });

    // Use an amendment to add hooks
    withHooks(hooks);

    // 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();
    assertEq(STORE.isRentedOut(address(bob.safe), address(erc1155s[0]), 0), true);

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

    // Stop the rental order
    vm.expectRevert();
    vm.prank(alice.addr);
    // Rental fails as hook is not enabled on stop
    stop.stopRent(rentalOrder);
}

[PASS] testHookNotEnabledOnStop() (gas: 2279036)

Tools Used

Manual Review

Recommended Mitigation Steps

The solution here is to differentiate between the whitelisted hooks and the actions the hook should be invoked on.

Refactor the logic of _addHooks and _removeHooks as follows. This will check first if STORE.hookStatus(target) > 0 which means it has been whitelisted by the protocol admin, and then it checks if it should be invoked or not. If it should not be invoked it skips to the next hook otherwise the invocation happens. In the case of a non whitelisted hook, it reverts as intended.

function _addHooks(
    Hook[] memory hooks,
    SpentItem[] memory offerItems,
    address rentalWallet
) internal {
    ...
    // Loop through each hook in the payload.
    for (uint256 i = 0; i < hooks.length; ++i) {
       ...

        // Check that the hook is reNFT-approved to execute on rental start.
-       if (!STORE.hookOnStart(target)) {
-           revert Errors.Shared_DisabledHook(target);
-       }

        // Get the offer item index for this hook.
        itemIndex = hooks[i].itemIndex;

+       if(STORE.hookStatus(target) > 0){
+           if(!STORE.hookOnStart(target)){
+               continue;
+           }
+       } else {
+           revert Errors.Shared_DisabledHook(target);
+       }

        ...
    }
}

function _removeHooks(
    Hook[] calldata hooks,
    Item[] calldata rentalItems,
    address rentalWallet
) internal {
    ...
    // Loop through each hook in the payload.
    for (uint256 i = 0; i < hooks.length; ++i) {
        ...

        // Check that the hook is reNFT-approved to execute on rental stop.
-       if (!STORE.hookOnStop(target)) {
-           revert Errors.Shared_DisabledHook(target);
-       }

        // Get the rental item index for this hook.
        itemIndex = hooks[i].itemIndex;
        
+       if(STORE.hookStatus(target) > 0){
+           if(!STORE.hookOnStop(target)){
+               continue;
+           }
+       } else {
+           revert Errors.Shared_DisabledHook(target);
+       }

        ...
    }
}

The line itemIndex = hooks[i].itemIndex; has to be placed before the fix otherwise a lender could accidentally send an incorrect index such that hooks[i].itemIndex > offerItems.length and pass the _addHooks function if onStart is disabled. The function would just continue as no hooks are enabled on start. But while trying to stop a rental there would be a problem because an item on that index would not be a rental if the onStop is enabled.

Assessed type

Invalid Validation

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L420-L423 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/Kernel.sol#L285 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/Kernel.sol#L383-L411 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/Kernel.sol#L548

Vulnerability details

Impact

Funds can be stuck on ESCRW if the module gets upgraded.

Proof of Concept

There are two ways to upgrade an ESCRW module.

1.) A permissioned actor calls upgrade on PaymentEscrow which will swap the implementation contract with a new one and storage will stay the same

function upgrade(address newImplementation) external onlyByProxy permissioned {
    // _upgrade is implemented in the Proxiable contract.
    _upgrade(newImplementation);
}

2.) The executor calls executeAction on the Kernel contract and executes the upgradeModule function. This will change the address of the ESCRW module and reconfigure the Stop policy as it is dependent on the ESCRW contract. This way the funds on the ESCRW contract are stuck as the settlePayment invoked during stopRent will call to a new contract.

function _upgradeModule(Module newModule_) internal {
    // Get the keycode of the new module
    Keycode keycode = newModule_.KEYCODE();

    // Get the address of the old module
    Module oldModule = getModuleForKeycode[keycode];

    // Check that the old module contract exists, and that the old module
    // address is not the same as the new module
    if (address(oldModule) == address(0) || oldModule == newModule_) {
        revert Errors.Kernel_InvalidModuleUpgrade(keycode);
    }

    // The old module no longer points to the keycode.
    getKeycodeForModule[oldModule] = Keycode.wrap(bytes5(0));

    // The new module points to the keycode.
    getKeycodeForModule[newModule_] = keycode;

    // The keycode points to the new module.
    getModuleForKeycode[keycode] = newModule_;

    // Initialize the new module contract.
    newModule_.INIT();

    // Reconfigure policies so that all policies that depended on the old
    // module will refetch the new module address from the kernel.
    _reconfigurePolicies(keycode);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Prevent upgrading the escrow module. If this is planned to be done in the future make sure that users can claim the funds from the old escrow contract in such a case.

Assessed type

Upgradable

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L195

Vulnerability details

Impact

The burn selector is missing inside the _checkTransaction. This allows renters to be able to burn the nfts in the safe. This will cause the lenders to have their nfts maliciously burned.

Proof of Concept

When a renter wants to execute the transaction from the safe that he owns the _checkTransaction on the Guard is invoked. The purpose of this guard is to prevent users from sending or approving the rented tokens from the safe.

However, the burn selector is not accounted for, which allows renters to maliciously burn the nft. In the case of a BASE order, this will result in the loss of the tokens (as they are stuck on the ESCRW) for the renter as well as the loss of the nft for the lender. Order cannot be stopped as nft transfer will fail because it was burnt. In case of a PAY order, this will cause a loss of tokens and the NFTs for the lender and 0 damage to the renter, so a malicious renter can intentionally burn the tokens.

Add the test and imports to the Rent.t.sol and run with forge test --match-path ./test/integration/Rent.t.sol -vvv

import {ISafe} from "@src/interfaces/ISafe.sol";
import {Enum} from "@safe-contracts/common/Enum.sol";
import {MockERC721} from "@test/mocks/tokens/standard/MockERC721.sol";
import {IERC721Errors} from "@openzeppelin-contracts/interfaces/draft-IERC6093.sol";

function testBurnNFTFromSafe() public {
    // Token to rent
    MockERC721 nftToken = erc721s[0];
    // TokenId to rent
    uint256 tokenId = 0;

    // 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);

    // 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(nftToken), tokenId), true);

    // Assert that the ERC721 is in the rental wallet of the fulfiller
    assertEq(nftToken.ownerOf(tokenId), address(bob.safe));

    ISafe safe = ISafe(address(bob.safe));
    // Avoid stack too deep error
    {
        address to = address(nftToken);
        uint256 value = 0;
        bytes memory data = abi.encodeWithSelector(nftToken.burn.selector, tokenId);
        Enum.Operation operation = Enum.Operation.Call;
        uint256 safeTxGas = 0;
        uint256 baseGas = 0;
        uint256 gasPrice = 0;
        address gasToken = address(0);
        address payable refundReceiver = payable(address(0));

        bytes32 txHash = ISafe(safe).getTransactionHash(
            to,
            value,
            data,
            operation,
            safeTxGas,
            baseGas,
            gasPrice,
            gasToken,
            refundReceiver,
            0                // safe.nonce() its 0 since it is a first tx
        );

        // Bob (renter) signs the tx
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(bob.privateKey, txHash);
        bytes memory signatures = abi.encodePacked(r, s, v);
        
        // This will burn the lent token(s) which is(are) inside the safe
        safe.execTransaction(to, value, data, operation, safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, signatures);
    }
    
    vm.expectRevert(abi.encodeWithSelector(IERC721Errors.ERC721NonexistentToken.selector, 0));
    nftToken.ownerOf(tokenId);
}

[PASS] testBurnNFTFromSafe() (gas: 1671213)

Tools Used

Manual Review

Recommended Mitigation Steps

Check for the burn selector and revert on active rental.

Assessed type

Invalid Validation

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L76-L87 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalStructs.sol#L154-L159 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalStructs.sol#L67-L70 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalStructs.sol#L50-L59 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L757-L768

Vulnerability details

Impact

Signatures from CREATE_SIGNER can be reused for sniping orders.

Proof of Concept

The _validateFulfiller function describes that the order sniping should not be allowed

It tries to do that by verifying the intendedFulfiller which prevents anyone else from fulfilling the order (other than intendedFulfiller) and using the signature.

/**
 * @dev Validates that the expected fulfiller of the order is the same as the address
 *      executed the order. This check is meant to prevent order sniping where one
 *      party receives a server-side signature but another party intercepts the
 *      signature and uses it.
 *
 * @param intendedFulfiller Address that was expected to execute the order.
 * @param actualFulfiller   Address that actually executed the order.
 */
function _validateFulfiller(
    address intendedFulfiller,
    address actualFulfiller
) internal pure {
    // Check actual fulfiller against the intended fulfiller.
    if (intendedFulfiller != actualFulfiller) {
        revert Errors.SignerPackage_UnauthorizedFulfiller(
            actualFulfiller,
            intendedFulfiller
        );
    }
    }

However, sniping could still be possible. First of all, we should understand that the signature is a signed rentPayload data. Let's look at the data

struct RentPayload {
    OrderFulfillment fulfillment;
    OrderMetadata metadata;
    uint256 expiration;
    address intendedFulfiller;
}

struct OrderFulfillment {
    // Rental wallet address.
    address recipient;
}

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;
}

From the above data we see that there are a few key variables we need to take a look at:

• fulfillment.recipient => a safe that gets the nft and has to be owned by intendedFulfiller
• expiration => when the signature expires and can be set as something far into the future eg. block.timestamp + 100 days
• OrderMetadata struct doesn't uniquely describe the order. The orderType is either BASE, PAY, or PAYEE so this is the same across most of the orders. The rentDuration is arbitrary but will most likely be enforced by the frontend to be something that makes sense eg. user could choose between a few options 1 day, 14 days, 1 month, etc. so it can be predictable (or just look on-chain which rentDurations are used most commonly)
• hooks and emittedExtraData could be empty. (this can be again looked up on chain to see which hooks the nfts from the collection you want to snipe use)
• as we can see the RentPayload struct is not dependent on any specific nft just a few pieces of data from the order that can be reused.
• How would this allow anyone to snipe orders? Let's have a look.
• The attacker (Alice) could lend herself worthless nfts to obtain signatures of different variations of RentPayload (the metadata will change, and other variables remain the same across different variations). Let's say she wants to borrow Bored Apes from legit users but the duration is unknown.
• She can start renting herself any nft (which can be deployed by her as the nft address is not a part of the RentPayload data) across many orders and just change the rentDuration (and possibly hooks and emittedExtraData if needed) each time. This way she will obtain signatures for different variations of the rentDuration. (note that she is the intendedFulfiller and the fulfillment.recipient safe is owned by her so this signature will only work for her which is perfect)
• Now that she holds signatures, she can just wait for a good offer to come by (she will have the signature for the rental creation ready beforehand if the rentDuration, hooks and emittedExtraData matches to the signatures that she created before while lending herself some nfts) and snipe it, since she already possesses signature from the CREATE_SIGNER

Tools Used

Manual Review

Recommended Mitigation Steps

Add a nonce to RentPayload struct to make it unique and prevent reusing signatures.

Assessed type

Invalid Validation

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L166-L183 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L95 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L99 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L32-L50

Vulnerability details

Impact

If the lender is a smart contract it could intentionally or accidentally revert on the onERC721Received or onERC1155Received callback while stopping the order.

Proof of Concept

Consider the following scenario

• Alice (lender) decides to lend a token to Bob (renter) using a PAY order.

• This way Alice pays Bob for playing games and earning rewards for Alice

• Now let's assume that the NFT price crashes while the rental is active.

• Alice gets angry as her NFT is now almost worthless

• Now there are two scenarios

  1.) Alice or Bob stops the rental after expiry. Bob will receive ERC20 tokens as payment and Alice will receive the now worthless nfts back.

  2.) Bob tries to stop the rental after expiry but Alice will revert (onERC721Received or onERC1155Received) while trying to transfer the now worthless NFT from the vault to her address. This way Bob doesn't get paid and Alice doesn't get the NFT back, which is now worthless anyway.

• If Alice becomes selfish in such a scenario and chooses the described option 2, she can deny the payment to Bob because her NFT lost value. The thinking of Alice would be "If i lost money so should you"

function reclaimRentalOrder(RentalOrder calldata rentalOrder) external {
   ...
    // Get a count for the number of items.
    uint256 itemCount = rentalOrder.items.length;

    // 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);
    }
}

function _transferERC721(Item memory item, address recipient) private {
    IERC721(item.token).safeTransferFrom(address(this), recipient, item.identifier);
}

function _transferERC1155(Item memory item, address recipient) private {
    IERC1155(item.token).safeTransferFrom(
        address(this),
        recipient,
        item.identifier,
        item.amount,
        ""
    );
}

Tools Used

Manual Review

Recommended Mitigation Steps

If the NFT transfer reverts consider sending it to the escrow contract for Alice to reclaim it herself.

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/modules/PaymentEscrow.sol#L257-L264 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L271-L277 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L100-L117

Vulnerability details

Impact

If the recipient of the ERC20 payment is blacklisted an order cannot be stopped. This will cause a loss for lenders and renters.

Proof of Concept

When calling stopRent the settlePayment on the ESCRW is invoked.

function stopRent(RentalOrder calldata order) external {
    ...
    ESCRW.settlePayment(order);
    ...
}

The settlePayment tries to transfer the ERC20 payment to the appropriate party. If the receiver gets blacklisted during the rental, the rental cannot be stopped. Both lender and renter could lose funds or assets in such cases.

function _settlePayment(
    Item[] calldata items,
    OrderType orderType,
    address lender,
    address renter,
    uint256 start,
    uint256 end
) internal {
    ...
    for (uint256 i = 0; i < items.length; ++i) {
        // Get the item.
        Item memory item = items[i];

        // Check that the item is a payment.
        if (item.isERC20()) {
            ...
            if (orderType.isPayOrder() && !isRentalOver) {
                // Interaction: a PAY order which hasnt ended yet. Payout is pro-rata.
                _settlePaymentProRata(        <=
                    item.token,
                    paymentAmount,
                    lender,
                    renter,
                    elapsedTime,
                    totalTime
                );
            }
            // If its a PAY order and the rental is over, or, if its a BASE order.
            else if (
                (orderType.isPayOrder() && isRentalOver) || orderType.isBaseOrder()
            ) {
                // Interaction: a pay order or base order which has ended. Payout is in full.
                _settlePaymentInFull(        <=
                    item.token,
                    paymentAmount,
                    item.settleTo,
                    lender,
                    renter
                );
            } else {
                revert Errors.Shared_OrderTypeNotSupported(uint8(orderType));
            }
        }
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Consider transferring the ERC20 payment to an escrow contract in case of a failure. The appropriate party can then manually withdraw the tokens from the escrow contract.

Assessed type

Token-Transfer