reNFT - AkshaySrivastav's results

Lines of code

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

Vulnerability details

Impact

The Guard policy in the smart contract is designed to permit the disabling of modules only if they are whitelisted.

However, the issue arises from the incorrect verification of input parameters in the Guard._checkTransaction function. Instead of checking the second parameter as it should, the function checks the first parameter of the disableModile call.

ISafe.sol

    function disableModule(address prevModule, address module) external;

Guard.sol

    uint256 constant gnosis_safe_disable_module_offset = 0x24;
    
    ...
    function _checkTransaction(address from, address to, bytes memory data) private view {
        bytes4 selector;

        // Load in the function selector.
        assembly {
            selector := mload(add(data, 0x20))
        }
        ...
        } else if (selector == gnosis_safe_disable_module_selector) {
            // Load the extension address from calldata.
            address extension = address(
                uint160(
                    uint256(
                        _loadValueFromCalldata(data, gnosis_safe_disable_module_offset)
                    )
                )
            );

            // Check if the extension is whitelisted.
            _revertNonWhitelistedExtension(extension);
        } else {
        ...
}

This means that in certain scenarios, the Stop policy, a critical security feature, can be unintentionally removed from a renter's safe wallet. This vulnerability poses a significant security risk, potentially leading to unauthorized changes and jeopardizing the safety of assets within the safe. Once the Stop policy gets removed from a Safe all ERC721 and ERC1155 can be stolen by renter.

Proof of Concept

The Guard::_checkTransaction ensures that any changes, particularly those involving the enabling or disabling of modules, are conducted securely and according to predefined rules. The vulnerability resides within the handling of the gnosis_safe_disable_module_selector in the function.

