Due to lack of validation, a malicious node can **repeatedly** prevent a destination node from redeeming their ticket

[hats-bug-reporter[bot]]

Github username: @0xmuxyz Submission hash (on-chain): 0xd055eede9cbee2e98e4826d67c20e86e3b77d7c9c3a3dbaccf31dfefa9784099 Severity: medium

Description:

Title

Due to lack of validation, a malicious node can repeatedly prevent a destination node from redeeming their ticket

Description

Within the HoprNodeSafeRegistry contract, the NodeSafeRecord struct would be defined like this:\ https://github.com/hats-finance/SafeStaking-by-HOPR-0x607386df18b663cf5ee9b879fbc1f32466ad5a85/blob/master/packages/ethereum/contracts/src/node-stake/NodeSafeRegistry.sol#L75-L78

    // Structure to store the mapping between nodes and their associated Safe contracts
    struct NodeSafeRecord {
        address safeAddress;
        uint96 nodeSigNonce;
    }

Within the HoprNodeSafeRegistry contract, the _nodeToSafe storage would be defined to store the NodeSafeRecord struct data that a given node address is associated with a Safe like this:\ https://github.com/hats-finance/SafeStaking-by-HOPR-0x607386df18b663cf5ee9b879fbc1f32466ad5a85/blob/master/packages/ethereum/contracts/src/node-stake/NodeSafeRegistry.sol#L91

    mapping(address => NodeSafeRecord) _nodeToSafe;

Within the HoprNodeSafeRegistry#registerSafeWithNodeSig(), the HoprNodeSafeRegistry#addNodeSafe() would be called with a given node (nodeChainKeyAddress) and a given Safe (safeAddress) in order to associate each other like this:\ https://github.com/hats-finance/SafeStaking-by-HOPR-0x607386df18b663cf5ee9b879fbc1f32466ad5a85/blob/master/packages/ethereum/contracts/src/node-stake/NodeSafeRegistry.sol#L169

    /**
     * @dev Register the Safe with a signature from the node.
     * This function can be called by any party.
     * @param safeAddress Address of safe
     * @param nodeChainKeyAddress Address of node
     * @param sig The signature provided by the node.
     */
    function registerSafeWithNodeSig(address safeAddress, address nodeChainKeyAddress, bytes calldata sig) external {
        ...
        // store those state, emit events etc.
        addNodeSafe(safeAddress, nodeChainKeyAddress);  ///<--------- @audit
    }

