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

Audit and remove unwanted builtin-actor code for FVM #424

Open
anorth opened this issue May 30, 2022 · 11 comments
Open

Audit and remove unwanted builtin-actor code for FVM #424

anorth opened this issue May 30, 2022 · 11 comments

Comments

@anorth
Copy link
Member

anorth commented May 30, 2022

The built-in actors were developed in an environment assuming there were no user-programmed actors. Some of the basic ones like the Init actor, multisig, payment channel have code in them that will limit their use by user-programmed actors once available.

As an example, the Init actor has an allow-list for which actor code CIDs are permitted to initialise which other actors, which is scoped to allow only the known actors. It may be ok (if unnecessary) to retain some kind of restriction on creation of built-in miner or singletone actors, but as written this will prevent anyone creating any other actor.

Various actors also contain caller checks that prohibit callers outside the built-in account and multisig actor. But anything that permits the built-in multisig should permit any actor to call it.

cc @jennijuju @ZenGround0 @arajasek

@anorth
Copy link
Member Author

anorth commented May 30, 2022

I would expect we don't need a FIP for the change here unless they change some behaviour to be other than what might be expected had we had user-programmed actors from the start.

@jennijuju
Copy link
Member

@ZenGround0 is going to take the first pass on audit what's need to be cleaned up.
Then one of the lotus team member may pick it up.

@jennijuju jennijuju added the P3 label Jun 13, 2022
@anorth
Copy link
Member Author

anorth commented Jul 24, 2022

Another item: the payment channel requires that addresses are of account actors specifically. If we implement either account abstraction or a standard signature verification method, we should instead delegate signature checking to the actor in question. cc @arajasek

(all calls to resolve an actor code CID are suspect)

@jennijuju jennijuju added this to the Network v17 milestone Aug 11, 2022
@anorth
Copy link
Member Author

anorth commented Aug 11, 2022

The miner actor only supports built-in accounts and multisigs as control addresses

@anorth anorth moved this to Todo in Network nv17 Aug 11, 2022
@anorth
Copy link
Member Author

anorth commented Aug 15, 2022

Almost anything that uses the built-in actor Type enum is suspect. We probably want to remove these syscalls entirely, so we need to remove the users of them. Most urgent is the paych and multisig (or any actor that uses their type). Ref #520

@anorth anorth moved this from Todo / In Scope to Opportunistic in Network nv17 Sep 22, 2022
@anorth anorth removed this from Network nv17 Oct 3, 2022
@arajasek
Copy link
Contributor

arajasek commented Nov 7, 2022

In #807 @ZenGround0 flags that we want to:

Also note that the market actor call to AuthenticateMessage should change to AuthenticateMessageExported. Any other method that we expect user actors to implement and builtin actors to call should also be handled this way.

@anorth anorth removed this from the Network v17 milestone Nov 7, 2022
@jennijuju jennijuju moved this to 📋 Backlog in Network v18 Nov 7, 2022
@arajasek
Copy link
Contributor

arajasek commented Nov 8, 2022

Adding to the reengineering API milestone, though I don't think we need to hit everything covered here as part of that effort.

@arajasek
Copy link
Contributor

Deprecating the old Miner::ControlAddress method, and using the new IsControllingAddress method would be nice.

@arajasek arajasek moved this from 📋 Backlog to 🏗 In progress in Network v18 Nov 25, 2022
@arajasek
Copy link
Contributor

arajasek commented Nov 25, 2022

@jennijuju and I are attempting to groom this issue a little. Here's a summary of proposed ideas -- some of these may simply require a quick check before being marked complete:

@jennijuju
Copy link
Member

@lemmih Do you know who from your team will pick up this issue yet?

@lemmih
Copy link
Contributor

lemmih commented Dec 9, 2022

@lemmih Do you know who from your team will pick up this issue yet?

@sudo-shashank will be looking at this once he's finished with his other builtin-actor tasks.

@lemmih lemmih self-assigned this Dec 13, 2022
@lemmih lemmih removed their assignment Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: 🏗 In progress
Development

No branches or pull requests

5 participants