Draft: Do not ignore BNB.RUNE solvency, only the solvency status of ETH.RUNE and synths

Edit: Rather than updating the version for V98 (and/or later), I am closing this for the time being.

This is a backwards-compatible non-Bifrost MR for #1416 (closed)'s secondary proposal, suitable for a +v0.1.0 rather than +v0.0.1 release.
(This and !2557 (merged) together close #1416 (closed).)

Specifically, the intended purpose is for handler_solvency.go to only ignore ERC20 RUNE (ETH.RUNE, actually burned on switching) and not to ignore BEP2 RUNE (BNB.RUNE, alarming if it were somehow moved elsewhere and re-switched).

To rephrase, use of IsRune() here instead of Equals(common.ERC20RuneAsset()) currently appears to me to be an unnecessary security vulnerability.

Something to note is that THOR.RUNE would also be ignored; however, due to the lines

		if !c.Asset.Chain.Equals(chain) {
			continue
		}

, THOR.RUNE (or any THOR. coin) should always be ignored anyway for external chain MsgSolvency messages.

Rather, it is if an Asgard somehow contained a synth (THORChain asset, but an external-chain 'Chain') that the synth should not be checked against the external-chain wallet balance as reported by the MsgSolvency message, thus an explicit IsSyntheticAsset() check I feel is more appropriate.

changed the description

approved this merge request

approved this merge request

added 10 commits

• 7c179912...696080f1 - 7 commits from branch thorchain:develop
• 65803e79 - handleV97 and insolvencyCheckV97, no logic changes
• 9b4ec1a5 - Only solvency-ignore ETH.RUNE, not BNB.RUNE too
• 8b75ddfd - Ignore any Asgard synths when checking solvency

Compare with previous version

Nope, I don't think we need to change this at the moment

1.BNB solvency checker is not enabled due to the way how Binance beacon chain works. 2. AsgardVault will not hold any synth asset , and we don't do solvency check on thorchain either

I see this is a change with little bebefit

Thank you for the context about the BNB solvency checker not being enabled.
Would you tell me more about why solvency is not checked for the Binance beacon chain?

(Having seen the Approvals, checking before altering the MR status-- @ursa9r / @akrokr , shall/should I close it?)

The reason we disable solvency checker on BNB beacon chain is because on beacon chain , there is no way to ask the binance daemon account balance on a specific height.

When bifrost ask binance daemon, "what is my current account balance?" , It always return the latest result. Since Binance beacon chain has 300ms block time , so it is a constantly moving target, it is very hard for each node to agree on each other about what balance it has. Given that solvency checker on BNB chain will likely cause more slashes ,or false positive.

Thank you for this context! Inability to query historical data sounds quite painful.
I feel even gladder that churn migrate observations can be used as a regular reference for vaults' wallet balances.

closed

marked this merge request as draft

changed the description