255 	} else if (selector == 	   gnosis_safe_disable_module_selector) {
256            // Load the extension address from calldata.
257            address extension = address(
258                uint160(
259                    uint256(
260                        _loadValueFromCalldata(data, gnosis_safe_disable_module_offset)
261                    )
262                )
263            );

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

In this segment, the function is intended to extract and verify the module address that is requested to be disabled.

However, the issue arises from the incorrect use of gnosis_safe_disable_module_offset. This offset is meant to point to the specific module intended for disabling, but due to the issue, it incorrectly references the first parameter instead of the second. This mismatch leads to the erroneous disabling of the wrong module, such as the Stop policy, rather than the intended target module.

The following foundry test shows the process described above. Copy and paste it into test folder to run.

<details> <summary> Click to expand Foundry Test </summary>

// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.20;

import {BaseTest} from "@test/BaseTest.sol";
import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol";
import {Errors} from "@src/libraries/Errors.sol";

contract DisableModuleTest is BaseTest {

    function test_exploit_disableModule() public {
        // Add a new extension to the protocol
        address EXTENSION = address(1001);
        vm.prank(deployer.addr);
        admin.toggleWhitelistExtension(EXTENSION, true);

        vm.startPrank(alice.addr);
        // A user enables this extension as a module in his safe wallet
        bytes memory transaction = abi.encodeWithSelector(
            alice.safe.enableModule.selector,
            EXTENSION
        );
        bytes memory transactionSignature = SafeUtils.signTransaction(
            address(alice.safe),
            alice.privateKey,
            address(alice.safe),
            transaction
        );
        SafeUtils.executeTransaction(
            address(alice.safe),
            address(alice.safe),
            transaction,
            transactionSignature
        );

        // Now both Stop policy and new extension are enabled as safe modules
        assertEq(alice.safe.isModuleEnabled(address(stop)), true);
        assertEq(alice.safe.isModuleEnabled(EXTENSION), true);

        // User removes Stop policy from Safe modules
        transaction = abi.encodeWithSelector(
            alice.safe.disableModule.selector,
            EXTENSION,
            address(stop)
        );
        transactionSignature = SafeUtils.signTransaction(
            address(alice.safe),
            alice.privateKey,
            address(alice.safe),
            transaction
        );
        SafeUtils.executeTransaction(
            address(alice.safe),
            address(alice.safe),
            transaction,
            transactionSignature
        );

        // Stop policy has been removed
        assertEq(alice.safe.isModuleEnabled(address(stop)), false);
        assertEq(alice.safe.isModuleEnabled(EXTENSION), true);


        // FURTHER SCENARIO -------------->


        // Incorrect behaviour, new extension cannot be removed
        transaction = abi.encodeWithSelector(
            alice.safe.disableModule.selector,
            address(1),
            EXTENSION
        );
        transactionSignature = SafeUtils.signTransaction(
            address(alice.safe),
            alice.privateKey,
            address(alice.safe),
            transaction
        );
        // Txn getting reverted with incorrect param
        vm.expectRevert(abi.encodeWithSelector(
            Errors.GuardPolicy_UnauthorizedExtension.selector,
            address(1)
        ));
        // vm.prank(alice.addr);
        SafeUtils.executeTransaction(
            address(alice.safe),
            address(alice.safe),
            transaction,
            transactionSignature
        );
    }
}

</details>

Tools Used

Foundry

Recommended Mitigation Steps

Consider checking the second parameter (module) of the disableModule call rather than the first (prevModule).

Assessed type

Context

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L182-L194 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L232-L238 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L394-L400

Vulnerability details

Impact

The ReNFT protocol leverages Seaport code for creating NFT Rentals. In the rental creation process the Lender and Rental has to encode, decode and sign rental information following the EIP712 standard.

The Signer contract is central to the process of encoding signed payloads and ensuring the correctness of encoded rental data. This contract must follow all the restrictions of EIP712 so that the typed structured data can be hashed and signed correctly by users.

However the contract violates EIP712 at three instances:

• omission of the rentalWallet parameter in _deriveRentalOrderHash
• exclusion of orderType and emittedExtraData in _deriveOrderMetadataHash
• incorrect field ordering in _deriveRentalTypehashes for rentPayloadTypeHash

These lead to the generation of incorrect hashes that do not accurately represent the data they are supposed to represent.

Incorrect hashes and incorrect signed data may result in the misrepresentation of rental orders to the users, and may even lead to validation failures.

Scenario Explanation

EIP-712 is a standard for securely hashing and signing data in smart contracts. It's important for contracts like Signer, which handles hashing of data in rental transactions, to follow this standard. EIP-712 makes sure that the data structure in these contracts is safely verifiable.

How Each Bug Deviates from EIP-712 Standard:

1. _deriveRentalOrderHash Function - Missing rentalWallet Parameter:

Standard Protocol: The standard needs all key parts of a data structure to be included in its hash.

Problem: Leaving out the rentalWallet parameter means the hash doesn't fully represent the rental order. This partial representation could lead to misunderstanding of the signed data.

    keccak256(
        abi.encode(
            _RENTAL_ORDER_TYPEHASH,
            order.seaportOrderHash,
            keccak256(abi.encodePacked(itemHashes)),
            keccak256(abi.encodePacked(hookHashes)),
            order.orderType,
            order.lender,
            order.renter,
            order.startTimestamp,
            order.endTimestamp
        )
    );

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L182-L194

2. _deriveOrderMetadataHash function - Omission of Parameters:

Standard Protocol: The EIP-712 standard requires a full representation of data in the hashing process.

Problem: Not including orderType and emittedExtraData goes against EIP-712's full data representation rule. This creates a hash that doesn't completely match the order's metadata, which could affect the accuracy of signature verification.

    keccak256(
        abi.encode(
            _ORDER_METADATA_TYPEHASH,
            metadata.rentDuration,
            keccak256(abi.encodePacked(hookHashes))
        )
    );

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L232-L238

3. _deriveRentalTypehashes Function - Alphabetical Sort Order Issue:

Standard Protocol: EIP-712 insists on sorting fields alphabetically for a consistent hash. EIP states that:

If the struct type references other struct types (and these in turn reference even more struct types), then the set of referenced struct types is collected, sorted by name and appended to the encoding. An example encoding is Transaction(Person from,Person to,Asset tx)Asset(address token,uint256 amount)Person(address wallet,string name).

Problem: Not sorting fields alphabetically in rentPayloadTypeHash breaks this rule. orderFulfillmentTypeString should be packed first followed by orderMetadataTypeString.

393            // Derive RentPayload type hash via combination of relevant type strings.
394            rentPayloadTypeHash = keccak256(
395                abi.encodePacked(
396                    rentPayloadTypeString,
397                    orderMetadataTypeString,
398                    orderFulfillmentTypeString
399                )
400            );

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

Not following EIP-712 closely in these aspects can impact the Signer contract's ability to correctly and securely handle transaction data. These issues need addressing to ensure the contract is fully compliant with EIP-712, which is key for the security and accuracy of its transactions.

Tools Used

Manual Review

Recommended Mitigation Steps

Adding the missing paramters and restore the alphabetical sort order at the instances mentioned above.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The Guard::updateHookStatus function is designed to allow protocol administrators to manage the status of hooks within the system. Hooks serve as essential components that customize and control flow of transactions within the protocol.

The vulnerability occurs when administrators disable the onStop hook using Guard::updateHookStatus. If the onStop hook gets disabled after a rental is created using that hook then that causes critical disruption in the rental closing flow.

This happens because the hook address is supplied by lender and renter at the time of rental creation, Both onStart and onStop gets executed on that user provided address at rental creation and stoppage respectively. Once a rental gets created its hook address cannot be modified. If the onStop hook gets disabled while the rental was active then the stopRent transaction will always revert.

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

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L210-L212

Proof of Concept

1. Suppose the onStart and onStop hooks are initially set as active by administrators for a particular contract address.
2. A user creates a rental transaction, and a whitelisted hook address is given as input.
3. The onStart hook gets executed as part of the rental creation process.
4. After some time, an administrator decides to disable the onStop hook for the same contract address.
5. As a result, users are unable to stop the rental, as the onStop hook will always revert any attempt to do so.

This sequence of events can result in situations where rentals cannot be stopped due to the disabling of critical hooks, causing potential issues and disruptions within the protocol.

The assets of lender and ERC20 payments will remain stuck in the renter's safe and escrow contract respectively.

Since this bug is dependent upon an admin interaction I am reporting it as medium.

The following foundry test shows the process described above. Add this test case in test/hooks/restricted-selector/RestrictedSelectorHook.t.sol file and run using command forge test --mp test/hooks/restricted-selector/RestrictedSelectorHook.t.sol.

<details> <summary> Click to expand Foundry Test </summary>

 function test_toggle_hook_status() public {
     // create a bitmap that will disable the `train()` function selector
     uint256 bitmap = 1;

     // Define the hook for the rental
     Hook[] memory hooks = new Hook[](1);
     hooks[0] = Hook({
         target: address(hook),
         itemIndex: 0,
         extraData: abi.encode(bitmap)
     });

     // start the rental with hook.
     RentalOrder memory rentalOrder = _startRentalWithGameToken(1, hooks);
     bytes32 rentalOrderHash = create.getRentalOrderHash(rentalOrder);
     assertEq(STORE.orders(rentalOrderHash), true);

     // Admin disables the onStop hook
     vm.prank(deployer.addr);
     guard.updateHookStatus(address(hook), uint8(3));    // 011

     // User tries to stop the rental
     // But txn always reverts
     vm.warp(block.timestamp + 500);
     vm.prank(alice.addr);
     vm.expectRevert(abi.encodeWithSelector(
         Errors.Shared_DisabledHook.selector,
         address(hook)
     ));
     stop.stopRent(rentalOrder);
     assertEq(STORE.orders(rentalOrderHash), true);
 }

</details>

Tools Used

Manual Review

Recommended Mitigation Steps

Consider rethinking about the need of onStop hook status because if onStart hook is executed for a rental then the onStop hook must also be executed irrespective of the hook status.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The Guard::checkTransaction function's current design unintentionally restricts native ETH transfers from the safe. This limitation originates from a data length check that requires transaction data to be at least 4 bytes long, aiming to validate the presence of a function selector.

However, since native ETH transfers have empty data fields, they fail this, leading to an inability to execute such transfers.

This results in a significant functional restriction, as users are unable to execute standard ETH transfers from their safes.

The Safe contracts contains receive function which by default enables them to receive native ETH. These native tokens can be transferred to safe by protocol users or current/future protocol hooks.

    receive() external payable {
        emit SafeReceived(msg.sender, msg.value);
    }

Proof of Concept

The Guard::checkTransaction is designed to validate transactions initiated by a rental safe. Its role is to ascertain whether a transaction meets the set criteria based on its destination, value, data and operation type. This function is a critical component in ensuring that transactions are executed in accordance with the protocol's rule.

A notable issue within the Guard::checkTransaction function is its requirement for transaction data to be at least 4 bytes long, as indicated by this code:

In src/policies/Guard.sol

328        // Require that a function selector exists.
329        if (data.length < 4) {
330            revert Errors.GuardPolicy_FunctionSelectorRequired();
331        }

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

This condition is ideal for transactions involving function calls where a function selector is necessary.

However, this becomes problematic for native ETH transfers, which are characteristically simple transactions without any data payload. As these transactions have a data length of zero bytes, failing the imposed condition.

Such transactions are automatically rejected by the Guard::checkTransaction function due to not meeting the 4-byte data length requirement.

The inability to transfer native ETH not only reduces the safe's practicality but also poses a severe limitation on user operations, affecting trust and reliability in the system.

The following foundry test shows the process described above. Copy and paste it into test folder to run.

<details> <summary> Click to expand Foundry Test </summary>

// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.20;

import {BaseTest} from "@test/BaseTest.sol";
import {SafeProxyFactory} from "@safe-contracts/proxies/SafeProxyFactory.sol";
import {SafeL2} from "@safe-contracts/SafeL2.sol";
import {ISafe} from "@src/interfaces/ISafe.sol";
import {Enum} from "@safe-contracts/common/Enum.sol";
import {Errors} from "@src/libraries/Errors.sol";

contract SafeStuckEth is BaseTest {

    // In this test case we'll see how ETH transfers works fine for normal Gnosis Safes
    // But doesn't work for ReNFT Safes
    function test_poc() public {
        vm.startPrank(alice.addr);

        // Create a new safe by directly interacting with Gnosis safe factory
        SafeProxyFactory safeFactory = factory.safeProxyFactory();
        SafeL2 safeSingleton = factory.safeSingleton();
        address[] memory owners = new address[](1);
        owners[0] = alice.addr;
        bytes memory initializerPayload = abi.encodeCall(
            ISafe.setup,
            (
                owners,
                1,
                address(0),
                new bytes(0),
                address(0),
                address(0),
                0,
                payable(address(0))
            )
        );
        // new safe is created
        address newSafe = address(
            safeFactory.createProxyWithNonce(
                address(safeSingleton),
                initializerPayload,
                uint256(keccak256(""))
            )
        );
        address receiver = address(0x1010101);

        // Transfer some ETH to the freshly deployed safe
        vm.deal(newSafe, 1 ether);
        assertEq(newSafe.balance, 1 ether);
        assertEq(receiver.balance, 0);

        // Pull out the sent ETH from the safe
        bytes memory transactionSignature = _signTransaction(
            address(newSafe),
            Enum.Operation.Call,
            alice.privateKey,
            address(receiver),
            1 ether,
            new bytes(0)
        );
        _executeTransaction(
            address(newSafe),
            Enum.Operation.Call,
            address(receiver),
            1 ether,
            new bytes(0),
            transactionSignature,
            new bytes(0)
        );
        // ETH succesfully pulled out from the safe
        assertEq(newSafe.balance, 0);
        assertEq(receiver.balance, 1 ether);

        /// Now replicate same scenario for a ReNFT safe

        // Send some ETH to an existing ReNFT safe of Alice
        vm.deal(address(alice.safe), 1 ether);
        assertEq(address(alice.safe).balance, 1 ether);
        assertEq(receiver.balance, 1 ether);

        transactionSignature = _signTransaction(
            address(alice.safe),
            Enum.Operation.Call,
            alice.privateKey,
            address(receiver),
            1 ether,
            new bytes(0)
        );
        // Try to pull out ETH
        // Txn reverts with error `GuardPolicy_FunctionSelectorRequired`
        vm.expectRevert(abi.encodeWithSelector(Errors.GuardPolicy_FunctionSelectorRequired.selector));
        _executeTransaction(
            address(alice.safe),
            Enum.Operation.Call,
            address(receiver),
            1 ether,
            new bytes(0),
            transactionSignature,
            new bytes(0)
        );
        // ETH remains stuck in Alice's ReNFT safe
        assertEq(address(alice.safe).balance, 1 ether);
        assertEq(receiver.balance, 1 ether);
    }

    function _signTransaction(
        address safe,
        Enum.Operation operation,
        uint256 ownerPrivateKey,
        address to,
        uint256 value,
        bytes memory transaction
    ) private view returns (bytes memory transactionSignature) {
        // get the safe nonce
        uint256 nonce = ISafe(safe).nonce();

        // get the eip712 compatible transaction hash that the safe owner will sign
        bytes32 transactionHash = ISafe(safe).getTransactionHash(
            to,
            value,
            transaction,
            operation,
            0 ether,
            0 ether,
            0 ether,
            address(0),
            payable(address(0)),
            nonce
        );

        // sign the transaction
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerPrivateKey, transactionHash);
        transactionSignature = abi.encodePacked(r, s, v);
    }
    function _executeTransaction(
        address safe,
        Enum.Operation operation,
        address to,
        uint256 value,
        bytes memory transaction,
        bytes memory signature,
        bytes memory expectedError
    ) private {
        // expect an error if error data was provided
        if (expectedError.length != 0) {
            vm.expectRevert(expectedError);
        }

        // execute the transaction
        ISafe(safe).execTransaction(
            to,
            value,
            transaction,
            operation,
            0 ether,
            0 ether,
            0 ether,
            address(0),
            payable(address(0)),
            signature
        );
    }
}

