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

refactor: storage redesign #120

Merged
merged 1 commit into from
Jun 28, 2023
Merged

refactor: storage redesign #120

merged 1 commit into from
Jun 28, 2023

Conversation

PaulRBerg
Copy link
Owner

@PaulRBerg PaulRBerg commented Jun 18, 2023

Addresses https://github.com/cantinasec/review-sablier2/issues/13, the problem explained in #100, and PRB-01M: Insufficient Protection Against Hostile Takeover.

Changelog:

  • The plugins and the permissions are now in the registry
  • There is no Annex contract anymore

@PaulRBerg PaulRBerg requested a review from andreivladbrg June 18, 2023 09:45
@PaulRBerg PaulRBerg force-pushed the feat/get-proxy branch 2 times, most recently from 61ec3ea to f55cba6 Compare June 18, 2023 10:00
@PaulRBerg PaulRBerg force-pushed the refactor/storage-redesign branch from e366797 to 6cbe0d6 Compare June 18, 2023 10:01
@PaulRBerg PaulRBerg force-pushed the refactor/storage-redesign branch 2 times, most recently from 34a7a83 to f58060c Compare June 18, 2023 10:15
Comment on lines +212 to +235
function uninstallPlugin(IPRBProxyPlugin plugin) external override onlyCallerWithProxy {
// Get the method list to uninstall.
bytes4[] memory methodList = plugin.methodList();

// The plugin must have at least one listed method.
uint256 length = methodList.length;
if (length == 0) {
revert PRBProxyRegistry_PluginEmptyMethodList(plugin);
}

// Uninstall every method in the list.
address owner = msg.sender;
for (uint256 i = 0; i < length;) {
delete _plugins[owner][methodList[i]];
unchecked {
i += 1;
}
}

// Log the plugin uninstallation.
emit UninstallPlugin(owner, _proxies[owner], plugin);
}

Check notice

Code scanning / Slither

Reentrancy vulnerabilities

Reentrancy in PRBProxyRegistry.uninstallPlugin(IPRBProxyPlugin) (src/PRBProxyRegistry.sol#212-233): External calls: - methodList = plugin.methodList() (src/PRBProxyRegistry.sol#214) State variables written after the call(s): - delete _plugins[owner][methodList[i]] (src/PRBProxyRegistry.sol#225)
Comment on lines +181 to +204
function installPlugin(IPRBProxyPlugin plugin) external override onlyCallerWithProxy {
// Get the method list to install.
bytes4[] memory methodList = plugin.methodList();

// The plugin must have at least one listed method.
uint256 length = methodList.length;
if (length == 0) {
revert PRBProxyRegistry_PluginEmptyMethodList(plugin);
}

// Install every method in the list.
address owner = msg.sender;
for (uint256 i = 0; i < length;) {
_plugins[owner][methodList[i]] = plugin;
unchecked {
i += 1;
}
}

// Log the plugin installation.
emit InstallPlugin(owner, _proxies[owner], plugin);
}

Check notice

Code scanning / Slither

Reentrancy vulnerabilities

Reentrancy in PRBProxyRegistry.installPlugin(IPRBProxyPlugin) (src/PRBProxyRegistry.sol#181-202): External calls: - methodList = plugin.methodList() (src/PRBProxyRegistry.sol#183) State variables written after the call(s): - _plugins[owner][methodList[i]] = plugin (src/PRBProxyRegistry.sol#194)
@PaulRBerg PaulRBerg force-pushed the refactor/storage-redesign branch from f58060c to 30a8cd4 Compare June 18, 2023 11:50
@PaulRBerg PaulRBerg force-pushed the refactor/storage-redesign branch 2 times, most recently from b47288b to c2b3225 Compare June 19, 2023 11:54
@PaulRBerg PaulRBerg changed the base branch from feat/get-proxy to main June 19, 2023 12:01
@PaulRBerg PaulRBerg changed the base branch from main to feat/get-proxy June 19, 2023 12:01
@PaulRBerg PaulRBerg force-pushed the refactor/storage-redesign branch from c2b3225 to 37dc8be Compare June 19, 2023 12:11
src/PRBProxy.sol Outdated Show resolved Hide resolved
Copy link
Collaborator

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good

A small thing but I think this should be updated:

- Rename `PRBProxyHelpers` to `PRBProxyAnnex` ([#96](https://github.com/PaulRBerg/prb-proxy/pull/96)) (@PaulRBerg)

@PaulRBerg
Copy link
Owner Author

Thanks for the review, both!

@andreivladbrg - the CHANGELOG should remain unchanged because its purpose is to document the historical evolution of the proxy.

@PaulRBerg PaulRBerg force-pushed the refactor/storage-redesign branch from 7a56a9a to 27479f6 Compare June 22, 2023 10:31
@PaulRBerg PaulRBerg force-pushed the feat/get-proxy branch 2 times, most recently from 3c7f207 to 3ee2fc3 Compare June 23, 2023 12:09
@PaulRBerg PaulRBerg force-pushed the refactor/storage-redesign branch 2 times, most recently from a060837 to 3c58593 Compare June 24, 2023 08:57
feat: add "getPluginByProxy" getter
feat: add "getPermissionByProxy" getter
perf: optimize "execute"
refactor: delete annex
refactor: delete 'system' scripts
refactor: get rid of proxy storage
refactor: rename getters
refactor: rename "selector" param to "method"
test: fix events and re-enable tests
test: update tests in light of new contract API
@PaulRBerg PaulRBerg force-pushed the refactor/storage-redesign branch from 3c58593 to 83b4993 Compare June 24, 2023 09:08
@PaulRBerg PaulRBerg changed the base branch from feat/get-proxy to main June 28, 2023 18:50
@PaulRBerg PaulRBerg merged commit 25ddbae into main Jun 28, 2023
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.

3 participants