DA0-DA0 / polytone

An account on every blockchain for every smart contract.
45 stars 7 forks source link

Remove controller #45

Closed 0xekez closed 1 year ago

0xekez commented 1 year ago

The controller was designed to support atomic token transfer and action on a remote chain. It has two rules:

  1. If a note has a controller, only the controller may execute messages.
  2. The controller may specify an arbitrary sender when executing messages.

The result of this is:

  1. Users have two accounts: their outpost account, and their regular Polytone account.
  2. The security level of the outpost account is the security level of the outpost, as the outpost is the controller.

While this works, it's not the greatest. Having two accounts isn't perfect UX, and handing off account security to another contract "feels" bad. Oak Security also classified this as a "major" issue in their audit report, saying that the requirement that Outposts are given unrestricted access to user accounts was quite sub-optimal.

The solution to this problem is non-obvious and complex. In their audit report, Oak suggests requiring the outpost user to pre-authorize a message for execution by the Outpost later. This doesn't work because not all of the information needed to create the second message is guarenteed to be avaliable at the time the first message is to be executed. For example, a transfer message can't be pre-crafted for a NFT collection being transfered to a remote chain for the first time, as the address of the NFT collection smart contract does not yet exist.

To resolve this, one could design some system wherein a transfer is pre-authorized for any non-existant denomination, or perhaps there are better schemes. Unfourtunately, schemes of this type belongs in the outpost code, not Polytone, as it requires custom code per token-type.

What a strange loop. Starting at from the permise of how to remove the controller, we arrive at the conclusion that we need the controller.

So why remove it? I am unsatisfied with this conclusion, the controller is a blemish on an otherwise simple and beautiful codebase, and off-chain it has become clear that we won't be building an outpost with Polytone in the near future.

JakeHartnell commented 1 year ago

Why not just make a comment about it and add a warning? We may as well keep it in the Polytone repo but not include it in the audit report.

So why remove it? I am unsatisfied with this conclusion, the controller is a blemish on an otherwise simple and beautiful codebase, and off-chain it has become clear that we won't be building an outpost with Polytone in the near future.

Upsetting, but feel there may be other takers.

0xekez commented 1 year ago

@JakeHartnell: Why not just make a comment about it and add a warning?

Having the controller complicates the API as the note module has an on_behalf_of field that is only actually usable if a controller is set, so there is a small cost. On the other hand, I think there is zero cost to leaving it in the commit history, as if we re-add it we won't have to change any existing note modules, as property (1) of controllers means there is a new note per-outpost. It also got covered in the audit, so we know it's logically correct if we ever want to add it back.

0xekez commented 1 year ago

^ strong opinion weakly held btw

0xekez commented 1 year ago

I've also added documentation about this to the outpost wiki page, which I think protects us from forgetting this exists in the commit history.