refactor: upgradeable adapters #122

0xDiscotech · 2024-07-25T16:22:27Z

🤖 Linear

…pter * feat: add initialize and authorize upgrade function on universal adapter * refactor: update deployment flow on factory and l2 deploy contracts * chore: update unit tests setup to deploy proxy and implementation

linear · 2024-07-25T16:22:34Z

OPT-168 Make adapters upgradeable (again)

Talked to circle and despite us following their process, they want the adapters to be upgradeable incase something related to their process was to ever change.

😞

excaliborr · 2024-07-25T17:39:21Z

src/contracts/L1OpUSDCBridgeAdapter.sol

@@ -42,15 +42,13 @@ contract L1OpUSDCBridgeAdapter is IL1OpUSDCBridgeAdapter, OpUSDCBridgeAdapter {
 * @param _usdc The address of the USDC Contract to be used by the adapter
 * @param _messenger The address of the L1 messenger
 * @param _linkedAdapter The address of the linked adapter
- * @param _owner The address of the owner of the contract
 * @dev The constructor is only used to initialize the OpUSDCBridgeAdapter immutable variables
 */
 constructor(
 address _usdc,


Make sure this contract inherits the UUPSUpgradeable from openzeppelin-upgradeable

or does it not need it because we inherit in the base contract?

Yeah it is inherited on the base contract

excaliborr · 2024-07-25T17:40:19Z

src/contracts/L2OpUSDCBridgeAdapter.sol

@@ -30,7 +30,7 @@ contract L2OpUSDCBridgeAdapter is IL2OpUSDCBridgeAdapter, OpUSDCBridgeAdapter {
 bytes4 internal constant _UPGRADE_TO_AND_CALL_SELECTOR = 0x4f1ef286;

 /// @inheritdoc IL2OpUSDCBridgeAdapter
- FallbackProxyAdmin public immutable FALLBACK_PROXY_ADMIN;
+ FallbackProxyAdmin public FALLBACK_PROXY_ADMIN;


This is no longer immutable, just change it to fallback_proxy_admin, wdyt?

Because it behaves as immutable, just that I need to set it on the initialize() function instead of on constructor. I added a note and the ignore flag.
Wdyt?

excaliborr · 2024-07-25T17:41:33Z

src/contracts/L2OpUSDCBridgeAdapter.sol

- FALLBACK_PROXY_ADMIN = new FallbackProxyAdmin(_usdc);
- }
+ address _linkedAdapter
+ ) OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter) {}


be sure to add __gap variable to reserve slots for new implementations on all three contracts

yes, good catch!

uint256[50] private __gap; seems enough, right?

yea should be fine

* chore: polish tests and add missing ones

* fix: deploy all contracts integration test

hexshire · 2024-07-26T03:30:59Z

src/contracts/universal/OpUSDCBridgeAdapter.sol

- address _owner
- ) Ownable(_owner) EIP712('OpUSDCBridgeAdapter', '1.0.0') {
+ // solhint-disable-next-line no-unused-vars
+ constructor(address _usdc, address _messenger, address _linkedAdapter) EIP712('OpUSDCBridgeAdapter', '1.0.0') {


Use EIP712Upgradeable.sol

very nice catch

hexshire · 2024-07-26T03:32:27Z

src/contracts/universal/OpUSDCBridgeAdapter.sol

+ * implementation contract
+ */
+ function initialize(address _owner) public virtual initializer {
+ __Ownable_init(_owner);


I think you will need to add this

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/f1b3e103cd6d48861e71357a7ac32b416c1b066f/contracts/utils/cryptography/EIP712Upgradeable.sol#L68

hexshire · 2024-07-26T03:35:29Z

src/contracts/universal/OpUSDCBridgeAdapter.sol

+ /**
+ * @notice Checks the caller is the owner to authorize the upgrade
+ */
+ function _authorizeUpgrade(address) internal virtual override onlyOwner {}


hexshire · 2024-07-26T03:37:51Z

test/unit/L1OpUSDCBridgeAdapter.t.sol

@@ -1,17 +1,19 @@
 pragma solidity ^0.8.25;

 import {Ownable} from '@openzeppelin/contracts/access/Ownable.sol';
+import {ERC1967Proxy} from '@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol';
+import {IL1OpUSDCBridgeAdapter, L1OpUSDCBridgeAdapter} from 'contracts/L1OpUSDCBridgeAdapter.sol';


excaliborr · 2024-07-26T15:53:50Z

src/contracts/L1OpUSDCBridgeAdapter.sol

- ) OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter, _owner) {}
+ address _linkedAdapter
+ ) OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter) {}
+


can we add the __gap to the L1 and L2 adapters as well, it will make maintanence should a chain op ever want to upgrade much easier

0xDiscotech · 2024-07-26T16:28:10Z

Deployments:

L1 factory: 0xC6570909811191DAfEd6a7775d6363662540F5E2

op:
  L1 Adapter: 0xfAf1Bb6E2f8007A51fBBA0AeFDE7631c175a1aBC
  L2 Factory: 0xa16243fF1BCB12b2627db3755FC0863D54a1bFD9
  L2 Adapter: 0x764057565403b23B5BC7FE920457870Df4348374

base:
  L1 Adapter: 0x9EC67f3F353a4B3123E93399725D4D1187Ab4A29
  L2 Factory: 0x4BA083dbf1Fd3e3c9b42BfF4F344e0dBeBb47337
  L2 Adapter: 0xD6Ebca002058c0E5d7Dcdf2ca6ca8a9C274ACA24

For OP

L1 Adapter owner:
cast call 0xfAf1Bb6E2f8007A51fBBA0AeFDE7631c175a1aBC 'owner()(address)' --rpc-url wss://ethereum-sepolia-rpc.publicnode.com -> 0x8421D6D2253d3f8e25586Aa6692b1Bf591da3779 ✅

