Add Plugin support

[cleanunicorn]

Implements #42.

[PaulRBerg]

Thanks very much @cleanunicorn, this is looking great so far. I left my feedback below.

Additionally, some notes:

• I didn't finish the migration to Foundry in this repo, so wherever you feel like a component is missing, please do take a look at prb-math and prb-contracts for inspiration, because both are already shipped.
• It looks like you have committed the artifacts in version control - perhaps this was a left-over from a previous checkout in which you had compiled the PRBProxy contracts with Hardhat. The Hardhat artifacts should be deleted.
• The tests are not structured correctly.
  • There is no need for a base PluginTest because the plugin functionality is incorporated in PRBProxy, and so the plugin-related events and variables should be added in the existing PRBProxyTest contract.
  • Crucially, this repo follows the testing trees/ branching approach. A fellow Ethereum developer called Sabnock who likes this testing approach wrote an article about it (he calls it "state building").
  • The easiest way to understand what I mean by this would be to take a look at the PRBMath test trees and the PRBContract test trees
  • To ensure a smooth developer experience while writing the tests trees, I recommend installing the vscode-tree-language VSCode extension to get syntax highlighting for the *.tree files.
  • The tests are meant to be separated with empty modifiers.
• While the Angular Commit Message Format doesn't have many hard-and-fast rules, I would have used different commit types than the ones that you have used so far:
  • Implementing the plugin is a change of type feat instead of chore.
  • Adding events in a smart contract is a change of type feat instead of chore (because you add functionality); also, when bundling lots of charge under the roof of one commit, it's helpful to add more information in the git commit body, e.g. in commit 306d0ef an additional docs: document plugin functions with NatSpec would have been helpful.
  • Adding tests is a change of type test instead of chore.
  • Fixing linting/ styling errors is a change of type chore or style instead of fix (the latter is reserved for smart contract bug fixes).

And some questions:

1. I'm just thinking out loud, but doesn't this plugin functionality introduce a vulnerability whereby a malicious target contract can call any arbitrary plugin by calling the fallback function on the proxy itself, after the user has called the target contract via the normal execute pathway?
2. May I have your permission to edit the git commit messages based on the feedback above? Or, alternatively, you could do it. The idea would be to git push with --force.

[cleanunicorn]

And some questions:

1. I'm just thinking out loud, but this plugin functionality introduce a vulnerability whereby a malicious target contract can call any arbitrary plugin by calling the fallback function on the proxy itself, after the user has called the target contract via the normal execute pathway?

We will need to think deeply about security issues that might appear since this does a delegatecall. We should properly think about attack vectors once the plugin system is closer to finality.

Currently there is no permission schema for plugins.

I don't understand the example you're explaining. I'm not sure this is an issue since the executed code should still be trusted (is this right?). Could you add more details?

Also, it's important to understand trusted and untrusted actors in the system.

2. May I have your permission to edit the git commit messages based on the feedback above? Or, alternatively, you could do it. The idea would be to git push with --force.

I tried rewriting, but feel free to push commits (rewrite history), I gave you permission.

[newts6]

Excuse my interjection here :)

Re:

1. I'm just thinking out loud, but this plugin functionality introduce a vulnerability whereby a malicious target contract can call any arbitrary plugin by calling the fallback function on the proxy itself, after the user has called the target contract via the normal execute pathway?

It might be worth having a plugin (or method) type which gets only invoked with a staticcall instead of delegatecall.

[cleanunicorn]

Open questions:

1. I still need additional information regarding the security issues. At the moment, I don't see an attack vector.
2. @newts6, how would the static call be useful? Can you give an example?
3. Installing a plugin can possibly overwrite another plugin. Should we make sure the installedPluginMethods[methodList[i]] == 0? Could we add an argument to install(plugin, bool overwrite)?
4. Uninstalling a plugin can potentially disable a method from another plugin. Should I make sure the removed method is from the specified plugin before disabling it?

[newts6]

2. @newts6, how would the static call be useful? Can you give an example?

Assurance of a reduced attack surface with non state changing calls would allow a more modular security policy for plugin installers. i.e. being way more relaxed with read only plugins.

Several use cases could benefit, first and foremost EIP1271 implementations ofc. But also I suppose most callbacks for ERC721/1155 safe transfers.

[PaulRBerg]

Replies

I'm not sure this is an issue since the executed code should still be trusted (is this right?). Could you add more details?

I think you're right - if you give permission to a target contract and you delegate-call to it, then if they are malicious they can do other things directly. They don't need to jump through hoops and use the plugin system.

Target contracts are indeed trusted actors under the current design of PRBProxy, and the plugin system you have implemented is orthogonal to it.

feel free to push commits (rewrite history), I gave you permission

Thank you! I pushed with --force to update the historical commit messages.

NOTE: you might have to delete your local feat/plugins to be able to fetch the latest changes.

3. Installing a plugin can possibly overwrite another plugin

Good catch, but I think that this is fine. Plugins should normally not overlap, just like ERCs don't usually overlap.

The complexity of implementing an overwrite check, and the maintenance cost do not seem worth it to me. Similarly, with the bool overwrite proposal - the overhead is not worth it.

Also, this is a rectifiable scenario - in case a user accidentally overwrites a plugin, they can simply uninstall it, and then install a new one that doesn't have the overwritten methods.

4. Should I make sure the removed method is from the specified plugin before disabling it?

