Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove controller #45

Merged
merged 1 commit into from
Apr 28, 2023
Merged

Remove controller #45

merged 1 commit into from
Apr 28, 2023

Conversation

0xekez
Copy link
Contributor

@0xekez 0xekez commented Apr 27, 2023

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.

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
Copy link
Member

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
Copy link
Contributor Author

0xekez commented Apr 28, 2023

@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
Copy link
Contributor Author

0xekez commented Apr 28, 2023

^ strong opinion weakly held btw

@0xekez
Copy link
Contributor Author

0xekez commented Apr 28, 2023

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.

@0xekez 0xekez merged commit 1baad94 into main Apr 28, 2023
4 checks passed
@0xekez 0xekez deleted the zeke/audit-fixes branch April 28, 2023 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants