Manual observation of a THORChain coin with MsgObservedTxIn could leave the Pool Module silently insolvent
Thinking on potential consequences of mis misuses of !3548 (merged) '[command] Manual In/Out Observation Commands',
I considered a situation where one node started a fictional observation with a --raw
observation,
then mixed in that fictional TxID with a group of other real TxIDs for Active nodes to manually observe,
relying on sufficient nodes for the 2/3rds threshold not checking each TxID one by one.
(The context would hypothetically be one in which nodes were used to semi-regularly manually observing some TxIDs.)
This is similarly relevant if a starting observation innocently (rather than than maliciously) contained an error.
Conveniently, if someone were to fake an inbound in this way and it were to reach observation consensus, the solvency checker would halt the chain (through MsgSolvency messages) and immediately draw attention to that something was strange.
Something I noticed, however: MsgDeposits are for THORChain coins only, and MsgObservedTxIns are for non-THORChain coins only,
but though MsgDeposit ValidateBasic
has
https://gitlab.com/thorchain/thornode/-/blob/v1.132.0/x/thorchain/types/msg_deposit.go#L31-33
for _, coin := range m.Coins {
if !coin.IsNative() {
return cosmos.ErrUnknownRequest("all coins must be native to THORChain")
there is no such validation check for that a MsgObservedTxIn's coins are not THORChain Coins.
Relatedly, as observable in the BNB-chain pools' Ragnarok, the Pool Module can become insolvent (broken invariant) without the chain being halted or anything happening to draw attention to this until someone happens to take a look and notice.
(#1919 (closed) , !3486 (merged) , !3495 (merged))
( From that time, with no chain halt, https://thornode-v1.ninerealms.com/thorchain/invariant/asgard?height=15207120 . )
The matter of the consequences of Pool Module insolvency could be addressed with an automatic halt or with a security event emission or with /thorchain/invariant/asgard
monitoring
(noting thorchain/devops/node-launcher!1095 (merged) )
, but at this time what I specifically suggest is the addition of a check in validate
of handler_observed_txin.go
for if any coin satisfies coin.IsNative()
.
https://gitlab.com/thorchain/thornode/-/blob/v1.132.0/common/coin.go#L84-87
// IsNative check whether the coin is native on THORChain
func (c Coin) IsNative() bool {
return c.Asset.GetChain().Equals(THORChain)
}
As evidence that this could actually happen I offer this regression test:
804c4fad
https://gitlab.com/thorchain/thornode/-/jobs/6840739259
This, with a job showing that the regression test passes,
I believe shows that such a supermajority-observed THOR.RUNE inbound could result in a swap-output outbound and leave the Pool Module insolvent, not necessarily noticed until after an outbound had already been signed
(which, if malicious rather than accidentally, could perhaps be a large enough outbound or outbounds to offset any bonded RUNE the node held in THORChain?).
If the coin were a Synth or Derived Asset, the consequence could be a different sort of broken invariant (also depending on the memo).
Again, I note that this would require 2/3rds of all Active nodes to manually observe a TxID of a THORChain coin inbound,
which I hope in practice would be noticed without reaching supermajority,
but which I would instead prefer be rejected at the validate
stage.
[Edit: Or, perhaps better, in the handle
stage with a continue
to not block unrelated valid ObservedTx observations in the same message.]
Specifically: !3557
'Do not act on MsgObservedTxIn observations of native coins'.