Broken invariant: TOR -> L1 loan open refund leaves TOR in the Pool Module
Context, handler_loan_open.go
's ``openLoanV121`:
https://gitlab.com/thorchain/thornode/-/blob/v1.127.0/x/thorchain/handler_loan_open.go#L331-333
tx := common.NewTx(txID, common.NoopAddress, common.NoopAddress, common.NewCoins(torCoin), nil, "noop")
// we do NOT pass affiliate info here as it was already taken out on the swap of the collateral to derived asset
swapMsg := NewMsgSwap(tx, msg.TargetAsset, msg.TargetAddress, msg.MinOut, common.NoAddress, zero, msg.Aggregator, msg.AggregatorTargetAddress, &msg.AggregatorTargetLimit, 0, 0, uint64(ssInterval), msg.Signer)
Example:
https://thornode-v1.ninerealms.com/thorchain/block?height=13441044
{
"chain": "THOR",
"code": "108",
"coin": "610210000 THOR.TOR",
"from": "noop",
"id": "E27410CFED19F961FAEAEA6D64D354F63F202FD7BF5C4801455E48A4D0E165A2",
"memo": "noop",
"reason": "fail swap, not enough fee",
"to": "noop",
"type": "refund"
},
TOR entering the Pool Module (queued swap):
https://thornode-v1.ninerealms.com/thorchain/balance/module/asgard?height=13441042
https://thornode-v1.ninerealms.com/thorchain/balance/module/asgard?height=13441043
{
"denom": "tor",
"amount": "610210000"
}
Broken invariant upon failed swap (and the TOR remaining in the Pool Module):
https://thornode-v1.ninerealms.com/thorchain/invariant/asgard?height=13441043
https://thornode-v1.ninerealms.com/thorchain/invariant/asgard?height=13441044
"oversolvent: 610210000tor"
refundTx
here calls TryAddTxOutItem
for the MsgSwap's Tx's FromAddress, here NoopAddress;
TryAddTxoutItem
here returns true, nil
for a NoopAddress, I believe so that swaps to NoopAddress appear to succeed without moving any funds.
|
(While on the subject, please note incidentally !3028 (closed)
'Refactor TryAddTxOutItem to return an error value only (like UnSafeAddTxOutItem), no bool value'
regarding TryAddTxOutItem
returning a redundant bool that always equals err == nil
.)
At this time I do not know what a preferable resolution would be; presumably the TOR should be burnt with unrefundableCoinCleanup
rather than left in a module,
but even if the FromAddress were the Lending Module's address it would perhaps only leave it stuck in the Lending Module instead of the Pool Module,
since the swap queue is used and so CacheContext
cannot leave the loan open itself uncommitted.
Does it seem worth using an IsModuleAccAddress
call in refundTx
to never refund anything to a module address, no matter what?
A related concern, relating to parallel code for a Derived Asset -> L1 Asset swap in loan repayment: https://gitlab.com/thorchain/thornode/-/blob/v1.127.0/x/thorchain/handler_loan_repayment.go#L248-249
tx := common.NewTx(txID, msg.Owner, common.NoopAddress, coins, common.Gas{fakeGas}, "noop")
swapMsg := NewMsgSwap(tx, msg.CollateralAsset, msg.Owner, msg.MinOut, common.NoAddress, cosmos.ZeroUint(), "", "", nil, 0, 0, uint64(ssInterval), msg.Signer)
Here the FromAddress is the msg.Owner, rather than common.NoopAddress .
When the repayment asset was a non-THOR Asset (with a non-THOR FromAddress)
the derived asset is burnt as in
https://thornode-v1.ninerealms.com/thorchain/block?height=13847537
fail to refund (239998 THOR.BTC): fail to prepare outbound tx: to address(bc1qwmclj6cf4qzvnlq7xry8wlj7xhx4rt93c6t9g9), is not of chain(THOR)
, but I worry that if the repayment asset is a THOR asset (RUNE or a synth)
that Derived Assets could be refunded to the balance of a non-module address even if EnableDerivedAssets
is 0.
(Hopefully no problematic consequences, but also not something that should ever happen.)
[Update: After testing, I have realised that 'msg.Owner' is the Layer 1 collateral address,
not the message broadcaster. The repayment Derived Asset {fail to refund} and burn does then behave as intended, though for symmetry I am still inclined for the FromAddress to not be hardcoded as the Layer 1 collateral address.]
Currently I am inclined to first seek to produce regression tests for both the above scenarios.
Perhaps making the from-Derived-Asset's swap's FromAddress the Lending Module address for both
, and barring refundTx
from refunding to a module address
could resolve both scenarios, but this should be tested.
(Edit: Though the funds have already been moved to the Pool Module with SendFromModuleToModule
,
this is the equivalent of how a MsgDeposit moves funds to the Pool Module before calling the internal handler,
with the FromAddress to refund to still the address which had moved them to the Pool Module.
If there were any refund it should be to the Lending Module,
but since to the Lending Module it should burn coins on attempted (swap queue) refund
since it cannot do a CacheContext reversion of the loan state change and attempt refund of the message signer's original coins.)
2024-01-31 update:
Illustrative regression test:
01d3948b
https://gitlab.com/thorchain/thornode/-/jobs/6065665787
A note about POL:
Even if a Reserve POL add liquidity refund attempt were to be unrefundable,
the unrefundable RUNE would still be sent to the Reserve by the clean-up process.
As it happens, if a POL add liquidity fails the RUNE is transferred directly back with SendFromModuleToModule
rather than refunded with refundTx
,
which avoids any fee deduction.
( https://gitlab.com/thorchain/thornode/-/blob/v1.127.0/x/thorchain/manager_network_current.go#L890-893 )
I comment that
!2928 (merged)
'[V113-specific] Set correct ToAddress when loan handlers make a MsgSwap'
has at some point been reverted (while leaving its "the ToAddress should be AsgardName's address" comment),
the swap event's ToAddress becoming NoopAddress instead of the Pool Module address.
In a MsgSwap, the from_address
is the address that sent the funds (normally a user, here the Lending Module),
the to_address
is the protocol address that received the funds before the swap (the Pool Module for native coins, an external Asgard address for external coins),
and the destination
is the target address to which the swap is made.
https://gitlab.com/thorchain/thornode/-/blob/v1.127.0/x/thorchain/handler_swap.go#L102-106
// since derived assets are disabled, only the protocol can use
// them (specifically lending)
if !msg.Tx.FromAddress.Equals(common.NoopAddress) && !msg.Tx.ToAddress.Equals(common.NoopAddress) && !msg.Destination.Equals(common.NoopAddress) {
return fmt.Errorf("swapping to/from a derived asset is not allowed, except for lending (%s or %s)", msg.Tx.FromAddress, msg.Destination)
}
Hmm. Here the comment indicates LendingName specificity, but the conditionals are all NoopAddress.
Like the swapV
Destination NoopAddress, then currently used to indicate protocol origin;
at the least the loan handler comments can be updated.
My proposed code at this time (including refundTx
and necessary IsModule
(/Acc)Address
tidying): !3407
!3407
'Treat refunds to network modules as unrefundable'.