Consolidate (Internal) amounts can be subtracted before being added; for Consolidates, zero flooring (apparent insolvency)
Summary of the summary:
Consolidate transactions have FromAddress and ToAddress identical, and so their Coins (unlike Gas) should not modify vault balances,
yet currently a Consolidate transaction with FromAddress equal to ToAddress
subtracts (and separately adds) the entire wallet balance from the THORNode vault balance,
something which Should Not Be.
Since the THORNode vault balance can be temporarily less than the wallet balance,
this can result in a Consolidate transaction flooring a vault balance being floored to zero
(from SafeSub
)
and then restoring it to a higher value than it should have.
(Instead, I rather strongly feel Coins for which FromAddress and ToAddress are the same should not modify vault balances.)
Summary of the below:
When an observed transaction's FromAddress and ToAddress are the same,
its Coins does not change the wallet balance (though its Gas does)
and so its Coins should not change the vault balance.
However, Consolidate transactions have the same FromAddress and ToAddress,
and also (referring to specific UTXOs) can consolidate inbounds that THORNode hasn't reflected in its vault balances yet.
This results in that a Consolidate transaction's Coins which should not change the vault balance
can instead zero it (hitting the SafeSub
limit)
and then raise it to a higher balance than it started at (insolvency).
My current proposal is to make the observed txin/txout handlers' tx.Tx.Coins SubFunds
and AddFunds
conditional on the tx.Tx's FromAddress and ToAddress being different.
!3472 (merged)
'Tx Coins to not change vault balances when their FromAddress and ToAddress are the same'.
Context: This Discord thread.
https://discord.com/channels/838986635756044328/1215756733960556676/1216086198356021370
Specifically, vault address D64GGw3AiWXzDZRTpHXBvL6c6M255kUAbA
( thorpub1addwnpepq23r8srfathem5jgu8szm9mrjylx9y9atjeawwz4kajqpg3y9vvy7xghtst
)
indicating a 100,130.36842903 DOGE.DOGE insolvency gap,
and DCNBGQT8rDjiGSLE5rwDvHfFD9NViUvVHp
( thorpub1addwnpepq0xtamtm6l35efh3f5wlt5fql7cx9j94fywtumz83vvnzagx46h76yk8sa3
)
indicating a 900.00000000 DOGE.DOGE insolvency gap.
For DCNBGQT8rDjiGSLE5rwDvHfFD9NViUvVHp
:
https://blockchair.com/dogecoin/transaction/f6a445cc2f39177412a6829aeada4dfa4182ad2085eee9866094bcbb25431ab5
Mar 7, 2024 17:40:37 UTC
900 DOGE inbound (Savers add)
https://blockchair.com/dogecoin/transaction/f5e27099b1778e31bc275c77758f501550694f85451c8320c1cf745abafdd6c8
Mar 7, 2024 17:42:04 UTC
323,704.25937846 DOGE consolidate (11.58 gas)
https://thornode-v1.ninerealms.com/thorchain/vault/thorpub1addwnpepq0xtamtm6l35efh3f5wlt5fql7cx9j94fywtumz83vvnzagx46h76yk8sa3?height=15013859
322,815.83937846 DOGE
https://thornode-v1.ninerealms.com/thorchain/vault/thorpub1addwnpepq0xtamtm6l35efh3f5wlt5fql7cx9j94fywtumz83vvnzagx46h76yk8sa3?height=15013860
https://thornode-v1.ninerealms.com/thorchain/vault/thorpub1addwnpepq0xtamtm6l35efh3f5wlt5fql7cx9j94fywtumz83vvnzagx46h76yk8sa3?height=15013861
0 DOGE
https://thornode-v1.ninerealms.com/thorchain/vault/thorpub1addwnpepq0xtamtm6l35efh3f5wlt5fql7cx9j94fywtumz83vvnzagx46h76yk8sa3?height=15013862
900 DOGE
This reflects two issues:
1. If an outbound is observed by THORNode before an inbound which makes it possible, THORNode's vault balance can floor at zero and afterwards appear insolvent by the amount of the inbound.
This perhaps could happen within a single THOR block according to observation order, confirmation-counting-unrelated.
Here this specifically relates to Consolidates being triggered for inbounds which THORNode has not yet acknowledge have taken place.
2. A Consolidate should not leave a vault appearing to have 0 balance in any block (regarding pool solvency).
The same goes for a Migrate, in which there should be no gap in which neither RetiringVault nor destination ActiveVault record funds in transit.
I note these portions of code:
https://gitlab.com/thorchain/thornode/-/blob/v1.128.0/x/thorchain/handler_observed_txin.go#L206-212
if hasFinalised {
if vault.IsAsgard() && !voter.UpdatedVault {
vault.AddFunds(tx.Tx.Coins)
voter.UpdatedVault = true
}
vault.InboundTxCount++
}
https://gitlab.com/thorchain/thornode/-/blob/v1.128.0/x/thorchain/handler_observed_txin.go#L230-232
if memo.IsOutbound() || memo.IsInternal() {
// do not process outbound handlers here, or internal handlers
continue
https://gitlab.com/thorchain/thornode/-/blob/v1.128.0/x/thorchain/handler_observed_txout.go#L262-263
vault.SubFunds(tx.Tx.Coins)
vault.OutboundTxCount++
IsInternal
( https://gitlab.com/thorchain/thornode/-/blob/v1.128.0/x/thorchain/memo/memo.go#L147-156 )
now refers exclusively to TxMigrate and TxConsolidate memos.
Something to consider is making the SubFunds
and AddFunds
conditional on the FromAddress and ToAddress not being the same,
so MsgObservedTxOuts never reach consensus before MsgObservedTxIns and do the SubFunds
before the AddFunds
.
(Checking FromAddress ToAddress equivalency for the SubFunds
rather than on a Consolidate memo is so a malicious outbound couldn't be sent to a different address with a Consolidate memo and not be deducted from the vault balance.
An inbound with a Consolidate memo hopefully matters less as it's an inbound and its memo wouldn't prompt any swap or other action, but others' thoughts on this are also welcome.)
Something further is to change the observed txin condition from
if hasFinalised {
to
if hasFinalised || memo.IsType(TxMigrate)
so that Migrate memos update the vault immediately without confirmation counting after the outbound has already been deducted.
(Restoring a hasFinalised-only condition for vault.InboundTxCount++
to mimic its previous form and not be called twice.)
Specifically, my current proposal to immediately address this is !3472 (merged)
'Tx Coins to not change vault balances when their FromAddress and ToAddress are the same'.
I note that I have a quite strong inclination to not revert (for non-Internal outbounds) !3351 (merged)
'Increase THORNode vault balance only after inbound confirmation counting'
, both regarding ideal behaviour and specific cases of vaults having become InactiveVaults after migration and confirmation counting (#1820 (closed)).
For thoroughness, D64GGw3AiWXzDZRTpHXBvL6c6M255kUAbA
:
https://thornode-v1.ninerealms.com/thorchain/vault/thorpub1addwnpepq23r8srfathem5jgu8szm9mrjylx9y9atjeawwz4kajqpg3y9vvy7xghtst?height=15021803
325405415580408
https://thornode-v1.ninerealms.com/thorchain/vault/thorpub1addwnpepq23r8srfathem5jgu8szm9mrjylx9y9atjeawwz4kajqpg3y9vvy7xghtst?height=15021804
0
This was due to consolidate transaction
https://thornode-v1.ninerealms.com/thorchain/tx/details/6b9ef50c2219ad831fe757803136cb61852c00e2272d1efc746662f0148666be?height=15021804
of size 335397294423311.
The relevant used-for-consolidate inbound of was
https://thornode-v1.ninerealms.com/thorchain/tx/details/a3bc0d1a1474f0fb0580691f46d2592738434237a6d10a67a2eeb2d0a6c0ba60
(a swap).