Within the the HoprNodeSafeRegistry#addNodeSafe(), a given node (nodeChainKeyAddress) and a given Safe (safeAddress) would be associated by storing them into the _nodeToSafe storage (record) like this:\ https://github.com/hats-finance/SafeStaking-by-HOPR-0x607386df18b663cf5ee9b879fbc1f32466ad5a85/blob/master/packages/ethereum/contracts/src/node-stake/NodeSafeRegistry.sol#L250-L254

    /**
     * @dev Internal function to store a node-safe pair and emit relevant events.
     * @param safeAddress Address of safe
     * @param nodeChainKeyAddress Address of node
     */
    function addNodeSafe(address safeAddress, address nodeChainKeyAddress) internal {
        ...
        NodeSafeRecord storage record = _nodeToSafe[nodeChainKeyAddress];

        // update record
        record.safeAddress = safeAddress;
        record.nodeSigNonce = record.nodeSigNonce + 1; // as of Solidity 0.8, this reverts on overflows
        ...

After a Node would be registered with a Safe above, the association between a Node and a Safe can be checked by calling the HoprNodeSafeRegistry#nodeToSafe() like this:\ https://github.com/hats-finance/SafeStaking-by-HOPR-0x607386df18b663cf5ee9b879fbc1f32466ad5a85/blob/master/packages/ethereum/contracts/src/node-stake/NodeSafeRegistry.sol#L114-L116

    /**
     * @dev Returns the Safe address associated with a specific node address.
     * @param nodeAddress The address of the Hopr node.
     * @return safeAddress The associated Safe address.
     */
    function nodeToSafe(address nodeAddress) external view returns (address) {
        return _nodeToSafe[nodeAddress].safeAddress;
    }

Within the HoprMultiSig contract, the noSafeSet() modifier would be defined to the following items like this:

• whether or not it would be initialized.

• whether or not registry.nodeToSafe(msg.sender) != address(0).\ https://github.com/hats-finance/SafeStaking-by-HOPR-0x607386df18b663cf5ee9b879fbc1f32466ad5a85/blob/master/packages/ethereum/contracts/src/MultiSig.sol#L72

  /**
   * Only permits operation if no Safe contract has been specified
   * in NodeSafeRegistry
   */
  modifier noSafeSet() {
      if (!initialized) { 
          revert MultiSigUninitialized();
      }
  
      if (registry.nodeToSafe(msg.sender) != address(0)) {   ///<------------- @audit 
          revert ContractNotResponsible();
      }
      _;
  }

When a node close an existing channel, the node call the Channels#closeIncomingChannel(). Within the Channels#closeIncomingChannel(), the node (msg.sender) would be checked via the HoprMultiSig#noSafeSet() modifier. Then, the Channels#_closeIncomingChannelInternal() would be called like this:\ https://github.com/hats-finance/SafeStaking-by-HOPR-0x607386df18b663cf5ee9b879fbc1f32466ad5a85/blob/master/packages/ethereum/contracts/src/Channels.sol#L503-L504

    /**
     * See `_closeIncomingChannelInternal`
     */
    function closeIncomingChannel(address source) external HoprMultiSig.noSafeSet() { ///<------------- @audit
        _closeIncomingChannelInternal(msg.sender, source);  ///<------------- @audit
    }

Within the Channels#_closeIncomingChannelInternal(), the ChannelStatus.CLOSED would be stored into the channel.status. And then, if there is remaining balance of token (balance > 0), the token would be transferred to the source node address like this:\ https://github.com/hats-finance/SafeStaking-by-HOPR-0x607386df18b663cf5ee9b879fbc1f32466ad5a85/blob/master/packages/ethereum/contracts/src/Channels.sol#L534 \ https://github.com/hats-finance/SafeStaking-by-HOPR-0x607386df18b663cf5ee9b879fbc1f32466ad5a85/blob/master/packages/ethereum/contracts/src/Channels.sol#L544-L545

    /**
     * Closes an incoming channel.
     *
     * This can happen immediately since it is up to the caller to
     * redeem their collected tickets.
     *
     * @param source source end of the channel to close
     */
    function _closeIncomingChannelInternal(address self, address source) internal {
        ...
        Channel storage channel = channels[channelId];
        ...
        uint256 balance = Balance.unwrap(channel.balance);

        channel.status = ChannelStatus.CLOSED; // ChannelStatus.CLOSED == 0 ///<------------- @audit
        channel.closureTime = Timestamp.wrap(0);
        channel.ticketIndex = TicketIndex.wrap(0);
        channel.balance = Balance.wrap(0);
        ...

        if (balance > 0) {
            if (token.transfer(source, balance) != true) { ///<------------- @audit
                revert TokenTransferFailed();
            }
        }
    }

Based on the HOPR protocol design, the Channels#closeIncomingChannel() is supposed to be called by the only source node, who deposit funds by calling the Channels#fundChannel(). However, within the HoprMultiSig#noSafeSet() modifier used on the Channels#closeIncomingChannel(), there is only validation to check whether or not the registry.nodeToSafe(msg.sender) is not address(0) like this:\ MultiSig.sol#L72

        if (registry.nodeToSafe(msg.sender) != address(0)) {   ///<------------- @audit 
            revert ContractNotResponsible();
        }

In other words, the validation above can be passed if registry.nodeToSafe(msg.sender) == address(0).

As long as a node does not register via the HoprNodeSafeRegistry#registerSafeWithNodeSig(), the return value of the registry.nodeToSafe(msg.sender) would always be address(0).

This means that any node address, who does not register via the HoprNodeSafeRegistry#registerSafeWithNodeSig(), can pass the validation above in the HoprMultiSig#noSafeSet() modifier.

Since the HoprMultiSig#noSafeSet() modifier can be passed if registry.nodeToSafe(msg.sender) == address(0), the malicious node, who does not register via the HoprNodeSafeRegistry#registerSafeWithNodeSig(), can pass the HoprMultiSig#noSafeSet() modifier on the Channels#closeIncomingChannel(). And therefore, the malicious node can freely call the Channels#closeIncomingChannel() to close an existing Channel.

If a malicious node observe a source node deposit funds to open a channel via the Channels#fundChannel(), the malicious node can close the channel by calling the Channels#closeIncomingChannel() just after the channel would be opened. This is problematic because a destination node does not have enough time to redeem their ticket if the channel is immediately closed just after the channel is opened.

More future, a malicious node can do same attack again and again. Thus, in the worst scenario, if a malicious node would observe many transaction of the Channels#fundChannel(), many destination node can not redeem their ticket via the Channels#redeemTicket() due to that a malicious node prevent these destination nodes from redeeming their ticket. This lead to break a HOPR protocol design (Payment Channel mechanism).

Attack scenario

Let's say:

• The node A is a source node.
• The node B is a destination node.
• The node C is a malicious node.
  • On the assumption that, node C is not registered via the HoprNodeSafeRegistry#registerSafeWithNodeSig(). And therefore, the node C can pass the check of the HoprMultiSig#noSafeSet() modifier.

Attack scenario: 1/ The node A deposit their funds by calling the Channels#fundChannel() to open the channel. After that, the channel status would be "Open" state. 2/ The node C would observe that the Channels#fundChannel() is called by the node A (when the step 1/ above), the node C would immediately close the Channel by calling the closeIncomingChannel() just after the node A deposited their funds and the channel was opened. 3/ The node B would like to redeem their ticket by calling the redeemTicket(). But, the node B can not redeem it due to that the channel state is already "Close" state and there was no enough time for the node B to redeem their ticket.

More further, a malicious node (like a node C) can do same attack again and again to prevent another destination nodes (like a node B) from redeeming their ticket.

In the worst scenario, if a malicious node would observe many transaction of the Channels#fundChannel(), many destination node can not redeem their ticket via the Channels#redeemTicket() due to that a malicious node prevent these destination nodes from redeeming their ticket. This lead to break a HOPR protocol design (Payment Channel mechanism).

Impact

This lead to break a protocol design (Payment Channel mechanism).

Recommendation

Within the Channels#closeIncomingChannel(), consider adding a validation in order to check whether or not the caller would be a source node, who deposited their funds via the Channels#fundChannel() to open the channel like this:

    function closeIncomingChannel(address source) external HoprMultiSig.noSafeSet() {
+       require(msg.sender == DEPOSITOR_SOURCE_NODE, "The caller must be the source node, who deposited their funds via the Channels#`fundChannel()` to open the channel");
        _closeIncomingChannelInternal(msg.sender, source);
    }

(NOTE：Although it has not implemented yet, the DEPOSITOR_SOURCE_NODE above would be a source node address, who deposited their funds via the Channels#fundChannel().)

[QYuQianchen]

Two comments:

1. Based on the HOPR protocol design, the Channels#closeIncomingChannel() is supposed to be called by the only destination node.
2. A channel is associated with its source and destination. A third party node cannot influence on others' channel