Recovered messages can be replayed

[ishantiw]

Description

The message recovery functionality does not prevent recoverable messages from being replayed. This should be prevented by:

• In the execute function of the RecoverMessageCommand class: Updating the status of each recovered CCMs to CCMStatusCode.RECOVERED and updating the outboxRoot of the terminated outbox account with this new value.
• In the verify method of the RecoverMessageCommand class: Prevent CCMs with a status different from CCMStatusCode.OK from being processed.

The execute method updating the terminated account’s outbox root with the CCM with the updated state of CCMStatusCode.RECOVERED

https://github.com/LiskHQ/lisk-sdk/blob/89e7504ef5eb6183aefe576a93be3d6052e56038/framework/src/modules/interoperability/mainchain/commands/recover_message.ts#L217-L227

The verify method checking that a CCM trying to be recovered has a status ok CCMStatusCode.OK
https://github.com/LiskHQ/lisk-sdk/blob/89e7504ef5eb6183aefe576a93be3d6052e56038/framework/src/modules/interoperability/mainchain/commands/recover_message.ts#L127-L132

Two problems exist that prevent this security control from working. Firstly, the code appends to the recoveredCCMs array with crossChainMessage, which is the originally encoded CCM and not the CCM updated by the _applyRecovery and _forwardRecovery functions

Snippet showing that the incorrect value is appended to the recoveredCCM array. The append also happens in the else branch, which is incorrect.

https://github.com/LiskHQ/lisk-sdk/blob/89e7504ef5eb6183aefe576a93be3d6052e56038/framework/src/modules/interoperability/mainchain/commands/recover_message.ts#L187-L209

Secondly, the CCM is not actually updated in the _forwardRecovery and _applyRecovery functions; these functions solely create the recoveredCCM variable without updating the CCM stored in the context object

Snippet showing how context.ccm is not updated

https://github.com/LiskHQ/lisk-sdk/blob/89e7504ef5eb6183aefe576a93be3d6052e56038/framework/src/modules/interoperability/mainchain/commands/recover_message.ts#L232-L241

Both of these flaws independently cause the recoveredCCM array to have the original CCM, leaving the outboxRoot unaltered and allowing the CCM to be recovered repeatedly.
Furthermore, CCMs targeting the mainchain can never be recovered because the code that appends the CCM to the recoveredCCMs array executes only in the else block. This is incorrect and different from the LIP's pseudo-code, where the recoveredCCM is appended independently of the branch taken. This makes messages targeting the mainchain irrecoverable because the calculateRootFromUpdateData function will fail when the recoveredCCM array has a different length than the params.idxs array.

https://github.com/LiskHQ/lips/blob/main/proposals/lip-0054.md#execution-1

Recommendation
fix the immediate vulnerabilities by: modifying the _applyRecovery and _forwardRecovery functions to update the CCM in the context object, encoding and appending the updated CCM to the recoveredCCM array instead of appending the encoding of the old CCM, and appending the CCM outside the else branch. This will prevent recoverable CCMs from being replayed.

Improve testing to prevent issues such as these from being introduced or re-introduced. Currently, the use of jest.spyOn(regularMerkleTree, 'verifyDataBlock').mockReturnValue(true) makes it harder to unit test invariants such as: a CCM should only be recoverable once. Additionally to these unit tests, create a list of invariants that should always hold and test them in end to end tests on a running blockchain system with a mainchain and several sidechains. If possible, use fuzzing to test these invariants thoroughly.

Affected Version

v6.0.0-beta.2