Don't jail SuperApps

[d10r]

The jailing mechanism makes it difficult to write contracts implementing the SuperApp interface which don't risk getting bricked.

Assumption:
SuperApps get jailed if reverting in order to avoid them causing harm to the protocol.

Way in which this could happen:

1. a malicious SuperApp may be able to exploit an unknown protocol vulnerability
2. a faulty SuperApp may harm users

Rebuttals:

1. While SuperApps may have a larger attack surface into the protocol than other interfacing contracts, this isn't a very compelling argument because:
   • jailing is only a weak (if any) defense here: a) a vulnerability may be exploitable without even triggering a jailing event, b) by the time the jailing event is triggered, the exploit may already have happened.
   • SuperApp whitelisting is the mechanism for this
2. Jailing will in many cases not just not prevent this from happening, but even make it worse. It cannot prevent the first instance of misbehavior and the damage which may arise from the missed callback execution. Additionally it has the effect of making all future callback executions fail too (by not even attempting them). While the first miss may already have irreparably damaged the SuperApp state, one can also easily imagine cases where the damage is confined to the user making the tx, not affecting other users. In such cases, the jailing does make the situation worse by breaking the contract for everybody.

Suggestion:

• disable the jailing mechanism.
• in the SuperApp docs: emphasize that failing callbacks will NOT make the whole transaction fail and could thus lead to the contract state to become inconsistent, potentially leading to problems like liquidations of outgoing streams nomore covered by incoming ones (this is actually the case with Jailing in place too - but with that danger gone, we may need to more explicitly highlight the danger of missed state updates).
• recommend the implementation of fallback/emergency mechanisms which allow to recover from such issues and/or to orderly migrate from the contract to a new one. Example: https://github.com/ngmachado/ul-integration/blob/main/contracts/AppLogic.sol#L176 allows to manually trigger an action which the callback is supposed to trigger, but which may fail (e.g. due to exceeding the SuperApp gas budget)

If we find good reasons for keeping the jailing mechanism, we could also consider making it non-automated, e.g. by having a blacklist of SuperApps curated by governance.

[d10r]

observations from the latest Ricochet incident:

• it wasn't expected that a SuperApp could still accept new incoming streams after being jailed
• missing: alerting system if SuperApp is jailed

[d10r]

update after another conversation about it:

Instead of removing the jailing mechanism, it could also be redefined to work in a different way:

Option A

We only keep the Jailed event and bool, but jailing doesn't have any other effect.

Rationale:
The event makes it easy to have monitoring and alerting for failing callbacks.
The bool allows SuperApps to activate special functionality (e.g. emergency stop or recovery).
Callbacks aren't blocked, because they're try/catch-wrapped anyway and blocking them may not help (or even harm).

Option B

• can't create new agreement to jailed app (not only are callbacks not triggered, but the whole tx fails)
• can delete agreements to jailed app (should callbacks still be triggered?)
• Optional: governance can unjail jailed apps

Rationale: If a goal of the mechanism is to protect app users, the most effective way to do so is likely blocking creation of new agreements to a potentially misbehaving app.
Deleting existing agreements needs to always work - not so clear though if blocking the callbacks after a revert is helpful, since it can't block the transaction anyway.
The option of governance un-jailing would allow apps with non-fatal bugs to keep operating once there's reason to believe that no harm will be done. There could e.g. be cases where a simple frontend adaptation could prevent "bad" txs from happening, allowing the app to continue operation.

[d10r]

Option A.1

• keep the Jailed event
• also call a hook (e.g function terminateCallbackReverted() external returns(bool disableFutureCallbacks) to be added to ISuperApp) which allows the SuperApp to react, e.g. by pausing the application such that the project's governance could review the situation and handle it as they deem appropriate. Handling of this additional callback would be optional. Only if handled and returning false for disableFutureCallbacks would the SF protocol continue to issue callbacks. In this case we'd probably also want a mechanism for the SuperApp to re-enable callbacks after a jailing event.

[d10r]

An important aspect was overlooked here: jailing is also about limiting the damage a SuperApp not properly returning borrowed deposits can cause.