reNFT - ravikiranweb3's results

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#L166-L183

Vulnerability details

Impact

Stop::stopRent() function can be front run by a bad actor to block the user from claiming their deposited asset back. The assets could be locked permanently in the safe.

Proof of Concept

In the RentalOrder structure, "rentalWallet" field is not used for creating the rental order hash. This leaves an opportunity for a bad actor to pass a different rentalWallet value while calling the stopRent() ahead of the valid user by taking priority with higher gas.

The front runner can pass the below sub calls successfully in the stopRent() function.

a) _validateRentalCanBeStoped() b) removehooks() c) rentalAssetUpdates value update d) ESCRW.settlePayment(order); e) STORE.removeRentals( _deriveRentalOrderHash(order), _convertToStatic(rentalAssetUpdates) )

The only problem function is _reclaimRentedItems(). If the front running caller can by pass the _reclaimRentedItems(), function, he should be able to execute the attack successfully.

So to hack, the front running caller passes an attack contract that implements a dummy "execTransactionFromModule()" function and returns true. This will ensure that attacker will by pass the asset claiming process and will proceed to settlePayment() against the rentals and also remove the records for assets rented in the protocol from the storage.

This front running execution will succeed.

Subsequently, when the transaction of the real valid user is processed, it will revert as there are no assets for the rental order hash in the storage.[They were already deleted by the front run transaction]

Hence the user will never be able to claim their assets back and will loose the rented assets permanently.

Tools Used

Manual Review

Recommended Mitigation Steps

a) include the rentalWallet as part of rental order hash. b) if there are limitations, then map the rentalWallet to rental Order hash and keep it in mapping so that claiming can be done on the rentalWallet only.

This will prevent such bypass and hence prevent locking of rented assets permanently.

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

Signer::_deriveRentalOrderHash() function is relaying on the caller to pass the Order data in exact same order, else the order data would result in different Rental Order hash. This may create working problems later on.

Proof of Concept

Lets say a rent Order is being created as below at the time of creating the order.

        // Create the items
        Item[] memory items = new Item[](2);
        items[0] = Item({
            itemType: ItemType.ERC721,
            settleTo: SettleTo.LENDER,
            token: address(erc721s[0]),
            amount: 1,
            identifier: 0
        });
        items[1] = Item({
            itemType: ItemType.ERC721,
            settleTo: SettleTo.LENDER,
            token: address(erc721s[1]),
            amount: 1,
            identifier: 0
        });

        // Create a mock rental order
        RentalOrder memory order = RentalOrder({
            seaportOrderHash: keccak256(abi.encode("seaportOrderHash")),
            items: items,
            hooks: new Hook[](0),
            orderType: OrderType.BASE,
            lender: bob.addr,
            renter: alice.addr,
            rentalWallet: address(alice.safe),
            startTimestamp: 10,
            endTimestamp: 11
        });

But, when the stop::stopRent() function is called, the order of the items are ordered as below.

        Item[] memory items = new Item[](2);
        items[0] = Item({
            itemType: ItemType.ERC721,
            settleTo: SettleTo.LENDER,
    @audit->token: address(erc721s[1]),
            amount: 1,
            identifier: 0
        });
        items[1] = Item({
            itemType: ItemType.ERC721,
            settleTo: SettleTo.LENDER,
    @audit->token: address(erc721s[0]),
            amount: 1,
            identifier: 0
        });

While the underlying data is exactly same, since the elements are ordered differently in Items array, the resulting RentalOrderHash will be very different.

This will prevent the stopRent() function from executing the order until the caller know the exact order and submits the stopRent() in the same order as it was used during the creation of order.

This will function like DOS until the order is fixed.

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a sorting order for Items array and hooks array so that irrespective of how the caller passes the order of items, the data is rearranged on chain so that as long as underlying data is same, the RentalOrderHash generate is same and relieves the caller from responsibility of order the underlying data in certain order every time for function to work properly.

Assessed type

Other

1) Module.INIT is not implemented for both Storage and Escrow Modules.

2) PaymentEscrow::MODULE_PROXY_INSTANTIATION can be front run by a bad actor

3) Storage::MODULE_PROXY_INSTANTIATION can be front run by a bad actor

4) All their contracts are using solidity version 0.8.20. They want to deploy to polygon and avalanche which only support up to solidity version 0.8.19 due to a breaking change from the PUSH0 opcode.

Reference: https://www.zaryabs.com/push0-opcode/

Although PUSH0 opcode is now included with the latest solidity compiler, you need to be careful while using it on any other chain than ETH mainnet.

There are still other chains like L2s that don't recognize the PUSH0 opcode. Therefore, if you try to use the latest compiler to deploy your contract on any such chain that doesn't support this opcode, your contract deployment shall fail.```