Vault Theft Recovery about interbtc HOT 9 CLOSED

Thanks for writing up this issue, it's a good starting point. Before we move to implementation, though, we should think about edge cases. Some from the top of my head that need to be discussed and probably have tests:

• what happens to the collateralization rate if you call slash_for_to_be_redeemed?
• what happens when the vault gets below the liquidation threshold while it is in the PendingLiquidation state. If you previously moved using slash_for_to_be_redeemed, will the correct amount be confiscated?
• What happens if a redeem is cancelled (reimbursed) while in the PendingLiquidation state that requires the locked collateral?
• We need a new storage field storing all transactions reported by recover_theft to prevent transactions from being reported multiple times
• This feature effectively allows the vault to loan bitcoin, which we should disincentivize -> we should probably take some punishment fee. To really disincentivize loaning the punishment needs to be proportional to the theft, but this will also place bigger vaults at higher risk.
• what actions are still allowed on vaults in the PendingLiquidation state? For sure open requests (issue and redeem) should be able to be fulfilled. But we'll probably should disallow new requests, similarly to banned vaults

from interbtc.

Triggering of the PendingLiquidation deadline can be performed either by off-chain workers, or by storing an on-chain heap data structure which stores VaultIds and is sorted in ascending order by the recovery_deadline_block. This is done by sending a pending_liquidation_elapsed(vault_id) extrinsic.

This could also be an incentivized action, i.e. some of the theft report fee could go to the account who submits pending_liquidation_elapsed(vault_id).

from interbtc.

Triggering of the PendingLiquidation deadline can be performed either by off-chain workers, or by storing an on-chain heap data structure which stores VaultIds and is sorted in ascending order by the recovery_deadline_block. This is done by sending a pending_liquidation_elapsed(vault_id) extrinsic.

This could also be an incentivized action, i.e. some of the theft report fee could go to the account who submits pending_liquidation_elapsed(vault_id).

While possible, I think it'd make more sense for the off-chain worker to do this

from interbtc.

@sander2

what happens to the collateralization rate if you call slash_for_to_be_redeemed?
what happens when the vault gets below the liquidation threshold while it is in the PendingLiquidation state. If you previously moved using slash_for_to_be_redeemed, will the correct amount be confiscated?

What I'm suggesting is that when a vault enters PendingLiquidation status, it is just like sending a report_vault_theft(...) extrinsic currently, but the liquidate(...) method treats the issued and to_be_issued tokens just as it currently does the to_be_redeemed ones. They are still moved to the liquidation vault. The change is equivalent to setting collateral_for_to_be_redeemed here to the entire backing_collateral of the vault.

However, one thing I just noticed and was not aware of is that the current implementation does not liquidate collateral that backs to_be_redeemed_tokens even in case of undercollateralization, which is too permissive even now in my opinion. If this is changed to actually move the collateral to the Liquidation Vault in case of undercollateralization, then we can reuse the liquidate(...) method with the modification in the paragraph above.

What happens if a redeem is cancelled (reimbursed) while in the PendingLiquidation state that requires the locked collateral?

Nothing would change there, the execution path would reach this line:

vault.is_liquidated() would return true for the PendingLiquidation status as well.

We need a new storage field storing all transactions reported by recover_theft to prevent transactions from being reported multiple times

Makes sense.

This feature effectively allows the vault to loan bitcoin, which we should disincentivize -> we should probably take some punishment fee. To really disincentivize loaning the punishment needs to be proportional to the theft, but this will also place bigger vaults at higher risk.

We can slash a small percentage of collateral on recovery, though this makes recovery from undercollateralization (with spent Bitcoin) more costly.

what actions are still allowed on vaults in the PendingLiquidation state? For sure open requests (issue and redeem) should be able to be fulfilled. But we'll probably should disallow new requests, similarly to banned vaults

The assumption would be that the vault maliciously spent the Bitcoin until proven otherwise, so it should be "quarantined" / banned, yeah.

from interbtc.

Overall, I like this approach. My suggestion is to move towards implementation if @sander2 @gregdhill @savudani8 you agree as well.

The edge cases raised by Sander are valid and should likely be thought out while implementing this feature.

Some more detailed notes:

