Use upgradeable proxies for tokens and collections#128
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR replaces ERC-1167 minimal proxy clones with upgradeable ERC-1967 proxies for both fungible tokens and NFT collections, enabling future contract upgrades through the ProxyAdmin governance mechanism.
Key Changes:
- Replaces OpenZeppelin's
Cloneslibrary withCreate2for deploying upgradeable proxies - Removes
payablemodifiers from proxy-related functions that don't need to accept ETH - Updates address prediction logic to account for proxy constructor parameters
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| contracts/test/AddressPrediction.t.sol | Adds tests verifying address prediction for upgradeable proxy deployments |
| contracts/src/libraries/Proxy.sol | Removes unnecessary payable modifier from upgradeToAndCall function |
| contracts/src/L2/ProxyAdmin.sol | Removes payable modifiers and type qualifiers from admin functions |
| contracts/src/FixedFungibleProtocolHandler.sol | Migrates from Clones to upgradeable Proxy pattern for token deployments |
| contracts/src/CollectionsProtocolHandler.sol | Migrates from Clones to upgradeable Proxy pattern for collection deployments |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Deploy a Proxy via CREATE2 with this handler as temporary admin for initialization | ||
| Proxy tokenProxy = new Proxy{salt: tickKey}(address(this)); |
There was a problem hiding this comment.
The handler contract temporarily assumes admin privileges during initialization. If this function can be called multiple times or reentered before admin transfer, it creates a window where the handler has elevated privileges. Consider adding reentrancy protection or documenting this assumption explicitly.
| // Deploy ERC721 proxy with CREATE2 using collectionId as salt for deterministic address | ||
| Proxy collectionProxy = new Proxy{salt: collectionId}(address(this)); |
There was a problem hiding this comment.
The handler contract temporarily assumes admin privileges during initialization. If this function can be called multiple times or reentered before admin transfer, it creates a window where the handler has elevated privileges. Consider adding reentrancy protection or documenting this assumption explicitly.
|
bugbot run |
🚨 Bugbot couldn't runSomething went wrong. Try again by commenting "Cursor review" or "bugbot run", or contact support (requestId: serverGenReqId_38fce432-6fc5-4367-b94e-eb7a4d976b60). |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| { | ||
| Proxy(_proxy).upgradeToAndCall{ value: msg.value }(_implementation, _data); | ||
| ) external onlyOwner { | ||
| Proxy(_proxy).upgradeToAndCall(_implementation, _data); |
There was a problem hiding this comment.
Bug: Proxy Upgrades Fail to Forward ETH
The upgradeAndCall functions in ProxyAdmin and Proxy lost their payable modifier and msg.value forwarding. This prevents sending ETH to payable initialization functions during proxy upgrades, removing previously supported functionality.
Note
Replace clone-based deployments with upgradeable CREATE2 proxies for tokens and collections, update deterministic address prediction, and adjust Proxy/ProxyAdmin APIs with a new test.
contracts/src/CollectionsProtocolHandler.sol):Cloneswith CREATE2-deployedProxy; initialize viaupgradeToAndCalland transfer admin toPredeploys.PROXY_ADMIN.collectionsImplementation; events and state now reference proxy address.predictCollectionAddressto useCreate2.computeAddresswithProxycreation code.contracts/src/FixedFungibleProtocolHandler.sol):Cloneswith CREATE2-deployedProxy; initialize viaupgradeToAndCalland transfer admin toPredeploys.PROXY_ADMIN.fixedFungibleImplementation; events/state now reference proxy address.predictTokenAddressByTickto useCreate2.computeAddresswithProxycreation code.contracts/src/L2/ProxyAdmin.sol): unify function params toaddress(non-payable) and remove value forwarding inupgradeAndCall.contracts/src/libraries/Proxy.sol): removepayablefromupgradeToAndCallsignature.contracts/test/AddressPrediction.t.solto assert predicted proxy addresses match deployed addresses for both handlers.Written by Cursor Bugbot for commit 79e6dc0. This will update automatically on new commits. Configure here.