In a similar vein to the above, I would classify this as a rectifiable issue that can easily be patched manually by the user.

I would have been open to implementing these checks if we had a different access control schema that allowed third-parties to call the installPlugin and uninstallPlugin functions on behalf of the user - in that case, a user could easily be caught off-guard by an accidental overwrite triggered by someone else.

But, since in our implementation, it is the user only who can install and uninstall plugins, they should be well aware of what they are installing.

Also worth mentioning is that the "user" referred to above wouldn't actually be the end user, i.e. the owner of the proxy - but rather a frontend developer who would parse the proxy API on behalf of the actual end user.

Feedback

On top of what I left in the comments above.

1. Test Function Names

This is something that I learned in the meantime - a few Foundry contributors are working on a new Foundry-based linter for Solidity called scopelint, which will use a specific regex for linting test function names. See point 2 in General Test Guidance section of the Foundry Book for more detail; for now, what matters is that I have updated your test names to adhere to these best practices. I will also update the base branch staging to follow this format, and then I will do the rebase myself.

2. Name of execute directory

I think that it is misleading to use the same directory name for the plugin execution tests. I have renamed it to run-plugin to make a clear distinction between the core execution function tests, and the plugin runs.

3. No testing trees

The tests still do not conform with the testing tree approach - there are no *.tree files.

But don't worry, I know I have asked for a lot already. Thank you for getting the ball rolling with the first suite of tests - I will refactor them to adhere to the system I have in place.

4. Install and Uninstall Tests should be separated

Different functions are always tested separately.

[PaulRBerg]

Also, @cleanunicorn, FYI - I have pushed a big commit to address all of the feedback left in the code review comments.

I am still due to refactor the tests to adhere to the testing tree approach - but once that's done, we can merge this in, after you review my feedback, too, of course.

Update: I have managed to complete the migration to Foundry on the staging branch, which has introduced a few git conflicts with this PR. I will fix them myself.

[PaulRBerg]

After pushing a few commits these days, we are now almost ready to merge this PR.

@cleanunicorn, could you take a final look at the latest changes to confirm that you're happy with the aesthetic changes I made (e.g. rename IPlugin to IPRBProxyPlugin)?

[PaulRBerg]

Two final security-related points from my end.

Strange Loops

Is it possible to install the proxy itself as a plugin by running the following steps:

1. Create a (malicious?) plugin contract that returns the methodList method selector itself in its implementation of methodList, among other methods implemented in PRBProxy itself (e.g. transferOwnership).
2. Call installPlugin and pass the proxy's address. The proxy itself will get stored as the associated plugin for the methodList method and the other method selectors, because the call to methodList will trigger a call to the fallback function, which would call the malicious plugin from step 1, which would return its method list.

Now, the fun part: calling any method not defined in the proxy itself will lead to infinite recursion in the fallback function, triggering a stack overflow.

However, calling a method defined in the proxy would result in a successful transaction. The question is whether this can lead to any exploit.

I don't think it can because all essential functions are protected by owner != msg.sender checks. Calling these functions via the fallback function would mean jumping through hoops to achieve the same goal.

Therefore, adding an explicit check in the installPlugin function that would prevent the proxy itself from becoming a plugin doesn't seem helpful to me - the gas cost would increase, but no security value would be added.

Empty Data

When the calldata is empty, the receive function gets called.

[PaulRBerg]

The PR LGTM now. Test coverage is 100%:

[PaulRBerg]

There are conflicts again .. I will fix them tomorrow.

[PaulRBerg]

There were many conflicts and frankly I was unsure what was the fix for several of them - to avoid the risk of deleting something accidentally, I have squashed all commits in one commit (was going to do this anyway, before merging), and rebased.

PR ready for your final review, @cleanunicorn!

[cleanunicorn]

Strange Loops

Is it possible to install the proxy itself as a plugin by running the following steps:

1. Create a (malicious?) plugin contract that returns the methodList method selector itself in its implementation of methodList, among other methods implemented in PRBProxy itself (e.g. transferOwnership).

It is possible for the plugin to try to overwrite transferOwnership, however the PRBProxy.transferOwnership will take precedence over the method defined in the plugin. If no method in the PRBProxy matches the one called, then the fallback method is activated. Thus, the proxy can't overwrite methods in PRBProxy.

2. Call installPlugin and pass the proxy's address. The proxy itself will get stored as the associated plugin for the methodList method and the other method selectors, because the call to methodList will trigger a call to the fallback function, which would call the malicious plugin from step 1, which would return its method list.

Now, the fun part: calling any method not defined in the proxy itself will lead to infinite recursion in the fallback function, triggering a stack overflow.

However, calling a method defined in the proxy would result in a successful transaction. The question is whether this can lead to any exploit.

I don't think it can because all essential functions are protected by owner != msg.sender checks. Calling these functions via the fallback function would mean jumping through hoops to achieve the same goal.

Therefore, adding an explicit check in the installPlugin function that would prevent the proxy itself from becoming a plugin doesn't seem helpful to me - the gas cost would increase, but no security value would be added.

Any method defined in PRBProxy is tried before any method in an installed proxy is called. This should protect from multiple attack vectors since it uses the security properties of PRBProxy.

Empty Data

When the calldata is empty, the receive function gets called.

This should be regarded as receiving ether. I believe we desire this behavior.

[PaulRBerg]

Thanks for the final review, @cleanunicorn! I agree with all the points you made.