</details>

Tools Used

Foundry

Recommended Mitigation Steps

Consider allowing data.length to be 0 as that will be used for performing native ETH transfers.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The Admin contract allows administrators to toggle the whitelist status of an extension using the toggleWhitelistExtension function. While this functionality is intended to manage the whitelist status of extensions, a critical issue arises when users add an extension as a safe module.

The problem occurs when an admin whitelists an extension, a user adds this extension as a safe module, and then the admin removes the extension from the whitelist.

In this scenario, the user becomes unable to remove the extension from their safe modules, resulting in unexpected behavior.

Proof of Concept

The Admin::toggleWhitelistExtension function allows administrators to toggle the whitelist status of an extension. This functionality is essential for controlling which extensions can be added as safe modules by users.

The vulnerability occurs when the following sequence of actions take place:

1. An admin whitelists an extension using the Admin::toggleWhitelistExtension function.
2. A user adds the same extension as a safe module of their safe.
3. The admin decides to rmeove the extension from the whitelist using the Admin::toggleWhitelistExtension function.

At this point, even though the admin has successfully removed the extension from the whitelist, the user is unable to remove the same extension from their safe modules.

This results in an inconsistency in the management of safe modules. An obvious case for this situation to occur, but not limited to, is if a bug gets discovered in an existing whitelisted extension.

