Reentrancy in `MessageProxyForSchain` leads to replay attacks

[code423n4]

Lines of code

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

Vulnerability details

Impact

The postIncomingMessages function calls _callReceiverContract(fromChainHash, messages[i], startingCounter + 1) which gives control to a contract that is potentially attacker controlled before updating the incomingMessageCounter.

for (uint256 i = 0; i < messages.length; i++) {
    // @audit re-entrant, can submit same postIncomingMessages again
    _callReceiverContract(fromChainHash, messages[i], startingCounter + 1);
}
connectedChains[fromChainHash].incomingMessageCounter += messages.length;

The attacker can re-enter into the postIncomingMessages function and submit the same messages again, creating a replay attack. Note that the startingCounter is the way how messages are prevented from replay attacks here, there are no further nonces.

POC

Attacker can submit two cross-chain messages to be executed:

1. Transfer 1000 USDC
2. A call to their attacker-controlled contract, could be masked as a token contract that allows re-entrance on transfer.

Some node submits the postIncomingMessages(params) transaction, transfers 1000 USDC, then calls the attackers contract, who can themself call postIncomingMessages(params) again, receive 1000 USDC a second time, and stop the recursion.

Recommended Mitigation Steps

Add a messageInProgressLocker modifier to postIncomingMessages as was done in MessageProxyForMainnet.

[GalloDaSballo]

@cstrangedk It seems like the https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/bls/SkaleVerifier.sol#L45 is a view function (no state change), as such how would the verify function be able to prove that a signature wasn't reused in the same tx, and in the same block?

I believe that as long as no state change has happened (verify -> _callReceiverContract -> reEntrancy -> verify), you still would be able to us the same signature as messages.length (and no other storage variable) was changed.

What do you think?

[GalloDaSballo]

After a conversation with the sponsor we agreed that the codebase in the contest is slightly different from the one they have internally.

As such the reEntrancy is exploitable in this version of the code. Because one signature allows to replay the message multiple times, leading to draining of funds potentially, I agree that High Severity is appropriate

[GalloDaSballo]

The Sponsor has mitigated as of the start of the contest, however the code with the vulnerability made it's way into the repo in scope

[cstrangedk]

Resolved via https://github.com/skalenetwork/IMA/pull/1054