filecoin-project / builtin-actors

The Filecoin built-in actors
Other
82 stars 79 forks source link

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

Open anorth opened 2 years ago

anorth commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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.

anorth commented 2 years ago

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)

anorth commented 2 years ago

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

anorth commented 2 years ago

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

arajasek commented 2 years ago

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.

arajasek commented 1 year ago

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

arajasek commented 1 year ago

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

arajasek commented 1 year ago

@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 commented 1 year ago

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

lemmih commented 1 year ago

@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.