[WP-H1] Transactions can be replayed when a connectedChain is removed and then reconnected

[code423n4]

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/MessageProxy.sol#L313-L317

Vulnerability details

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/MessageProxy.sol#L313-L317

function removeConnectedChain(string memory schainName) public virtual override onlyChainConnector {
    bytes32 schainHash = keccak256(abi.encodePacked(schainName));
    require(connectedChains[schainHash].inited, "Chain is not initialized");
    delete connectedChains[schainHash];
}

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/MessageProxy.sol#L402-L409

function _addConnectedChain(bytes32 schainHash) internal onlyChainConnector {
    require(!connectedChains[schainHash].inited,"Chain is already connected");
    connectedChains[schainHash] = ConnectedChainInfo({
        incomingMessageCounter: 0,
        outgoingMessageCounter: 0,
        inited: true
    });
}

In the current implementation, when a connected chain is removed, the incomingMessageCounter and outgoingMessageCounter will be deleted.

And if it's reconnected again, the incomingMessageCounter and outgoingMessageCounter will be reset to 0.

However, since the contract is using connectedChains[fromChainHash].incomingMessageCounter and signature to ensure that the message can only be processed once.

Impact

1. Once the incomingMessageCounter resets to 0, all the past messages (transactions) can be replayed with the old signatures.

2. Another impact is that, for the particular reconnected schain, both inbound and outbound messages may not be able to be processed properly, as the counter is now out of sync with the remote schain.

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/MessageProxyForSchain.sol#L212-L221

require(
    _verifyMessages(
        _hashedArray(messages, startingCounter, fromChainName),
        signature
    ),
    "Signature is not verified"
);
require(
    startingCounter == connectedChains[fromChainHash].incomingMessageCounter,
    "Starting counter is not qual to incoming message counter");

Recommendation

Recommendation

When removing and connecting a schain, instead of delete/reset the couter, consider leaving the counter as it is when removeConnectedChain, _addConnectedChain also should not reset the counter:

function removeConnectedChain(string memory schainName) public virtual override onlyChainConnector {
    bytes32 schainHash = keccak256(abi.encodePacked(schainName));
    require(connectedChains[schainHash].inited, "Chain is not initialized");
    connectedChains[targetChainHash].inited = false;
}

function _addConnectedChain(bytes32 schainHash) internal onlyChainConnector {
    require(!connectedChains[schainHash].inited,"Chain is already connected");
    connectedChains[schainHash].inited = true;
}

[DimaStebaev]

Acknowledged, and work is on the roadmap.

[GalloDaSballo]

I believe the finding to have validity, in that, a set of signed messages can be replayed if the chain is disconnected and then re-connected while maintaining the same validators.

At this time I think Medium Severity (External Conditions Reliance) to be more appropriate and that the issue can be fully sidestepped by changing validators on a chain reconnect