The following foundry test shows the process described above. Copy and paste it into test folder to run.

NOTE:

For this PoC to work the gnosis_safe_disable_module_offset value in src/libraries/RentalConstants.sol need to be updated to 0x44.

- uint256 constant gnosis_safe_disable_module_offset = 0x24;
+ uint256 constant gnosis_safe_disable_module_offset = 0x44;

<details> <summary> Click to expand Foundry Test </summary>

// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.20;

import {BaseTest} from "@test/BaseTest.sol";
import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol";
import {Errors} from "@src/libraries/Errors.sol";

contract WhitelistExtensionTest is BaseTest {
    function test_poc() public {
        // Add a new extension to the protocol
        address EXTENSION = address(1001);
        vm.prank(deployer.addr);
        admin.toggleWhitelistExtension(EXTENSION, true);

        vm.prank(alice.addr);
        // A user enables this extension as a module in his safe wallet
        bytes memory transaction = abi.encodeWithSelector(
            alice.safe.enableModule.selector,
            EXTENSION
        );
        bytes memory transactionSignature = SafeUtils.signTransaction(
            address(alice.safe),
            alice.privateKey,
            address(alice.safe),
            transaction
        );
        SafeUtils.executeTransaction(
            address(alice.safe),
            address(alice.safe),
            transaction,
            transactionSignature
        );

        // Now both Stop policy and new extension are enabled as safe modules
        assertEq(alice.safe.isModuleEnabled(address(stop)), true);
        assertEq(alice.safe.isModuleEnabled(EXTENSION), true);

        // Admin removes the extension
        vm.prank(deployer.addr);
        admin.toggleWhitelistExtension(EXTENSION, false);

        // User tries to remove the Extension from its Safe modules
        transaction = abi.encodeWithSelector(
            alice.safe.disableModule.selector,
            address(1),
            EXTENSION
        );
        transactionSignature = SafeUtils.signTransaction(
            address(alice.safe),
            alice.privateKey,
            address(alice.safe),
            transaction
        );
        // Txn getting reverted with incorrect param
        vm.expectRevert(abi.encodeWithSelector(
            Errors.GuardPolicy_UnauthorizedExtension.selector,
            EXTENSION
        ));
        SafeUtils.executeTransaction(
            address(alice.safe),
            address(alice.safe),
            transaction,
            transactionSignature
        );

        // Extension is still enabled
        assertEq(alice.safe.isModuleEnabled(EXTENSION), true);
    }
}

