reNFT - ZdravkoHr's results

Lines of code

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

Vulnerability details

Impact

When users create rents, they can specify whitelisted hook contracts to be executed for each asset on creation and on stop. Currently, if a hook's stop functionality becomes inactive, all rents that have this hook will not be able to be stopped.

Proof of Concept

Alice creates a rent order with 5 items and lists 5 different hooks for each item. Time passes and the rent duration ends. Alice (or some other party) calls Stop.stopRent. Because there are hooks present, _removeHooks_ will be called.

        if (order.hooks.length > 0) {
            _removeHooks(order.hooks, order.items, order.rentalWallet);
        }

_removeHooks runs a loop iteration for each hook of the order and calls that hook's onStop function, but only if the hook's stop functionality is currently active.

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

This means that if the hook is currently disabled, the whole transaction will be reverted and stopping the rental will not be possible. And even worse, if the hook's onStop function reverts because of a bug in the hook contract, the rent will be stuck forever, even if it gets activated again.

Tools Used

Manual Review

Recommended Mitigation Steps

A hook has three events that it reacts to:

• start
• transaction
• stop

When the start is disabled, the protocol prohibits creating orders with that hook. When the transaction is disabled, the protocol fallbacks to tx verification in Guard.sol. When the stop is disabled, the hook's code should just not be executed.

So instead of reverting if the stop functionality of a hook is inactive, use continue to skip the current loop iteration.

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

Assessed type

DoS

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L373-L375 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L379-L400 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L231-L238

Vulnerability details

Impact

There are multiple mistakes in Signer.sol because of which the protocol is not EIP-712 compliant. When users sign their data with their wallet, a signed message is generated. That signed message will be then validated on chain to prove that the user did indeed give their permission to execute the action. Breaking the rules of EIP-712 will make this process impossible. Since everything in the project is based on signed messages, nothing will work.

Proof of Concept

The problems which cause EIP-712 non-compliance can be divided in two categories:

• incorrect type strings;
• incorrect implementation of the hashStruct function from the standard

I hard a hard time deciding whether to split this report into 2 separate ones, but decided to submit all of them at once.

First, what is wrong with the type string? According to the definition of encodeType, when a struct references other structs, they have to be appended to the end of the whole type string, sorted alphabetically. As we can see in Signer.sol, that is not the case. Type string do not include referenced structs.

As we continue to analyze the code, it becomes clear here that the idea is to append the hook type string and item type string at the end of the rental order type string (which references them) and only then calculate the rental order type hash.

  rentalOrderTypeHash = keccak256(
     abi.encode(rentalOrderTypeString, hookTypeString, itemTypeString)
  );

The problem is that abi.encode() is used instead of abi.encodePacked(). Encoding two strings with abi.encode() is not the same as concatenating them because the bytes will be padded. That's a violation of the standard.

Next, let's look at how the rentPayloadTypeHash is calculated.

This time, abi.encodePacked() is correctly used to append the corresponding type strings.

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

However, there are still 2 mistakes. The first one is that the strings are not sorted alphabetically (F comes before M, so orderFulfillmentTypeString should come before orderMetadataTypeString). The second one is that the OrderMetadata struct references the Hook struct, but it is again missing from its type string.

  bytes memory orderMetadataTypeString = abi.encodePacked(
      "OrderMetadata(uint8 orderType,uint256 rentDuration,Hook[] hooks,bytes emittedExtraData)"
  );

The last issue is that the type hash for OrderMetadata does not hash all of its fields. OrderMetadata consists of 4 fields.

struct OrderMetadata {
    OrderType orderType;
    uint256 rentDuration;
    Hook[] hooks;
    bytes emittedExtraData;
}

In the type hash calculation, orderType and emittedExtraData are left out.

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

There is a discrepancy now between the type hash and the type string, because the type string expects that all fields will be hashed.

bytes memory orderMetadataTypeString = abi.encodePacked(
    "OrderMetadata(uint8 orderType,uint256 rentDuration,Hook[] hooks,bytes emittedExtraData)"
 );

This will again result in inability to verify the EIP-712 signed message. Even worse, if the type string did not include the two fields, a false verification would have been possible where a renter passes whatever orderType and emittedExtraData they want.

Tools Used

Foundry, Remix IDE, EIP-712 Demo

Recommended Mitigation Steps

1. Appending strings with abi.encode and abi.encodePacked is dangerous. Instead, go over the whole file and change all type string to include all referenced structs.

2. Hash all fields of the OrderMetadata struct here

Assessed type

Error

Judge has assessed an item in Issue #297 as 2 risk. The relevant finding follows:

L6

[LOW-1] PUSH0 is not supported on some chains

The protocol will be deployed on all chains that are supported by Safe. This includes chains like Polygon where the PUSH0 opcode is not supported. The current pragma of the contracts ^0.8.20 allows compiling with 0.8.20 which uses PUSH0. If the contracts are compiled with this compiler, they will not work on these chains after deployment.

[LOW-2] _calculateFee rounds in favour of users, and not of the protocol

The Escrow Module keeps the ERC20 tokens during an active rental that have to be paid to a certain party. When the rent is stopped, the tokens are transferred to the recipient and a small fee is taken for the protocol. The issue is that the protocol does not round in its own favour but in users' in _calculateFee.

    function _calculateFee(uint256 amount) internal view returns (uint256) {
        // Uses 10,000 as a denominator for the fee.
        return (amount * fee) / 10000;
    }

Since any ERC20 tokens are in scope of this audit, if the ERC20 is with little decimals, for example 3, the loss will be of importance.

For instance: 120234 * 1000 / 10000

120.234 tokens with fee of 10%. The fee taken should be 12.0234, but it will be 12.023, which is a loss of 0.0004 tokens for this example.

My recommendation is to round in your favour:

    function _calculateFee(uint256 amount) internal view returns (uint256) {
        // Uses 10,000 as a denominator for the fee.
  -      return (amount * fee) / 10000;
  +      return ((amount * fee) + 5000) / 10000);
    }

[LOW-3] A risk of transferring locked NFTs if the Stop Policy is enabled as a whitelisted delegate address

The Stop Policy will be enable as a Safe Module, then safes will delegate call to the policy which inherits from Reclaimer.sol and that policy will call reclaimRentalOrder. If the stop policy becomes an enabled delegate address, everyone will be able to call reclaimRentalOrder and move the locked NFTs.

[LOW-4] The contracts may stop working if a reorg happens

If a reorg happens, the block.chainId will be changed. In Signer.sol the CHAIN_ID is set in the constructor and cannot be changed afterwards. In the case of reorg, there will be an incongruity between block.chainId and _CHAIN_ID and the whole Signer will not work. I recommend making _CHAIN_ID a storage variable (currently immutable) and adding a function that updates it to block.chainid

[LOW-5] Allowed rental execution on deadline

Signer._validateProtocolSignatureExpiration() allows executing rentals exactly on the deadline, when they should be already expired. Change that

   function _validateProtocolSignatureExpiration(uint256 expiration) internal view {
        // Check that the signature provided by the protocol signer has not expired.
        // @audit allows execution at expiration
-        if (block.timestamp > expiration) {
+        if (block.timestamp >= expiration) {
            revert Errors.SignerPackage_SignatureExpired(block.timestamp, expiration);
        }
    }

[LOW-5] Users will not be able to deactivate their safe extensions if they have been removed from the whitelist

In case where the protocol decides to remove a given extension from the whitelist, safe users will not be able to remove that extension because it checks if it is whitelisted and reverts otherwise

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. 
            // @audit will revert if not whitelisted anymore
            _revertNonWhitelistedExtension(extension);
        }