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

Complete unit test coverage for upgradeable renouncable proxy #112

Merged
merged 9 commits into from
Nov 25, 2024

Conversation

roleengineer
Copy link
Contributor

Two mock contracts were implemented to test the behavior of the upgradeable renounceable proxy under different scenarios:

  • Mock with Selector Clashes: This mock simulates cases where there are selector clashes between the proxy and implementation, allowing us to test how the proxy handles such situations.
  • Mock with Stateful Implementation: This mock simulates an implementation which works with state, allowing us to evaluate how the proxy handles state.

Tested:

  • Proxy's Native Functions: all native proxy functions to ensure their correctness in various scenarios, including when selector clashes or a stateful implementation are present.
  • Behavior with Clashing Implementations: how the proxy behaves when the implementation contract has selector clashes, leading to potential vulnerabilities or unexpected behavior.
  • Behavior with Stateful Implementations: how the proxy behaves with a stateful implementation, ensuring it handles state correctly.

During testing, a serious issue was discovered where the proxy's native functionality can be bypassed by a malicious implementation. Specifically:

Proxy Admin & Implementation: any address (not just the proxy admin) can change the proxy's admin and implementation. This is a critical vulnerability, if we have a constraint that only proxy native functionality can change admin and implementation.

Given the findings, conclusion is that fixes are required in the proxy contract to address these security issues and ensure the integrity of the upgradeable renouncable proxy functionality.

@roleengineer roleengineer self-assigned this Nov 25, 2024
@roleengineer
Copy link
Contributor Author

solves #90

@roleengineer roleengineer linked an issue Nov 25, 2024 that may be closed by this pull request
Copy link
Member

@benjaminbollen benjaminbollen left a comment

Choose a reason for hiding this comment

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

Great work!

I left small questions, you can see whether they need resolving or not; happy to merge following your response

import {ERC1967Utils} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Utils.sol";
import {
UpgradeableRenounceableProxy, IUpgradeableRenounceableProxy
} from "src/groups/UpgradeableRenounceableProxy.sol";
Copy link
Member

Choose a reason for hiding this comment

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

I can't believe I never realised using an absolute path is so much more readable and better. Internal style-guide updated !

…eableProxy.t.sol


improve comment clarity for internal function _upgradeToAndCall
@benjaminbollen
Copy link
Member

both questions resolved. LGTM!

@benjaminbollen benjaminbollen merged commit 097c254 into beta Nov 25, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

write additional unit tests for proxy
2 participants