However, one thing I just noticed and was not aware of is that the current implementation does not liquidate collateral that backs to_be_redeemed_tokens even in case of undercollateralization, which is too permissive even now in my opinion. If this is changed to actually move the collateral to the Liquidation Vault in case of undercollateralization, then we can reuse the liquidate(...) method with the modification in the paragraph above.

to_be_redeemed collateral should not be liquidated. Otherwise, a user would be forced to retry and burn their tokens instead of being able to reimburse (creating another edge case for the user).

This feature effectively allows the vault to loan bitcoin, which we should disincentivize -> we should probably take some punishment fee. To really disincentivize loaning the punishment needs to be proportional to the theft, but this will also place bigger vaults at higher risk.

We can slash a small percentage of collateral on recovery, though this makes recovery from undercollateralization (with spent Bitcoin) more costly.

Considering the complexity, I'd be in favor of just taking the theft reporting fee from the vault on every report (https://spec.interlay.io/spec/fee.html#theftfee) instead of introducing another fee. When the Vault is finally liquidated, there is no other fee taken. That way:

• "Manual" actions by other Vaults are incentivized
• Vaults are punished for taking a BTC loan
• Logic is a bit simpler IMO

what actions are still allowed on vaults in the PendingLiquidation state? For sure open requests (issue and redeem) should be able to be fulfilled. But we'll probably should disallow new requests, similarly to banned vaults

The assumption would be that the vault maliciously spent the Bitcoin until proven otherwise, so it should be "quarantined" / banned, yeah.

Reusing the same restrictions as for the banned status is likely a good idea.

from interbtc.

@sander2 @gregdhill @nud3l I'd like to suggest simply banning vaults rather than adding an intermediary step to confiscating collateral. The vault's status is still set to PendingTheft(...), but it would ensure no edge cases are introduced. Otherwise, critical logic in the code would need to be changed, such as cancel_redeem.

The downsides of simply banning vaults rather than freezing their collateral or calling slash_for_to_be_redeemed on them are:

• Vaults would be able to withdraw collateral that backs tokens which were not stolen (by redeeming or requesting to replace)
• A race condition would be introduced, since TheftRecoveryPeriod would be replaced by the PunishmentDelay. Vaults could become unbanned before someone triggers liquidation. If there is an overlap of one block between the ban being active and pending_liquidation_elapsed() being callable it's probably fine though.
• Rewarding theft reporters has to be refactored out of the liquidate method, but this looks fairly simple

In spite of the above, all concerns raised in the previous comments would be removed:

• Vaults in PendingTheft status would still be liquidated for undercollateralization, like an active vault
• No new requests can be made to PendingTheft vaults
• Vaults are punished using the report-theft fee, so they would not abuse this feature
• Handling of pending / cancelled requests would not change at all

from interbtc.

If we can reuse existing logic, I think then it's the right approach to do so.

Questions/comments:

The downsides of simply banning vaults rather than freezing their collateral or calling slash_for_to_be_redeemed on them are:

• Vaults would be able to withdraw collateral that backs tokens which were not stolen (by redeeming or requesting to replace)
  That is fine. They would only be able to withdraw the collateral if they fulfill the requests.

• A race condition would be introduced, since TheftRecoveryPeriod would be replaced by the PunishmentDelay. Vaults could become unbanned before someone triggers liquidation. If there is an overlap of one block between the ban being active and pending_liquidation_elapsed() being callable it's probably fine though.

Is there a way to ensure that there is no race conditions? We should make sure vaults are liquidated. If they are unbanned, users might request to issue and send BTC to a vault that should be liquidated.
My suggestion (which adds some extra checks) is to treat any Vault as banned as long as it's in the PendingTheft. That way, the Vault might be "unbanned" but it's still being treated as being banned before the liquidation is executed.

from interbtc.

My suggestion (which adds some extra checks) is to treat any Vault as banned as long as it's in the PendingTheft. That way, the Vault might be "unbanned" but it's still being treated as being banned before the liquidation is executed.

I agree, this seems to most elegant - we can impose the restrictions of banned vaults without actually touching the banned_until field just by adding a check for PendingTheft status here.

from interbtc.

Theft reporting removed in #677

from interbtc.