</details>

Tools Used

Manual Review

Recommended Mitigation Steps

One way of mitigating this issue is to add a separate whitelisted mapping of addresses which can only be used for Safe.disableModule call.

Assessed type

Context

[L-01] Lack of Bytecode validation in PaymentEscrow::_safeTransfer function

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

The PaymentEscrow::_safeTransfer function does not validate whether the token address provided contains bytecode or not. This lack of validation poses a potential risk, as the function assumes that the address is always a valid ERC20 token contract.

        // Call transfer() on the token.
        (bool success, bytes memory data) = token.call(
            abi.encodeWithSelector(IERC20.transfer.selector, to, value)
        );

Consider adding pre-condition check in the PaymentEscrow::_safeTransfer function to verify that the token address contains a bytecode.

[L-02] Potential Revert in PaymentEscrow._settlePaymentProRata due to zero token value transfer

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

The PaymentEscrow::_settlePaymentProRata function is designed to handle pro-rata payment settlements for PAY lend orders.

However, there is a potential edge case leading to unintended behavior. When a PAY lend order is terminated just before its scheduled end time, the internal function _calculatePaymentProRata may round a payment of 0.5 tokens in favour of the renter.

This rounding can result in the calculated token amount for the lender being zero. Consequently, attempting a token transfer of zero value can cause the function to revert, especially if the token contract does not allow zero-value transfers.

        // Calculate the pro-rata payment for renter and lender.
        (uint256 renterAmount, uint256 lenderAmount) = _calculatePaymentProRata(
            amount,
            elapsedTime,
            totalTime
        );