L2 Adapter owner:
cast call 0x764057565403b23B5BC7FE920457870Df4348374 "owner()(address)" --rpc-url https://sepolia.optimism.io -> 0x8421D6D2253d3f8e25586Aa6692b1Bf591da3779 ✅

Fallback proxy admin:
cast call 0x764057565403b23B5BC7FE920457870Df4348374 "FALLBACK_PROXY_ADMIN()(address)" --rpc-url https://sepolia.optimism.io -> 0xA6abC0390c9aD135025b98a69d9764FBEdE7D651

USDC:
cast call 0x764057565403b23B5BC7FE920457870Df4348374 "USDC()(address)" --rpc-url https://sepolia.optimism.io -> 0x77ac613B82b30fAc7d545E2D62723c4ac3E4d5c4 ✅

USDC owner:
cast call 0x764057565403b23B5BC7FE920457870Df4348374 "owner()(address)" --rpc-url https://sepolia.optimism.io -> 0x8421D6D2253d3f8e25586Aa6692b1Bf591da3779

USDC admin:
cast call 0x77ac613B82b30fAc7d545E2D62723c4ac3E4d5c4 "admin()(address)" --rpc-url https://sepolia.optimism.io -> 0xA6abC0390c9aD135025b98a69d9764FBEdE7D651 (fallback proxy admin) ✅

hexshire · 2024-07-27T04:50:56Z

src/contracts/L2OpUSDCDeploy.sol

+ // Deploy L2 Adapter implementation and proxy, initializing it with the owner
+ address _l2AdapterImpl = address(new L2OpUSDCBridgeAdapter(_usdcProxy, _L2_MESSENGER, _l1Adapter));
+ address _l2Adapter =
+  address(new ERC1967Proxy(_l2AdapterImpl, abi.encodeCall(OpUSDCBridgeAdapter.initialize, _l2AdapterOwner)));


I think this could be a problem when the contracts are verfied on etherscan, the name of the contract will be ERC1967Proxy ... I would wrap the ERC1967Proxy into something like contact AdapterProxy is ERC1967Proxy

If we arent changing anything lets leave it as is, circle uses custom location for their proxy, we dont, it will be easier to read since we dont change anything logic wise

Closes OPT-168

0xDiscotech added 2 commits July 25, 2024 00:44

feat: add unit tests for upgradeable features on universal contract

1541ad2

0xDiscotech self-assigned this Jul 25, 2024

0xDiscotech changed the base branch from dev to audit/spearbit July 25, 2024 16:22

fix: deploy fallback proxy admin on initialize function

e0e4e93

excaliborr reviewed Jul 25, 2024

View reviewed changes

0xDiscotech added 5 commits July 25, 2024 18:10

feat: add gap

3e77b1d

* chore: polish tests and add missing ones

fix: l1 factory unit tests

954912c

fix: increment deployments salt counter by 2 on l1 factory

4001a6e

* fix: deploy all contracts integration test

Merge branch 'audit/spearbit' into refactor/upgradeable-adapters

2d5a201

fix: empty expect revert due to possible foundry bug

523897d

0xDiscotech requested a review from hexshire July 26, 2024 03:14

0xDiscotech marked this pull request as ready for review July 26, 2024 03:14

hexshire reviewed Jul 26, 2024

View reviewed changes

0xDiscotech added 3 commits July 26, 2024 00:47

fix: lint

d4d0ba0

feat: add eip712 upgradeable

5ae1002

fix: lint

d5d49aa

0xDiscotech requested review from excaliborr and hexshire July 26, 2024 15:21

excaliborr reviewed Jul 26, 2024

View reviewed changes

feat: add gap variables on l1 and l2 adapters

3d5efec

excaliborr approved these changes Jul 26, 2024

View reviewed changes

hexshire reviewed Jul 27, 2024

View reviewed changes

hexshire approved these changes Jul 27, 2024

View reviewed changes

excaliborr merged commit 601ec75 into audit/spearbit Jul 29, 2024
4 checks passed

excaliborr deleted the refactor/upgradeable-adapters branch July 29, 2024 11:50

hexshire pushed a commit that referenced this pull request Jul 29, 2024

refactor: upgradeable adapters (#122)

6f2ef30

Closes OPT-168

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: upgradeable adapters #122

refactor: upgradeable adapters #122

0xDiscotech commented Jul 25, 2024

linear bot commented Jul 25, 2024

excaliborr Jul 25, 2024

excaliborr Jul 25, 2024

0xDiscotech Jul 25, 2024

excaliborr Jul 25, 2024

hexshire Jul 26, 2024

0xDiscotech Jul 26, 2024

excaliborr Jul 25, 2024

0xDiscotech Jul 25, 2024

0xDiscotech Jul 25, 2024

excaliborr Jul 25, 2024

hexshire Jul 26, 2024 •

edited

Loading

0xDiscotech Jul 26, 2024

hexshire Jul 26, 2024

hexshire Jul 26, 2024

hexshire Jul 26, 2024

excaliborr Jul 26, 2024

0xDiscotech commented Jul 26, 2024 •

edited

Loading

hexshire Jul 27, 2024

excaliborr Jul 29, 2024

refactor: upgradeable adapters #122

refactor: upgradeable adapters #122

Conversation

0xDiscotech commented Jul 25, 2024

🤖 Linear

linear bot commented Jul 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hexshire Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xDiscotech commented Jul 26, 2024 • edited Loading

For OP

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hexshire Jul 26, 2024 •

edited

Loading

0xDiscotech commented Jul 26, 2024 •

edited

Loading