reNFT - imare's results

Lines of code

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

Vulnerability details

Hooks represent added functionalities or restriction and are applied to the rented token inside the wallet. When a lender places a order and decides to use hooks. This hooks have three main callbacks one of them is Hook#onStop. If this callback fails the rented items are blocked inside the wallet. And event the GUARD_ADMIN cannot unstuck it.

Impact

Rented items can become stuck inside the lender wallet if a hook fails on the onStop call.

Proof of Concept

For a successfully created lend order the Hook#onStart is used without revert:

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L606-L612 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L496-L503

The lender can also interact with a hook trough the Hook#onTransaction when making transaction trough the assigned lender wallet.

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

But

when a lender or renter tries to stop the renting he/she calls the stop policy method stopRent and the hook is again called trough Hook#onStop

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

If this call fails it reverts and becouse the hooks are hashed with the lender order they cannot be removed in the order parameter when calling stopRent.

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L300 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L176-L179

If the revert on Hook#onStop happens the rented items become stuck inside the lender wallet.

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

Even the "GUARD_ADMIN" account which has some control over hooks has no available method to change the end address of the failing hook.

All of the above stands also when doing orders in batch mode.

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

Tools Used

Manual review

Recommended Mitigation Steps

Implement hooks address that are always only proxies so the implementation can be changed by a permissioned account or

When a hook fails in Hook#onStop implement a option for the lender to retrieve/settle the lent items.

Something like :

+           bytes32 hashedItem = _hashItem(item);

            try
                IHook(target).onStop(
                    rentalWallet,
                    item.token,
                    item.identifier,
                    item.amount,
                    hooks[i].extraData
                )
            {} catch Error(string memory revertReason) {
+               hookOnStopFailed[rentalWallet][hashedItem] = true;
                // Revert with reason given.
                revert Errors.Shared_HookFailString(revertReason);
            } catch Panic(uint256 errorCode) {
                // Convert solidity panic code to string.
                string memory stringErrorCode = LibString.toString(errorCode);

+               hookOnStopFailed[rentalWallet][hashedItem] = true;

                // Revert with panic code.
                revert Errors.Shared_HookFailString(
                    string.concat("Hook reverted: Panic code ", stringErrorCode)
                );
            } catch (bytes memory revertData) {
+               hookOnStopFailed[rentalWallet][hashedItem] = true;
                // Fallback to an error that returns the byte data.
                revert Errors.Shared_HookFailBytes(revertData);
            }
        }

If hookOnStopFailed[rentalWallet][hashedItem] becomes true give some other option to retrieve/settle the lent items.

Assessed type

Other