To address this issue, it is recommended to add a conditional check within the _settlePaymentProRata function to handle the case where the calculated token amount for the lender is zero.

[L-03] NFT Transfer Prior to State Update in Stop::stopRent Function

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L293C5-L302C11

In the Stop::stopRent function, there is an order of operations issue where the NFT transfer is executed before updating the protocol state. The _reclaimRentedItems(order) function, which handles the NFT transfer, is called prior to the STORE.removeRentals function that updates the protocol state.

This ordering breaks the widely used checks-effects-interactions pattern for smart contract development.

In src/policies/Stop.sol

293       _reclaimRentedItems(order);
294
295        // Interaction: Transfer ERC20 payments from the escrow contract to the respective recipients.
296        ESCRW.settlePayment(order);
297
298        // Interaction: Remove rentals from storage by computing the order hash.
299        STORE.removeRentals(
300            _deriveRentalOrderHash(order),
301            _convertToStatic(rentalAssetUpdates)
302        );

To address this issue, it is recommended to reorder the operations so that the protocol state is updated before executing the NFT transfer.

-        _reclaimRentedItems(order);

-        ESCRW.settlePayment(order);

        STORE.removeRentals(
            _deriveRentalOrderHash(order),
            _convertToStatic(rentalAssetUpdates)
        );

+        _reclaimRentedItems(order); // NFT Transfer

+        ESCRW.settlePayment(order);

[I-01] Unused import of OrderFulfillment in Create.sol

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

In the Create.sol contract, there is an import statement for OrderFulfillment from the RentalStructs.sol library. However, it is not utilized in any part of the contract's code.

It is advisable to remove the import statement for OrderFulfillment from the Create.sol contract. This action will enhance the gas optimization during deployment.

In src/policies/Create.sol file:

import {
    RentalOrder,
    RentPayload,
    SeaportPayload,
    Hook,
-   OrderFulfillment,
    OrderMetadata,
    OrderType,
    Item,
    ItemType,
    SettleTo,
    RentalId,
    RentalAssetUpdate
} from "@src/libraries/RentalStructs.sol";