reNFT - jasonxiale's results

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L235-L243 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L126-L136 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L118-L128

Vulnerability details

Impact

Guard._checkTransaction is used to prevent transactions that involve transferring an ERC721 or ERC1155. In current implementation, if the renter borrows an ERC721/ERC1155 token, Guard._checkTransaction makes sure that the token can't be transferred. The issue is that ERC1155, one token ID may represent more than a NFT token, in such case. If a renter already owns some ERC1155 for tokenId, and he borrows some tokenId from the system, then until the rential expires, he can't transfer his own ERC1155.

Proof of Concept

In Guard._checkTransaction is called when rent safe wants to transfer some token, within Guard._checkTransaction, Guard._revertSelectorOnActiveRental is used to check ERC1155

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

And STORE.isRentedOut is defined as:

    function isRentedOut(
        address recipient,
        address token,
        uint256 identifier
    ) external view returns (bool) {
        // calculate the rental ID
        RentalId rentalId = RentalUtils.getItemPointer(recipient, token, identifier);

        // Determine if there is a positive amount
        return rentedAssets[rentalId] != 0;
    }

So it means that if rentedAssets[rentalId] is larger than 0, the transfer for the token will be revert, this is Ok for ERC721, because for ERC721, one tokenId means one NFT, but for ERC1155, this will prevent the renter from transferring his own ERC1155 token

Suppose Alice has some ERC1155 for 0x1234 tokenId, Bob list his 0x1234 token on the market and Alice borrows Bob's token. Until Alice repay Bob's rential order, Alice can't transfer her own ERC1155.

Tools Used

VIM

Recommended Mitigation Steps

Assessed type

Other

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L218-L239 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L339-L409 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L180-L194

Vulnerability details

Impact

Signer._deriveRentalTypehashes and Signer._deriveOrderMetadataHash don't follow EIP-712

Proof of Concept

1. Function Signer._deriveOrderMetadataHash is supposed to use struct OrderMetadata to calcluate hash.

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

But when calculating hash in Signer.sol#L231-L238, orderType and emittedExtraData members are missing

218     function _deriveOrderMetadataHash(
219         OrderMetadata memory metadata
220     ) internal view returns (bytes32) {
    ...
229 
230         // Derive and return the metadata hash as specified by EIP-712.
231         return
232             keccak256(
233                 abi.encode(
234                     _ORDER_METADATA_TYPEHASH,
235                     metadata.rentDuration,
236                     keccak256(abi.encodePacked(hookHashes))
237                 )
238             );
239     }

2. In Signer._deriveRentalTypehashes, while calcluating rentPayloadTypeHash, the order of param of abi.encodePacked is abi.encodePacked(rentPayloadTypeString, orderMetadataTypeString, orderFulfillmentTypeString), this is incorrect. Quoting from EIP-712

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).

So the order should be sorted by name, but the code doesn't sort the struct by names

339     function _deriveRentalTypehashes()
340         internal
341         pure
342         returns (
343             bytes32 itemTypeHash,
344             bytes32 hookTypeHash,
345             bytes32 rentalOrderTypeHash,
346             bytes32 orderFulfillmentTypeHash,
347             bytes32 orderMetadataTypeHash,
348             bytes32 rentPayloadTypeHash
349         )
350     {
    ...
387 
388             // Construct the RentPayload type string.
389             bytes memory rentPayloadTypeString = abi.encodePacked(
390                 "RentPayload(OrderFulfillment fulfillment,OrderMetadata metadata,uint256 expiration,address intendedFulfiller)"
391             );
392 
393             // Derive RentPayload type hash via combination of relevant type strings.
394             rentPayloadTypeHash = keccak256(
395                 abi.encodePacked( <<<---- Here the order of abi.encodePacked is not correct
396                     rentPayloadTypeString,
397                     orderMetadataTypeString,
398                     orderFulfillmentTypeString
399                 )
400             );
401 
    ...
407         }
408     }

Tools Used

VIM

Recommended Mitigation Steps

Assessed type

Other

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L265-L306 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L313-L364

Vulnerability details

Impact

Stop.stopRent and Stop.stopRentBatch are used to terminated a rental, and in this function ERC721/ERC1155 will be returned to lender's address, and the payment(ERC20) will be sent to lender/renter depending on the rental type. If the renter's safe or lender's safe is balcklisted for the borrowed ERC721/ERC1155 or payment ERC20, Stop.stopRent/stopRentBatch will be reverted, in such case, the borrowed assets will be stuck in rental's safe.

Proof of Concept

I'll take Stop.stopRent as an exampel: In Stop.stopRent, ESCRW.settlePayment is used to transfer ERC20 payments from the escrow contract to the respective recipients.

In PaymentEscrow._settlePayment will finally calls PaymentEscrow._safeTransfer to transfer the token PaymentEscrow._safeTransfer is defined as:

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

        // Because both reverting and returning false are allowed by the ERC20 standard
        // to indicate a failed transfer, we must handle both cases.
        //
        // If success is false, the ERC20 contract reverted.
        //
        // If success is true, we must check if return data was provided. If no return
        // data is provided, then no revert occurred. But, if return data is provided,
        // then it must be decoded into a bool which will indicate the success of the
        // transfer.
        if (!success || (data.length != 0 && !abi.decode(data, (bool)))) {
            revert Errors.PaymentEscrowModule_PaymentTransferFailed(token, to, value);
        }
    }

In PaymentEscrow._safeTransfer, transfer is used to send token to recipients.

If ERC20 tokens(like USDC with blacklist) are used in the order, when from or to in in blacklist, the transfer tx will be reverted. In such case, Stop.stopRent will be reverted.

1. Suppose Alice creates a PAY orders, in the rental order, she submit a ERC721 and a ERC20, the ERC20 is USDC( or other ERC20 with blacklist).
2. Bob sees the order, and he fulfills the rental order
3. Then after sometime, if Bob doesn't want to return Alice's ERC721 back to Alice, he can get his safe address been blacklisted by the USDC.
4. In such case, when Alice tries to call Stop.stopRent to get her ERC721 back, during ESCRW.settlePayment(order);, the function will be reverted in PaymentEscrow._safeTransfer because Bob's address is blacklisted.

Tools Used

VIM

Recommended Mitigation Steps

Assessed type

Other

Create._processPayOrderOffer and Create._processPayeeOrderConsideration can be bypassed partially

File: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L276-L286 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L218-L220 In _processBaseOrderOffer only check offers.length shoulde be larger than 0, if the offer type is ERC1155, the function doesn't check if ERC1155's amount is larger than 0. If the offers in Create._processBaseOrderOffer are all ERC115 type, and 0 as amount, it acts like offers.length == 0, and should revert Similar issue happens in Create._processPayOrderOffer too.

if Stop.stopRent or stop.stopRentBatch is not called, after the rental duration, the renter can continue use the lender's token for free

File: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L313-L365 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L265-L306 In current implementation, after the rential duration, the ERC721/ERC1155 will be owned by the renter if Stop.stopRent/stopRentBatch is not called, this might be an issue because the renter can use the assets for free