From e60e43e577a4a2f029c524ac9360126586967672 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Thu, 25 Jul 2024 00:44:53 -0300 Subject: [PATCH 01/11] feat: inherit uups and ownable upgradeable contracts on universal adapter * 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 --- src/contracts/L1OpUSDCBridgeAdapter.sol | 6 ++---- src/contracts/L1OpUSDCFactory.sol | 17 ++++++++++------- src/contracts/L2OpUSDCBridgeAdapter.sol | 5 ++--- src/contracts/L2OpUSDCDeploy.sol | 12 +++++++----- src/contracts/universal/OpUSDCBridgeAdapter.sol | 15 +++++++++++---- test/unit/L1OpUSDCBridgeAdapter.t.sol | 12 ++++++++---- test/unit/L2OpUSDCBridgeAdapter.t.sol | 13 +++++++++---- test/unit/OpUSDCBridgeAdapter.t.sol | 2 +- 8 files changed, 50 insertions(+), 32 deletions(-) diff --git a/src/contracts/L1OpUSDCBridgeAdapter.sol b/src/contracts/L1OpUSDCBridgeAdapter.sol index 42a32802..ec763abf 100644 --- a/src/contracts/L1OpUSDCBridgeAdapter.sol +++ b/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, address _messenger, - address _linkedAdapter, - address _owner - ) OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter, _owner) {} + address _linkedAdapter + ) OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter) {} /*/////////////////////////////////////////////////////////////// MIGRATION diff --git a/src/contracts/L1OpUSDCFactory.sol b/src/contracts/L1OpUSDCFactory.sol index 41071dcc..5a9ecb1b 100644 --- a/src/contracts/L1OpUSDCFactory.sol +++ b/src/contracts/L1OpUSDCFactory.sol @@ -1,12 +1,13 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.25; +import {ERC1967Proxy} from '@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol'; import {L1OpUSDCBridgeAdapter} from 'contracts/L1OpUSDCBridgeAdapter.sol'; import {IL1OpUSDCFactory} from 'interfaces/IL1OpUSDCFactory.sol'; import {IL2OpUSDCDeploy} from 'interfaces/IL2OpUSDCDeploy.sol'; - import {IUSDC} from 'interfaces/external/IUSDC.sol'; import {CrossChainDeployments} from 'libraries/CrossChainDeployments.sol'; +import {OpUSDCBridgeAdapter} from 'src/contracts/universal/OpUSDCBridgeAdapter.sol'; /** * @title L1OpUSDCFactory @@ -30,8 +31,8 @@ contract L1OpUSDCFactory is IL1OpUSDCFactory { /// @dev Used to check the first init tx doesn't match it since it is already defined in the L2 factory contract bytes4 internal constant _INITIALIZE_SELECTOR = 0x3357162b; - /// @notice The L2 Adapter is the second contract to be deployed on the L2 factory so its nonce is 2 - uint256 internal constant _L2_ADAPTER_DEPLOYMENT_NONCE = 2; + /// @notice The L2 Adapter proxy is the third of the L2 deployments so at that moment the nonce is 3 + uint256 internal constant _L2_ADAPTER_DEPLOYMENT_NONCE = 3; /// @inheritdoc IL1OpUSDCFactory IUSDC public immutable USDC; @@ -87,8 +88,8 @@ contract L1OpUSDCFactory is IL1OpUSDCFactory { // Update the salt counter so the L2 factory is deployed with a different salt to a different address and get it uint256 _currentNonce = ++deploymentsSaltCounter; - // Precalculate the l1 adapter - _l1Adapter = CrossChainDeployments.precalculateCreateAddress(address(this), _currentNonce); + // Precalculate the l1 adapter proxy address + _l1Adapter = CrossChainDeployments.precalculateCreateAddress(address(this), _currentNonce + 1); // Get the L1 USDC naming and decimals to ensure they are the same on the L2, guaranteeing the same standard IL2OpUSDCDeploy.USDCInitializeData memory _usdcInitializeData = IL2OpUSDCDeploy.USDCInitializeData( @@ -112,8 +113,10 @@ contract L1OpUSDCFactory is IL1OpUSDCFactory { // Precalculate the L2 adapter address _l2Adapter = CrossChainDeployments.precalculateCreateAddress(_l2Factory, _L2_ADAPTER_DEPLOYMENT_NONCE); - // Deploy the L1 adapter - address(new L1OpUSDCBridgeAdapter(address(USDC), _l1Messenger, _l2Adapter, _l1AdapterOwner)); + + // Deploy L1 Adapter implementation and proxy, initializing it with the owner + address _l1AdapterImpl = address(new L1OpUSDCBridgeAdapter(address(USDC), _l1Messenger, _l2Adapter)); + new ERC1967Proxy(_l1AdapterImpl, abi.encodeCall(OpUSDCBridgeAdapter.initialize, _l1AdapterOwner)); emit ProtocolDeployed(_l1Adapter, _l2Factory, _l2Adapter); } diff --git a/src/contracts/L2OpUSDCBridgeAdapter.sol b/src/contracts/L2OpUSDCBridgeAdapter.sol index db5ab3cf..ab0fb8f8 100644 --- a/src/contracts/L2OpUSDCBridgeAdapter.sol +++ b/src/contracts/L2OpUSDCBridgeAdapter.sol @@ -59,9 +59,8 @@ contract L2OpUSDCBridgeAdapter is IL2OpUSDCBridgeAdapter, OpUSDCBridgeAdapter { constructor( address _usdc, address _messenger, - address _linkedAdapter, - address _owner - ) OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter, _owner) { + address _linkedAdapter + ) OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter) { FALLBACK_PROXY_ADMIN = new FallbackProxyAdmin(_usdc); } /* solhint-enable no-unused-vars */ diff --git a/src/contracts/L2OpUSDCDeploy.sol b/src/contracts/L2OpUSDCDeploy.sol index b60f5701..225e90eb 100644 --- a/src/contracts/L2OpUSDCDeploy.sol +++ b/src/contracts/L2OpUSDCDeploy.sol @@ -1,10 +1,12 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.25; +import {ERC1967Proxy} from '@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol'; import {L2OpUSDCBridgeAdapter} from 'contracts/L2OpUSDCBridgeAdapter.sol'; import {USDC_PROXY_CREATION_CODE} from 'contracts/utils/USDCProxyCreationCode.sol'; import {IL2OpUSDCDeploy} from 'interfaces/IL2OpUSDCDeploy.sol'; import {IUSDC} from 'interfaces/external/IUSDC.sol'; +import {OpUSDCBridgeAdapter} from 'src/contracts/universal/OpUSDCBridgeAdapter.sol'; /** * @title L2OpUSDCDeploy @@ -37,13 +39,13 @@ contract L2OpUSDCDeploy is IL2OpUSDCDeploy { // Deploy USDC proxy bytes memory _usdcProxyCArgs = abi.encode(_usdcImplAddr); bytes memory _usdcProxyInitCode = bytes.concat(USDC_PROXY_CREATION_CODE, _usdcProxyCArgs); - (address _usdcProxy) = _deployCreate(_usdcProxyInitCode); + address _usdcProxy = _deployCreate(_usdcProxyInitCode); emit USDCProxyDeployed(_usdcProxy); - // Deploy L2 Adapter - bytes memory _l2AdapterCArgs = abi.encode(_usdcProxy, _L2_MESSENGER, _l1Adapter, _l2AdapterOwner); - bytes memory _l2AdapterInitCode = bytes.concat(type(L2OpUSDCBridgeAdapter).creationCode, _l2AdapterCArgs); - (address _l2Adapter) = _deployCreate(_l2AdapterInitCode); + // 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))); emit L2AdapterDeployed(_l2Adapter); // Deploy the FallbackProxyAdmin internally in the L2 Adapter to keep it unique diff --git a/src/contracts/universal/OpUSDCBridgeAdapter.sol b/src/contracts/universal/OpUSDCBridgeAdapter.sol index 7c10fc77..a8defae0 100644 --- a/src/contracts/universal/OpUSDCBridgeAdapter.sol +++ b/src/contracts/universal/OpUSDCBridgeAdapter.sol @@ -1,12 +1,13 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.25; -import {Ownable} from '@openzeppelin/contracts/access/Ownable.sol'; +import {OwnableUpgradeable} from '@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol'; +import {UUPSUpgradeable} from '@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol'; import {MessageHashUtils} from '@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol'; import {SignatureChecker} from '@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol'; import {IOpUSDCBridgeAdapter} from 'interfaces/IOpUSDCBridgeAdapter.sol'; -abstract contract OpUSDCBridgeAdapter is IOpUSDCBridgeAdapter, Ownable { +abstract contract OpUSDCBridgeAdapter is UUPSUpgradeable, OwnableUpgradeable, IOpUSDCBridgeAdapter { using MessageHashUtils for bytes32; using SignatureChecker for address; @@ -30,13 +31,17 @@ abstract contract OpUSDCBridgeAdapter is IOpUSDCBridgeAdapter, Ownable { * @param _usdc The address of the USDC Contract to be used by the adapter * @param _messenger The address of the messenger contract * @param _linkedAdapter The address of the linked adapter - * @param _owner The address of the owner of the contract */ // solhint-disable-next-line no-unused-vars - constructor(address _usdc, address _messenger, address _linkedAdapter, address _owner) Ownable(_owner) { + constructor(address _usdc, address _messenger, address _linkedAdapter) { USDC = _usdc; MESSENGER = _messenger; LINKED_ADAPTER = _linkedAdapter; + _disableInitializers(); + } + + function initialize(address _owner) public initializer { + __Ownable_init(_owner); } /*/////////////////////////////////////////////////////////////// @@ -104,4 +109,6 @@ abstract contract OpUSDCBridgeAdapter is IOpUSDCBridgeAdapter, Ownable { if (!_signer.isValidSignatureNow(_messageHash, _signature)) revert IOpUSDCBridgeAdapter_InvalidSignature(); } + + function _authorizeUpgrade(address) internal virtual override onlyOwner {} } diff --git a/test/unit/L1OpUSDCBridgeAdapter.t.sol b/test/unit/L1OpUSDCBridgeAdapter.t.sol index 23a6c77b..045b07a7 100644 --- a/test/unit/L1OpUSDCBridgeAdapter.t.sol +++ b/test/unit/L1OpUSDCBridgeAdapter.t.sol @@ -1,17 +1,18 @@ 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'; import {IOpUSDCBridgeAdapter} from 'interfaces/IOpUSDCBridgeAdapter.sol'; +import {OpUSDCBridgeAdapter} from 'src/contracts/universal/OpUSDCBridgeAdapter.sol'; import {Helpers} from 'test/utils/Helpers.sol'; contract ForTestL1OpUSDCBridgeAdapter is L1OpUSDCBridgeAdapter { constructor( address _usdc, address _messenger, - address _linkedAdapter, - address _owner - ) L1OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter, _owner) {} + address _linkedAdapter + ) L1OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter) {} function forTest_setBurnAmount(uint256 _amount) external { burnAmount = _amount; @@ -58,7 +59,10 @@ abstract contract Base is Helpers { (_signerAd, _signerPk) = makeAddrAndKey('signer'); vm.etch(_messenger, 'xDomainMessageSender'); - adapter = new ForTestL1OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter, _owner); + address _adapterImpl = address(new ForTestL1OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter)); + adapter = ForTestL1OpUSDCBridgeAdapter( + address(new ERC1967Proxy(_adapterImpl, abi.encodeCall(OpUSDCBridgeAdapter.initialize, _owner))) + ); } } diff --git a/test/unit/L2OpUSDCBridgeAdapter.t.sol b/test/unit/L2OpUSDCBridgeAdapter.t.sol index 7ce42c0b..7d44bb44 100644 --- a/test/unit/L2OpUSDCBridgeAdapter.t.sol +++ b/test/unit/L2OpUSDCBridgeAdapter.t.sol @@ -1,8 +1,10 @@ pragma solidity ^0.8.25; import {Ownable} from '@openzeppelin/contracts/access/Ownable.sol'; +import {ERC1967Proxy} from '@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol'; import {L2OpUSDCBridgeAdapter} from 'contracts/L2OpUSDCBridgeAdapter.sol'; import {IOpUSDCBridgeAdapter} from 'interfaces/IOpUSDCBridgeAdapter.sol'; +import {OpUSDCBridgeAdapter} from 'src/contracts/universal/OpUSDCBridgeAdapter.sol'; import {Helpers} from 'test/utils/Helpers.sol'; contract ForTestL2OpUSDCBridgeAdapter is L2OpUSDCBridgeAdapter { @@ -12,9 +14,8 @@ contract ForTestL2OpUSDCBridgeAdapter is L2OpUSDCBridgeAdapter { constructor( address _usdc, address _messenger, - address _linkedAdapter, - address _owner - ) L2OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter, _owner) {} + address _linkedAdapter + ) L2OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter) {} function forTest_setIsMessagingDisabled() external { isMessagingDisabled = true; @@ -54,7 +55,11 @@ abstract contract Base is Helpers { function setUp() public virtual { (_signerAd, _signerPk) = makeAddrAndKey('signer'); - adapter = new ForTestL2OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter, _owner); + + address _adapterImpl = address(new ForTestL2OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter)); + adapter = ForTestL2OpUSDCBridgeAdapter( + address(new ERC1967Proxy(_adapterImpl, abi.encodeCall(OpUSDCBridgeAdapter.initialize, _owner))) + ); } } diff --git a/test/unit/OpUSDCBridgeAdapter.t.sol b/test/unit/OpUSDCBridgeAdapter.t.sol index 2b3d8f33..826891db 100644 --- a/test/unit/OpUSDCBridgeAdapter.t.sol +++ b/test/unit/OpUSDCBridgeAdapter.t.sol @@ -12,7 +12,7 @@ contract ForTestOpUSDCBridgeAdapter is OpUSDCBridgeAdapter { address _messenger, address _linkedAdapter, address _owner - ) OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter, _owner) {} + ) OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter) {} function receiveMessage(address _user, uint256 _amount) external override {} From 1541ad2d0da20739746d45d8169f819fc0f5f3cf Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Thu, 25 Jul 2024 13:20:00 -0300 Subject: [PATCH 02/11] feat: add unit tests for upgradeable features on universal contract --- test/unit/OpUSDCBridgeAdapter.t.sol | 44 +++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 5 deletions(-) diff --git a/test/unit/OpUSDCBridgeAdapter.t.sol b/test/unit/OpUSDCBridgeAdapter.t.sol index 826891db..2d2ef3ba 100644 --- a/test/unit/OpUSDCBridgeAdapter.t.sol +++ b/test/unit/OpUSDCBridgeAdapter.t.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.25; +import {ERC1967Proxy} from '@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol'; import {MessageHashUtils} from '@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol'; import {OpUSDCBridgeAdapter} from 'contracts/universal/OpUSDCBridgeAdapter.sol'; import {Test} from 'forge-std/Test.sol'; @@ -10,8 +11,7 @@ contract ForTestOpUSDCBridgeAdapter is OpUSDCBridgeAdapter { constructor( address _usdc, address _messenger, - address _linkedAdapter, - address _owner + address _linkedAdapter ) OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter) {} function receiveMessage(address _user, uint256 _amount) external override {} @@ -41,15 +41,19 @@ abstract contract Base is Test { address internal _usdc = makeAddr('opUSDC'); address internal _owner = makeAddr('owner'); address internal _linkedAdapter = makeAddr('linkedAdapter'); + address internal _messenger = makeAddr('messenger'); + address internal _adapterImpl; address internal _signerAd; uint256 internal _signerPk; address internal _notSignerAd; uint256 internal _notSignerPk; - address internal _messenger = makeAddr('messenger'); function setUp() public virtual { (_signerAd, _signerPk) = makeAddrAndKey('signer'); - adapter = new ForTestOpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter, _owner); + _adapterImpl = address(new ForTestOpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter)); + adapter = ForTestOpUSDCBridgeAdapter( + address(new ERC1967Proxy(_adapterImpl, abi.encodeCall(OpUSDCBridgeAdapter.initialize, _owner))) + ); } } @@ -59,8 +63,38 @@ contract OpUSDCBridgeAdapter_Unit_Constructor is Base { */ function test_constructorParams() public view { assertEq(adapter.USDC(), _usdc, 'USDC should be set to the provided address'); + assertEq(adapter.MESSENGER(), _messenger, 'Messenger should be set to the provided address'); assertEq(adapter.LINKED_ADAPTER(), _linkedAdapter, 'Linked adapter should be set to the provided address'); - assertEq(adapter.owner(), _owner, 'Owner should be set to the provided address'); + } +} + +contract OpUSDCBridgeAdapter_Unit_Initialize is Base { + error InvalidInitialization(); + + /** + * @notice Check that the initialize function works as expected + * @dev Needs to be checked on the proxy since the initialize function is disabled on the implementation + */ + function test_initialize(address _owner) public { + // Deploy a proxy contract setting the , and call the initialize function on it to set the owner + ForTestOpUSDCBridgeAdapter _newAdapter = ForTestOpUSDCBridgeAdapter( + address(new ERC1967Proxy(address(_adapterImpl), abi.encodeCall(OpUSDCBridgeAdapter.initialize, _owner))) + ); + + // Assert + assertEq(_newAdapter.owner(), _owner, 'Owner should be set to the provided address'); + } + /** + * @notice Check that the initialize function reverts if it was already called + */ + + function test_revertIfAlreadyInitialize(address _sender, address _owner) public { + // Expect revert with `InvalidInitialization` error + vm.expectRevert(InvalidInitialization.selector); + + // Execute + vm.prank(_sender); + adapter.initialize(_owner); } } From e0e4e93905814de3be665f1cc1b394c2a5005df0 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Thu, 25 Jul 2024 14:24:21 -0300 Subject: [PATCH 03/11] fix: deploy fallback proxy admin on initialize function --- src/contracts/L2OpUSDCBridgeAdapter.sol | 19 +++++++++++++++---- .../universal/OpUSDCBridgeAdapter.sol | 17 +++++++++++++---- src/interfaces/IL2OpUSDCBridgeAdapter.sol | 2 ++ 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/src/contracts/L2OpUSDCBridgeAdapter.sol b/src/contracts/L2OpUSDCBridgeAdapter.sol index ab0fb8f8..f83f93f4 100644 --- a/src/contracts/L2OpUSDCBridgeAdapter.sol +++ b/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; /// @inheritdoc IL2OpUSDCBridgeAdapter bool public isMessagingDisabled; @@ -60,9 +60,7 @@ contract L2OpUSDCBridgeAdapter is IL2OpUSDCBridgeAdapter, OpUSDCBridgeAdapter { address _usdc, address _messenger, address _linkedAdapter - ) OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter) { - FALLBACK_PROXY_ADMIN = new FallbackProxyAdmin(_usdc); - } + ) OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter) {} /* solhint-enable no-unused-vars */ /*/////////////////////////////////////////////////////////////// @@ -278,4 +276,17 @@ contract L2OpUSDCBridgeAdapter is IL2OpUSDCBridgeAdapter, OpUSDCBridgeAdapter { emit USDCFunctionSent(_selector); } + + /** + * @notice Sets the owner of the contract and deploys the `FallbackProxyAdmin` contract + * @param _owner The address of the owner + * @dev This function needs only used during the deployment of the proxy contract, and it is disabled for the + * implementation contract + * @dev The `FallbackProxyAdmin` contract is deployed since it can't be deployed on the constructor, otherwise, the + * owner would be set to the implementation contract address instead of the proxy one + */ + function initialize(address _owner) public virtual override initializer { + super.initialize(_owner); + FALLBACK_PROXY_ADMIN = new FallbackProxyAdmin(USDC); + } } diff --git a/src/contracts/universal/OpUSDCBridgeAdapter.sol b/src/contracts/universal/OpUSDCBridgeAdapter.sol index a8defae0..fe9a64d9 100644 --- a/src/contracts/universal/OpUSDCBridgeAdapter.sol +++ b/src/contracts/universal/OpUSDCBridgeAdapter.sol @@ -40,10 +40,6 @@ abstract contract OpUSDCBridgeAdapter is UUPSUpgradeable, OwnableUpgradeable, IO _disableInitializers(); } - function initialize(address _owner) public initializer { - __Ownable_init(_owner); - } - /*/////////////////////////////////////////////////////////////// MESSAGING ///////////////////////////////////////////////////////////////*/ @@ -98,6 +94,16 @@ abstract contract OpUSDCBridgeAdapter is UUPSUpgradeable, OwnableUpgradeable, IO userNonces[msg.sender][_nonce] = true; } + /** + * @notice Sets the owner of the contract + * @param _owner The address of the owner + * @dev This function needs only used during the deployment of the proxy contract, and it is disabled for the + * implementation contract + */ + function initialize(address _owner) public virtual initializer { + __Ownable_init(_owner); + } + /** * @notice Check the signature of a message * @param _signer the address that signed the message @@ -110,5 +116,8 @@ abstract contract OpUSDCBridgeAdapter is UUPSUpgradeable, OwnableUpgradeable, IO if (!_signer.isValidSignatureNow(_messageHash, _signature)) revert IOpUSDCBridgeAdapter_InvalidSignature(); } + /** + * @notice Checks the caller is the owner to authorize the upgrade + */ function _authorizeUpgrade(address) internal virtual override onlyOwner {} } diff --git a/src/interfaces/IL2OpUSDCBridgeAdapter.sol b/src/interfaces/IL2OpUSDCBridgeAdapter.sol index d75f04bb..0cb8432a 100644 --- a/src/interfaces/IL2OpUSDCBridgeAdapter.sol +++ b/src/interfaces/IL2OpUSDCBridgeAdapter.sol @@ -73,6 +73,8 @@ interface IL2OpUSDCBridgeAdapter { * such as mint and burn between others. Because of this, the FallbackProxyAdmin contract is used as a middleware, * being controlled by the L2OpUSDCBridgeAdapter contract and allowing to call the admin functions through it while * also being able to call the fallback function of the USDC proxy. + * @dev Declared with immutable notation even though it is not defined on the constructor because it is set on the + * `initialize` function which replicates the behavior of the constructor. */ // solhint-disable-next-line func-name-mixedcase function FALLBACK_PROXY_ADMIN() external view returns (FallbackProxyAdmin _fallbackProxyAdmin); From 3e77b1da43ede5a12c1c84a62619f33098562651 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Thu, 25 Jul 2024 18:10:25 -0300 Subject: [PATCH 04/11] feat: add gap * chore: polish tests and add missing ones --- .../universal/OpUSDCBridgeAdapter.sol | 3 + test/unit/L2OpUSDCBridgeAdapter.t.sol | 24 ++++- test/unit/L2OpUSDCDeploy.t.sol | 21 +++-- test/unit/OpUSDCBridgeAdapter.t.sol | 90 ++++++++++++------- 4 files changed, 96 insertions(+), 42 deletions(-) diff --git a/src/contracts/universal/OpUSDCBridgeAdapter.sol b/src/contracts/universal/OpUSDCBridgeAdapter.sol index fe9a64d9..550fb753 100644 --- a/src/contracts/universal/OpUSDCBridgeAdapter.sol +++ b/src/contracts/universal/OpUSDCBridgeAdapter.sol @@ -20,6 +20,9 @@ abstract contract OpUSDCBridgeAdapter is UUPSUpgradeable, OwnableUpgradeable, IO /// @inheritdoc IOpUSDCBridgeAdapter address public immutable MESSENGER; + /// @notice Reserve 50 storage slots to be safe on future upgrades + uint256[50] internal __gap; + /// @inheritdoc IOpUSDCBridgeAdapter mapping(address _user => mapping(uint256 _nonce => bool _used)) public userNonces; diff --git a/test/unit/L2OpUSDCBridgeAdapter.t.sol b/test/unit/L2OpUSDCBridgeAdapter.t.sol index 7d44bb44..03910c05 100644 --- a/test/unit/L2OpUSDCBridgeAdapter.t.sol +++ b/test/unit/L2OpUSDCBridgeAdapter.t.sol @@ -42,11 +42,12 @@ abstract contract Base is Helpers { address internal _user = makeAddr('user'); address internal _owner = makeAddr('owner'); - address internal _signerAd; - uint256 internal _signerPk; address internal _usdc = makeAddr('opUSDC'); address internal _messenger = makeAddr('messenger'); address internal _linkedAdapter = makeAddr('linkedAdapter'); + address internal _adapterImpl; + address internal _signerAd; + uint256 internal _signerPk; event MigratingToNative(address _messenger, address _roleCaller); event MessageSent(address _user, address _to, uint256 _amount, address _messenger, uint32 _minGasLimit); @@ -56,7 +57,7 @@ abstract contract Base is Helpers { function setUp() public virtual { (_signerAd, _signerPk) = makeAddrAndKey('signer'); - address _adapterImpl = address(new ForTestL2OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter)); + _adapterImpl = address(new ForTestL2OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter)); adapter = ForTestL2OpUSDCBridgeAdapter( address(new ERC1967Proxy(_adapterImpl, abi.encodeCall(OpUSDCBridgeAdapter.initialize, _owner))) ); @@ -925,3 +926,20 @@ contract L2OpUSDCBridgeAdapter_Unit_CallUsdcTransaction is Base { adapter.callUsdcTransaction(_data); } } + +contract L2OpUSDCBridgeAdapter_Unit_Initialize is Base { + /** + * @notice Check that the initialize function works as expected + * @dev Needs to be checked on the proxy since the initialize function is disabled on the implementation + */ + function test_initialize(address _owner) public { + // Deploy a proxy contract setting the , and call the initialize function on it to set the owner + ForTestL2OpUSDCBridgeAdapter _newAdapter = ForTestL2OpUSDCBridgeAdapter( + address(new ERC1967Proxy(address(_adapterImpl), abi.encodeCall(OpUSDCBridgeAdapter.initialize, _owner))) + ); + + // Assert + assertEq(_newAdapter.owner(), _owner, 'Owner should be set to the provided address'); + assertGt(address(_newAdapter.FALLBACK_PROXY_ADMIN()).code.length, 0, 'Fallback admin was not deployed'); + } +} diff --git a/test/unit/L2OpUSDCDeploy.t.sol b/test/unit/L2OpUSDCDeploy.t.sol index d95cf958..c5974256 100644 --- a/test/unit/L2OpUSDCDeploy.t.sol +++ b/test/unit/L2OpUSDCDeploy.t.sol @@ -2,6 +2,7 @@ pragma solidity 0.8.25; import {Ownable} from '@openzeppelin/contracts/access/Ownable.sol'; +import {ERC1967Utils} from '@openzeppelin/contracts/proxy/ERC1967/ERC1967Utils.sol'; import {L2OpUSDCDeploy} from 'contracts/L2OpUSDCDeploy.sol'; import {USDC_PROXY_CREATION_CODE} from 'contracts/utils/USDCProxyCreationCode.sol'; import {Test} from 'forge-std/Test.sol'; @@ -44,7 +45,7 @@ contract Base is Test, Helpers { address internal _l2AdapterOwner = makeAddr('l2AdapterOwner'); address internal _usdcImplAddr = makeAddr('usdcImpl'); uint256 internal _usdcProxyDeploymentNonce = 1; - uint256 internal _l2AdapterDeploymentNonce = 2; + uint256 internal _l2AdapterDeploymentNonce = 3; address internal _dummyContract; @@ -87,6 +88,8 @@ contract Base is Test, Helpers { } contract L2OpUSDCDeploy_Unit_Constructor is Base { + using ERC1967Utils for address; + event USDCImplementationDeployed(address _l2UsdcImplementation); event USDCProxyDeployed(address _l2UsdcProxy); event L2AdapterDeployed(address _l2Adapter); @@ -122,24 +125,24 @@ contract L2OpUSDCDeploy_Unit_Constructor is Base { // Calculate the usdc proxy address address _usdcProxy = _precalculateCreateAddress(address(factory), _usdcProxyDeploymentNonce); - // Calculate the l2 adapter address - address _l2Adapter = _precalculateCreateAddress(address(factory), _l2AdapterDeploymentNonce); + // Calculate the l2 adapter proxy address + address _l2AdapterProxy = _precalculateCreateAddress(address(factory), _l2AdapterDeploymentNonce); // Expect the adapter deployment event to be properly emitted vm.expectEmit(true, true, true, true); - emit L2AdapterDeployed(_l2Adapter); + emit L2AdapterDeployed(_l2AdapterProxy); // Execute vm.prank(_create2Deployer); new L2OpUSDCDeploy(_l1Adapter, _l2AdapterOwner, _usdcImplAddr, _usdcInitializeData, _emptyInitTxs); // Assert the adapter was deployed - assertGt(_l2Adapter.code.length, 0, 'L2 adapter was not deployed'); + assertGt(_l2AdapterProxy.code.length, 0, 'L2 adapter was not deployed'); // Check the constructor values were properly passed - assertEq(IOpUSDCBridgeAdapter(_l2Adapter).USDC(), _usdcProxy, 'USDC proxy was not set'); - assertEq(IOpUSDCBridgeAdapter(_l2Adapter).MESSENGER(), _l2Messenger, 'L2 messenger was not set'); - assertEq(IOpUSDCBridgeAdapter(_l2Adapter).LINKED_ADAPTER(), _l1Adapter, 'L1 factory was not set'); - assertEq(Ownable(_l2Adapter).owner(), _l2AdapterOwner, 'L2 adapter owner was not set'); + assertEq(IOpUSDCBridgeAdapter(_l2AdapterProxy).USDC(), _usdcProxy, 'USDC proxy was not set'); + assertEq(IOpUSDCBridgeAdapter(_l2AdapterProxy).MESSENGER(), _l2Messenger, 'L2 messenger was not set'); + assertEq(IOpUSDCBridgeAdapter(_l2AdapterProxy).LINKED_ADAPTER(), _l1Adapter, 'L1 factory was not set'); + assertEq(Ownable(_l2AdapterProxy).owner(), _l2AdapterOwner, 'L2 adapter owner was not set'); } /** diff --git a/test/unit/OpUSDCBridgeAdapter.t.sol b/test/unit/OpUSDCBridgeAdapter.t.sol index 2d2ef3ba..42a33d63 100644 --- a/test/unit/OpUSDCBridgeAdapter.t.sol +++ b/test/unit/OpUSDCBridgeAdapter.t.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.25; +import {OwnableUpgradeable} from '@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol'; import {ERC1967Proxy} from '@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol'; import {MessageHashUtils} from '@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol'; import {OpUSDCBridgeAdapter} from 'contracts/universal/OpUSDCBridgeAdapter.sol'; @@ -33,6 +34,10 @@ contract ForTestOpUSDCBridgeAdapter is OpUSDCBridgeAdapter { function forTest_checkSignature(address _signer, bytes32 _messageHash, bytes memory _signature) public view { _checkSignature(_signer, _messageHash, _signature); } + + function forTest_authorizeUpgrade(address _newAdapter) public { + _authorizeUpgrade(_newAdapter); + } } abstract contract Base is Test { @@ -68,36 +73,6 @@ contract OpUSDCBridgeAdapter_Unit_Constructor is Base { } } -contract OpUSDCBridgeAdapter_Unit_Initialize is Base { - error InvalidInitialization(); - - /** - * @notice Check that the initialize function works as expected - * @dev Needs to be checked on the proxy since the initialize function is disabled on the implementation - */ - function test_initialize(address _owner) public { - // Deploy a proxy contract setting the , and call the initialize function on it to set the owner - ForTestOpUSDCBridgeAdapter _newAdapter = ForTestOpUSDCBridgeAdapter( - address(new ERC1967Proxy(address(_adapterImpl), abi.encodeCall(OpUSDCBridgeAdapter.initialize, _owner))) - ); - - // Assert - assertEq(_newAdapter.owner(), _owner, 'Owner should be set to the provided address'); - } - /** - * @notice Check that the initialize function reverts if it was already called - */ - - function test_revertIfAlreadyInitialize(address _sender, address _owner) public { - // Expect revert with `InvalidInitialization` error - vm.expectRevert(InvalidInitialization.selector); - - // Execute - vm.prank(_sender); - adapter.initialize(_owner); - } -} - contract OpUSDCBridgeAdapter_Unit_SendMessage is Base { /** * @notice Execute vitual function to get 100% coverage @@ -139,6 +114,36 @@ contract OpUSDCBridgeAdapter_Unit_CancelSignature is Base { } } +contract OpUSDCBridgeAdapter_Unit_Initialize is Base { + error InvalidInitialization(); + + /** + * @notice Check that the initialize function works as expected + * @dev Needs to be checked on the proxy since the initialize function is disabled on the implementation + */ + function test_initialize(address _owner) public { + // Deploy a proxy contract setting the , and call the initialize function on it to set the owner + ForTestOpUSDCBridgeAdapter _newAdapter = ForTestOpUSDCBridgeAdapter( + address(new ERC1967Proxy(address(_adapterImpl), abi.encodeCall(OpUSDCBridgeAdapter.initialize, _owner))) + ); + + // Assert + assertEq(_newAdapter.owner(), _owner, 'Owner should be set to the provided address'); + } + + /** + * @notice Check that the initialize function reverts if it was already called + */ + function test_revertIfAlreadyInitialize(address _sender, address _owner) public { + // Expect revert with `InvalidInitialization` error + vm.expectRevert(InvalidInitialization.selector); + + // Execute + vm.prank(_sender); + adapter.initialize(_owner); + } +} + contract OpUSDCBridgeAdapter_Unit_CheckSignature is Base { /** * @notice Check that the signature is valid @@ -170,3 +175,28 @@ contract OpUSDCBridgeAdapter_Unit_CheckSignature is Base { adapter.forTest_checkSignature(_signerAd, _hashedMessage, _signature); } } + +contract OpUSDCBridgeAdapter_Unit_AuthorizeUpgrade is Base { + /** + * @notice Check that the function reverts if the caller is not the owner + */ + function test_revertIfCallerNotOwner(address _caller, address _newAdapter) public { + vm.assume(_caller != adapter.owner()); + + // Expect revert with `OwnableUnauthorizedAccount` error + vm.expectRevert(abi.encodeWithSelector(OwnableUpgradeable.OwnableUnauthorizedAccount.selector, _caller)); + + // Execute + vm.prank(_caller); + adapter.forTest_authorizeUpgrade(_newAdapter); + } + + /** + * @notice Check that the doesn't revert if the caller is the owner + */ + function test_doNothing(address _newAdapter) public { + // Execute + vm.prank(adapter.owner()); + adapter.forTest_authorizeUpgrade(_newAdapter); + } +} From 954912cb93610b81b98c5169de850476da49e2df Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Thu, 25 Jul 2024 18:20:11 -0300 Subject: [PATCH 05/11] fix: l1 factory unit tests --- test/unit/L1OpUSDCFactory.t.sol | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/unit/L1OpUSDCFactory.t.sol b/test/unit/L1OpUSDCFactory.t.sol index 3cb4f7da..871e5e49 100644 --- a/test/unit/L1OpUSDCFactory.t.sol +++ b/test/unit/L1OpUSDCFactory.t.sol @@ -157,7 +157,7 @@ contract L1OpUSDCFactory_Unit_Deploy is Base { // Calculate the L1 Adapter address uint256 _factoryNonce = vm.getNonce(address(factory)); - address _l1Adapter = factory.forTest_precalculateCreateAddress(address(factory), _factoryNonce); + address _l1Adapter = factory.forTest_precalculateCreateAddress(address(factory), _factoryNonce + 1); // Calculate the l2 factory address bytes memory _l2FactoryCArgs = abi.encode( @@ -172,7 +172,7 @@ contract L1OpUSDCFactory_Unit_Deploy is Base { factory.forTest_precalculateCreate2Address(_salt, keccak256(_l2FactoryInitCode), factory.L2_CREATE2_DEPLOYER()); // Calculate the L2 adapter address - address _l2Adapter = factory.forTest_precalculateCreateAddress(_l2Factory, 2); + address _l2Adapter = factory.forTest_precalculateCreateAddress(_l2Factory, 3); // Mock all the `deploy` function calls _mockDeployCalls(); @@ -264,7 +264,7 @@ contract L1OpUSDCFactory_Unit_Deploy is Base { // Precalculate the L1 adapter address uint256 _factoryNonce = vm.getNonce(address(factory)); - address _l1Adapter = factory.forTest_precalculateCreateAddress(address(factory), _factoryNonce); + address _l1Adapter = factory.forTest_precalculateCreateAddress(address(factory), _factoryNonce + 1); // Get the L2 factory deployment tx bytes memory _l2FactoryCArgs = abi.encode( @@ -304,7 +304,7 @@ contract L1OpUSDCFactory_Unit_Deploy is Base { // Calculate the L1 Adapter address uint256 _factoryNonce = vm.getNonce(address(factory)); - address _l1Adapter = factory.forTest_precalculateCreateAddress(address(factory), _factoryNonce); + address _l1Adapter = factory.forTest_precalculateCreateAddress(address(factory), _factoryNonce + 1); // Calculate the l2 factory address bytes memory _l2FactoryCArgs = abi.encode( @@ -319,7 +319,7 @@ contract L1OpUSDCFactory_Unit_Deploy is Base { factory.forTest_precalculateCreate2Address(_salt, keccak256(_l2FactoryInitCode), factory.L2_CREATE2_DEPLOYER()); // Calculate the L2 adapter address - address _l2Adapter = factory.forTest_precalculateCreateAddress(_l2Factory, 2); + address _l2Adapter = factory.forTest_precalculateCreateAddress(_l2Factory, 3); // Mock all the `deploy` function calls _mockDeployCalls(); @@ -341,7 +341,7 @@ contract L1OpUSDCFactory_Unit_Deploy is Base { // Calculate the L1 Adapter address uint256 _factoryNonce = vm.getNonce(address(factory)); - address _expectedL1Adapter = factory.forTest_precalculateCreateAddress(address(factory), _factoryNonce); + address _expectedL1Adapter = factory.forTest_precalculateCreateAddress(address(factory), _factoryNonce + 1); // Calculate the l2 factory address bytes memory _l2FactoryCArgs = abi.encode( @@ -356,7 +356,7 @@ contract L1OpUSDCFactory_Unit_Deploy is Base { factory.forTest_precalculateCreate2Address(_salt, keccak256(_l2FactoryInitCode), factory.L2_CREATE2_DEPLOYER()); // Calculate the L2 adapter address - address _expectedL2Adapter = factory.forTest_precalculateCreateAddress(_expectedL2Factory, 2); + address _expectedL2Adapter = factory.forTest_precalculateCreateAddress(_expectedL2Factory, 3); // Mock all the `deploy` function calls _mockDeployCalls(); From 4001a6e08d4b150d3f8211450bc12f4cc499e141 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Thu, 25 Jul 2024 18:41:05 -0300 Subject: [PATCH 06/11] fix: increment deployments salt counter by 2 on l1 factory * fix: deploy all contracts integration test --- src/contracts/L1OpUSDCFactory.sol | 4 ++-- test/integration/Factories.t.sol | 24 ++++++++++++------------ test/unit/L1OpUSDCFactory.t.sol | 10 +++++----- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/contracts/L1OpUSDCFactory.sol b/src/contracts/L1OpUSDCFactory.sol index 5a9ecb1b..1ec1efe4 100644 --- a/src/contracts/L1OpUSDCFactory.sol +++ b/src/contracts/L1OpUSDCFactory.sol @@ -86,10 +86,10 @@ contract L1OpUSDCFactory is IL1OpUSDCFactory { if (bytes4(_l2Deployments.usdcInitTxs[0]) == _INITIALIZE_SELECTOR) revert IL1OpUSDCFactory_NoInitializeTx(); // Update the salt counter so the L2 factory is deployed with a different salt to a different address and get it - uint256 _currentNonce = ++deploymentsSaltCounter; + uint256 _currentNonce = deploymentsSaltCounter += 2; // Precalculate the l1 adapter proxy address - _l1Adapter = CrossChainDeployments.precalculateCreateAddress(address(this), _currentNonce + 1); + _l1Adapter = CrossChainDeployments.precalculateCreateAddress(address(this), _currentNonce); // Get the L1 USDC naming and decimals to ensure they are the same on the L2, guaranteeing the same standard IL2OpUSDCDeploy.USDCInitializeData memory _usdcInitializeData = IL2OpUSDCDeploy.USDCInitializeData( diff --git a/test/integration/Factories.t.sol b/test/integration/Factories.t.sol index a0cf46ca..44a90992 100644 --- a/test/integration/Factories.t.sol +++ b/test/integration/Factories.t.sol @@ -29,9 +29,9 @@ contract Integration_Factories is IntegrationBase { l1Factory.deploy(address(OPTIMISM_L1_MESSENGER), _owner, 'Optimism', _l2Deployments); // Check the adapter was properly deployed on L1 - assertEq(IOpUSDCBridgeAdapter(_l1Adapter).USDC(), address(MAINNET_USDC)); - assertEq(IOpUSDCBridgeAdapter(_l1Adapter).MESSENGER(), address(OPTIMISM_L1_MESSENGER)); - assertEq(IOpUSDCBridgeAdapter(_l1Adapter).LINKED_ADAPTER(), _l2Adapter); + assertEq(IOpUSDCBridgeAdapter(_l1Adapter).USDC(), address(MAINNET_USDC), '1'); + assertEq(IOpUSDCBridgeAdapter(_l1Adapter).MESSENGER(), address(OPTIMISM_L1_MESSENGER), '2'); + assertEq(IOpUSDCBridgeAdapter(_l1Adapter).LINKED_ADAPTER(), _l2Adapter, '3'); assertEq(Ownable(_l1Adapter).owner(), _owner); bytes32 _salt = bytes32(l1Factory.deploymentsSaltCounter()); @@ -49,19 +49,19 @@ contract Integration_Factories is IntegrationBase { // Check the adapter was properly deployed on L2 IUSDC _l2Usdc = IUSDC(IOpUSDCBridgeAdapter(_l2Adapter).USDC()); - assertEq(IOpUSDCBridgeAdapter(_l2Adapter).MESSENGER(), address(L2_MESSENGER)); - assertEq(IOpUSDCBridgeAdapter(_l2Adapter).LINKED_ADAPTER(), _l1Adapter); - assertEq(Ownable(_l2Adapter).owner(), _owner); + assertEq(IOpUSDCBridgeAdapter(_l2Adapter).MESSENGER(), address(L2_MESSENGER), '4'); + assertEq(IOpUSDCBridgeAdapter(_l2Adapter).LINKED_ADAPTER(), _l1Adapter, '5'); + assertEq(Ownable(_l2Adapter).owner(), _owner, '6'); // Check the L2 factory was deployed - assertGt(_l2Factory.code.length, 0); + assertGt(_l2Factory.code.length, 0, '7'); // Check the USDC was properly deployed on L2 - assertEq(_l2Usdc.name(), 'Bridged USDC (Optimism)'); - assertEq(_l2Usdc.symbol(), _usdcSymbol); - assertEq(_l2Usdc.decimals(), _usdcDecimals); - assertEq(_l2Usdc.currency(), _usdcCurrency); - assertGt(_l2Usdc.implementation().code.length, 0); + assertEq(_l2Usdc.name(), 'Bridged USDC (Optimism)', '8'); + assertEq(_l2Usdc.symbol(), _usdcSymbol, '9'); + assertEq(_l2Usdc.decimals(), _usdcDecimals, '10'); + assertEq(_l2Usdc.currency(), _usdcCurrency, '11'); + assertGt(_l2Usdc.implementation().code.length, 0, '12'); // Check the USDC permissions and allowances were properly set assertEq(_l2Usdc.admin(), address(IL2OpUSDCBridgeAdapter(_l2Adapter).FALLBACK_PROXY_ADMIN())); diff --git a/test/unit/L1OpUSDCFactory.t.sol b/test/unit/L1OpUSDCFactory.t.sol index 871e5e49..dd987766 100644 --- a/test/unit/L1OpUSDCFactory.t.sol +++ b/test/unit/L1OpUSDCFactory.t.sol @@ -145,7 +145,7 @@ contract L1OpUSDCFactory_Unit_Deploy is Base { factory.deploy(_l1Messenger, _l1AdapterOwner, _chainName, _l2Deployments); // Assert - assertEq(factory.deploymentsSaltCounter(), _saltBefore + 1, 'Invalid salt counter'); + assertEq(factory.deploymentsSaltCounter(), _saltBefore + 2, 'Invalid salt counter'); } /** @@ -153,7 +153,7 @@ contract L1OpUSDCFactory_Unit_Deploy is Base { * @dev Assuming the `L1OpUSDCBridgeAdapter` sets the immutables correctly to check we are passing the right values */ function test_deployL1Adapter() public { - bytes32 _salt = bytes32(factory.deploymentsSaltCounter() + 1); + bytes32 _salt = bytes32(factory.deploymentsSaltCounter() + 2); // Calculate the L1 Adapter address uint256 _factoryNonce = vm.getNonce(address(factory)); @@ -257,7 +257,7 @@ contract L1OpUSDCFactory_Unit_Deploy is Base { */ function test_sendFactoryDeploymentMessage() public { uint256 _zeroValue = 0; - bytes32 _salt = bytes32(factory.deploymentsSaltCounter() + 1); + bytes32 _salt = bytes32(factory.deploymentsSaltCounter() + 2); // Mock all the `deploy` function calls _mockDeployCalls(); @@ -300,7 +300,7 @@ contract L1OpUSDCFactory_Unit_Deploy is Base { * @notice Check the `L1AdapterDeployed` event is properly emitted */ function test_emitEvent() public { - bytes32 _salt = bytes32(factory.deploymentsSaltCounter() + 1); + bytes32 _salt = bytes32(factory.deploymentsSaltCounter() + 2); // Calculate the L1 Adapter address uint256 _factoryNonce = vm.getNonce(address(factory)); @@ -337,7 +337,7 @@ contract L1OpUSDCFactory_Unit_Deploy is Base { * @notice Check the returned addresses are the expected ones */ function test_returnAdapters() public { - bytes32 _salt = bytes32(factory.deploymentsSaltCounter() + 1); + bytes32 _salt = bytes32(factory.deploymentsSaltCounter() + 2); // Calculate the L1 Adapter address uint256 _factoryNonce = vm.getNonce(address(factory)); From 523897db8263407db3ca9fce13a1caa25f602a5f Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Thu, 25 Jul 2024 23:51:24 -0300 Subject: [PATCH 07/11] fix: empty expect revert due to possible foundry bug --- test/integration/L1OpUSDCBridgeAdapter.t.sol | 8 ++++++-- test/integration/L2OpUSDCBridgeAdapter.t.sol | 5 ++++- test/unit/L2OpUSDCBridgeAdapter.t.sol | 2 ++ 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/test/integration/L1OpUSDCBridgeAdapter.t.sol b/test/integration/L1OpUSDCBridgeAdapter.t.sol index 6be89235..152052b6 100644 --- a/test/integration/L1OpUSDCBridgeAdapter.t.sol +++ b/test/integration/L1OpUSDCBridgeAdapter.t.sol @@ -153,8 +153,8 @@ contract Integration_Bridging is IntegrationBase { * @notice Test signature message reverts with incorrect signature */ function test_bridgeFromL1WithIncorrectSignature() public { - (address _signerAd, uint256 _signerPk) = makeAddrAndKey('signer'); vm.selectFork(mainnet); + (address _signerAd, uint256 _signerPk) = makeAddrAndKey('signer'); // We need to do this instead of `deal` because deal doesnt change `totalSupply` state vm.prank(MAINNET_USDC.masterMinter()); @@ -172,8 +172,12 @@ contract Integration_Bridging is IntegrationBase { // Different address can execute the message vm.startPrank(_user); - vm.expectRevert(IOpUSDCBridgeAdapter.IOpUSDCBridgeAdapter_InvalidSignature.selector); + /// NOTE: Didn't us `vm.expectRevert(IOpUSDCBridgeAdapter.IOpUSDCBridgeAdapter_InvalidSignature.selector)` because + /// it reverts with that error, but then the test fails because of a foundry issue with the error message + /// `contract signer does not exist`, which is not true. + vm.expectRevert(); l1Adapter.sendMessage(_signerAd, _signerAd, _amount, _signature, _USER_NONCE, _deadline, _MIN_GAS_LIMIT); + vm.stopPrank(); } } diff --git a/test/integration/L2OpUSDCBridgeAdapter.t.sol b/test/integration/L2OpUSDCBridgeAdapter.t.sol index ba7a64bb..5851918b 100644 --- a/test/integration/L2OpUSDCBridgeAdapter.t.sol +++ b/test/integration/L2OpUSDCBridgeAdapter.t.sol @@ -222,7 +222,10 @@ contract Integration_Bridging is IntegrationBase { // Different address can execute the message vm.startPrank(_user); - vm.expectRevert(IOpUSDCBridgeAdapter.IOpUSDCBridgeAdapter_InvalidSignature.selector); + /// NOTE: Didn't us `vm.expectRevert(IOpUSDCBridgeAdapter.IOpUSDCBridgeAdapter_InvalidSignature.selector)` because + /// it reverts with that error, but then the test fails because of a foundry issue with the error message + /// `contract signer does not exist`, which is not true. + vm.expectRevert(); l2Adapter.sendMessage(_signerAd, _signerAd, _amount, _signature, _USER_NONCE, _deadline, _MIN_GAS_LIMIT); vm.stopPrank(); } diff --git a/test/unit/L2OpUSDCBridgeAdapter.t.sol b/test/unit/L2OpUSDCBridgeAdapter.t.sol index 80c06080..665b852b 100644 --- a/test/unit/L2OpUSDCBridgeAdapter.t.sol +++ b/test/unit/L2OpUSDCBridgeAdapter.t.sol @@ -1015,6 +1015,8 @@ contract L2OpUSDCBridgeAdapter_Unit_Initialize is Base { * @dev Needs to be checked on the proxy since the initialize function is disabled on the implementation */ function test_initialize(address _owner) public { + vm.assume(_owner != address(0)); + // Deploy a proxy contract setting the , and call the initialize function on it to set the owner ForTestL2OpUSDCBridgeAdapter _newAdapter = ForTestL2OpUSDCBridgeAdapter( address(new ERC1967Proxy(address(_adapterImpl), abi.encodeCall(OpUSDCBridgeAdapter.initialize, _owner))) From d4d0ba0b082c0b4284bff96cd4253d9ccabe6444 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Fri, 26 Jul 2024 00:44:13 -0300 Subject: [PATCH 08/11] fix: lint --- src/contracts/L2OpUSDCBridgeAdapter.sol | 1 + .../universal/OpUSDCBridgeAdapter.sol | 10 +++--- src/interfaces/IOpUSDCBridgeAdapter.sol | 32 ++++++++++--------- test/unit/L1OpUSDCBridgeAdapter.t.sol | 2 +- test/unit/OpUSDCBridgeAdapter.t.sol | 8 ++--- 5 files changed, 28 insertions(+), 25 deletions(-) diff --git a/src/contracts/L2OpUSDCBridgeAdapter.sol b/src/contracts/L2OpUSDCBridgeAdapter.sol index 3246e381..185558a1 100644 --- a/src/contracts/L2OpUSDCBridgeAdapter.sol +++ b/src/contracts/L2OpUSDCBridgeAdapter.sol @@ -32,6 +32,7 @@ contract L2OpUSDCBridgeAdapter is IL2OpUSDCBridgeAdapter, OpUSDCBridgeAdapter { bytes4 internal constant _UPDATE_MASTER_MINTER_SELECTOR = 0xaa20e1e4; /// @inheritdoc IL2OpUSDCBridgeAdapter + // solhint-disable-next-line var-name-mixedcase FallbackProxyAdmin public FALLBACK_PROXY_ADMIN; /// @inheritdoc IL2OpUSDCBridgeAdapter diff --git a/src/contracts/universal/OpUSDCBridgeAdapter.sol b/src/contracts/universal/OpUSDCBridgeAdapter.sol index 0185ed43..e7367da7 100644 --- a/src/contracts/universal/OpUSDCBridgeAdapter.sol +++ b/src/contracts/universal/OpUSDCBridgeAdapter.sol @@ -116,6 +116,11 @@ abstract contract OpUSDCBridgeAdapter is UUPSUpgradeable, OwnableUpgradeable, EI __Ownable_init(_owner); } + /** + * @notice Checks the caller is the owner to authorize the upgrade + */ + function _authorizeUpgrade(address) internal virtual override onlyOwner {} + /** * @notice Check the signature of a message * @param _signer the address that signed the message @@ -129,11 +134,6 @@ abstract contract OpUSDCBridgeAdapter is UUPSUpgradeable, OwnableUpgradeable, EI if (!_signer.isValidSignatureNow(_messageHash, _signature)) revert IOpUSDCBridgeAdapter_InvalidSignature(); } - /** - * @notice Checks the caller is the owner to authorize the upgrade - */ - function _authorizeUpgrade(address) internal virtual override onlyOwner {} - /** * @notice Hashes the bridge message struct * @param _message The bridge message struct to hash diff --git a/src/interfaces/IOpUSDCBridgeAdapter.sol b/src/interfaces/IOpUSDCBridgeAdapter.sol index e7b92b9d..c2cf30b6 100644 --- a/src/interfaces/IOpUSDCBridgeAdapter.sol +++ b/src/interfaces/IOpUSDCBridgeAdapter.sol @@ -2,21 +2,6 @@ pragma solidity 0.8.25; interface IOpUSDCBridgeAdapter { - /** - * @notice The struct to hold the data for a bridge message with signature - * @param to The target address on the destination chain - * @param amount The amount of tokens to send - * @param deadline The deadline for the message to be executed - * @param nonce The nonce of the user - * @param minGasLimit The minimum gas limit for the message to be executed - */ - struct BridgeMessage { - address to; - uint256 amount; - uint256 deadline; - uint256 nonce; - uint32 minGasLimit; - } /*/////////////////////////////////////////////////////////////// ENUMS ///////////////////////////////////////////////////////////////*/ @@ -34,6 +19,23 @@ interface IOpUSDCBridgeAdapter { Upgrading, Deprecated } + + /** + * @notice The struct to hold the data for a bridge message with signature + * @param to The target address on the destination chain + * @param amount The amount of tokens to send + * @param deadline The deadline for the message to be executed + * @param nonce The nonce of the user + * @param minGasLimit The minimum gas limit for the message to be executed + */ + struct BridgeMessage { + address to; + uint256 amount; + uint256 deadline; + uint256 nonce; + uint32 minGasLimit; + } + /*/////////////////////////////////////////////////////////////// EVENTS ///////////////////////////////////////////////////////////////*/ diff --git a/test/unit/L1OpUSDCBridgeAdapter.t.sol b/test/unit/L1OpUSDCBridgeAdapter.t.sol index 3382f021..3c10408e 100644 --- a/test/unit/L1OpUSDCBridgeAdapter.t.sol +++ b/test/unit/L1OpUSDCBridgeAdapter.t.sol @@ -2,7 +2,7 @@ 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'; +import {L1OpUSDCBridgeAdapter} from 'contracts/L1OpUSDCBridgeAdapter.sol'; import {L1OpUSDCBridgeAdapter} from 'contracts/L1OpUSDCBridgeAdapter.sol'; import {IOpUSDCBridgeAdapter} from 'interfaces/IOpUSDCBridgeAdapter.sol'; import {OpUSDCBridgeAdapter} from 'src/contracts/universal/OpUSDCBridgeAdapter.sol'; diff --git a/test/unit/OpUSDCBridgeAdapter.t.sol b/test/unit/OpUSDCBridgeAdapter.t.sol index bebdf6e2..62187914 100644 --- a/test/unit/OpUSDCBridgeAdapter.t.sol +++ b/test/unit/OpUSDCBridgeAdapter.t.sol @@ -32,13 +32,13 @@ contract ForTestOpUSDCBridgeAdapter is OpUSDCBridgeAdapter { uint32 _minGasLimit ) external override {} - function forTest_checkSignature(address _signer, bytes32 _messageHash, bytes memory _signature) public view { - _checkSignature(_signer, _messageHash, _signature); - } - function forTest_authorizeUpgrade(address _newAdapter) public { _authorizeUpgrade(_newAdapter); } + + function forTest_checkSignature(address _signer, bytes32 _messageHash, bytes memory _signature) public view { + _checkSignature(_signer, _messageHash, _signature); + } } abstract contract Base is Test { From 5ae100269360bffd494afb4ce1ea421529197f74 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Fri, 26 Jul 2024 12:16:07 -0300 Subject: [PATCH 09/11] feat: add eip712 upgradeable --- src/contracts/L1OpUSDCBridgeAdapter.sol | 13 +++++ src/contracts/L2OpUSDCBridgeAdapter.sol | 27 ++++----- .../universal/OpUSDCBridgeAdapter.sol | 24 ++++---- test/integration/L1OpUSDCBridgeAdapter.t.sol | 18 +++++- test/integration/L2OpUSDCBridgeAdapter.t.sol | 18 +++++- test/unit/L1OpUSDCBridgeAdapter.t.sol | 57 ++++++++++++++++--- test/unit/L2OpUSDCBridgeAdapter.t.sol | 17 ++++-- test/unit/OpUSDCBridgeAdapter.t.sol | 32 +---------- test/utils/Helpers.sol | 4 +- test/utils/SigUtils.sol | 4 +- 10 files changed, 131 insertions(+), 83 deletions(-) diff --git a/src/contracts/L1OpUSDCBridgeAdapter.sol b/src/contracts/L1OpUSDCBridgeAdapter.sol index 53347973..642606c7 100644 --- a/src/contracts/L1OpUSDCBridgeAdapter.sol +++ b/src/contracts/L1OpUSDCBridgeAdapter.sol @@ -47,6 +47,19 @@ contract L1OpUSDCBridgeAdapter is IL1OpUSDCBridgeAdapter, OpUSDCBridgeAdapter { address _linkedAdapter ) OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter) {} + /** + * @notice Sets the owner of the contract + * @param _owner The address of the owner + * @dev This function needs only used during the deployment of the proxy contract, and it is disabled for the + * implementation contract + */ + function initialize(address _owner) external virtual override initializer { + __Ownable_init(_owner); + string memory _name = 'L1OpUSDCBridgeAdapter'; + string memory _version = '1.0.0'; + __EIP712_init(_name, _version); + } + /*/////////////////////////////////////////////////////////////// MIGRATION ///////////////////////////////////////////////////////////////*/ diff --git a/src/contracts/L2OpUSDCBridgeAdapter.sol b/src/contracts/L2OpUSDCBridgeAdapter.sol index 185558a1..1a75af62 100644 --- a/src/contracts/L2OpUSDCBridgeAdapter.sol +++ b/src/contracts/L2OpUSDCBridgeAdapter.sol @@ -63,6 +63,20 @@ contract L2OpUSDCBridgeAdapter is IL2OpUSDCBridgeAdapter, OpUSDCBridgeAdapter { ) OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter) {} /* solhint-enable no-unused-vars */ + /** + * @notice Sets the owner of the contract + * @param _owner The address of the owner + * @dev This function needs only used during the deployment of the proxy contract, and it is disabled for the + * implementation contract + */ + function initialize(address _owner) external virtual override initializer { + __Ownable_init(_owner); + string memory _name = 'L1OpUSDCBridgeAdapter'; + string memory _version = '1.0.0'; + __EIP712_init(_name, _version); + FALLBACK_PROXY_ADMIN = new FallbackProxyAdmin(USDC); + } + /*/////////////////////////////////////////////////////////////// MIGRATION ///////////////////////////////////////////////////////////////*/ @@ -274,19 +288,6 @@ contract L2OpUSDCBridgeAdapter is IL2OpUSDCBridgeAdapter, OpUSDCBridgeAdapter { emit USDCFunctionSent(_selector); } - /** - * @notice Sets the owner of the contract and deploys the `FallbackProxyAdmin` contract - * @param _owner The address of the owner - * @dev This function needs only used during the deployment of the proxy contract, and it is disabled for the - * implementation contract - * @dev The `FallbackProxyAdmin` contract is deployed since it can't be deployed on the constructor, otherwise, the - * owner would be set to the implementation contract address instead of the proxy one - */ - function initialize(address _owner) public virtual override initializer { - super.initialize(_owner); - FALLBACK_PROXY_ADMIN = new FallbackProxyAdmin(USDC); - } - /*/////////////////////////////////////////////////////////////// INTERNAL FUNCTIONS ///////////////////////////////////////////////////////////////*/ diff --git a/src/contracts/universal/OpUSDCBridgeAdapter.sol b/src/contracts/universal/OpUSDCBridgeAdapter.sol index e7367da7..c0052bb0 100644 --- a/src/contracts/universal/OpUSDCBridgeAdapter.sol +++ b/src/contracts/universal/OpUSDCBridgeAdapter.sol @@ -3,12 +3,12 @@ pragma solidity 0.8.25; import {OwnableUpgradeable} from '@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol'; import {UUPSUpgradeable} from '@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol'; -import {EIP712} from '@openzeppelin/contracts/utils/cryptography/EIP712.sol'; +import {EIP712Upgradeable} from '@openzeppelin/contracts-upgradeable/utils/cryptography/EIP712Upgradeable.sol'; import {MessageHashUtils} from '@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol'; import {SignatureChecker} from '@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol'; import {IOpUSDCBridgeAdapter} from 'interfaces/IOpUSDCBridgeAdapter.sol'; -abstract contract OpUSDCBridgeAdapter is UUPSUpgradeable, OwnableUpgradeable, EIP712, IOpUSDCBridgeAdapter { +abstract contract OpUSDCBridgeAdapter is UUPSUpgradeable, OwnableUpgradeable, EIP712Upgradeable, IOpUSDCBridgeAdapter { using MessageHashUtils for bytes32; using SignatureChecker for address; @@ -44,13 +44,19 @@ abstract contract OpUSDCBridgeAdapter is UUPSUpgradeable, OwnableUpgradeable, EI * @param _linkedAdapter The address of the linked adapter */ // solhint-disable-next-line no-unused-vars - constructor(address _usdc, address _messenger, address _linkedAdapter) EIP712('OpUSDCBridgeAdapter', '1.0.0') { + constructor(address _usdc, address _messenger, address _linkedAdapter) { USDC = _usdc; MESSENGER = _messenger; LINKED_ADAPTER = _linkedAdapter; _disableInitializers(); } + /** + * @notice Initialize the contract + * @param _owner The owner of the contract + */ + function initialize(address _owner) external virtual initializer {} + /*/////////////////////////////////////////////////////////////// MESSAGING ///////////////////////////////////////////////////////////////*/ @@ -106,16 +112,6 @@ abstract contract OpUSDCBridgeAdapter is UUPSUpgradeable, OwnableUpgradeable, EI userNonces[msg.sender][_nonce] = true; } - /** - * @notice Sets the owner of the contract - * @param _owner The address of the owner - * @dev This function needs only used during the deployment of the proxy contract, and it is disabled for the - * implementation contract - */ - function initialize(address _owner) public virtual initializer { - __Ownable_init(_owner); - } - /** * @notice Checks the caller is the owner to authorize the upgrade */ @@ -128,7 +124,7 @@ abstract contract OpUSDCBridgeAdapter is UUPSUpgradeable, OwnableUpgradeable, EI * @param _signature the signature of the message */ function _checkSignature(address _signer, bytes32 _messageHash, bytes memory _signature) internal view { - // Uses the EIP712 typed data hash + // Uses the EIP712Upgradeable typed data hash _messageHash = _hashTypedDataV4(_messageHash); if (!_signer.isValidSignatureNow(_messageHash, _signature)) revert IOpUSDCBridgeAdapter_InvalidSignature(); diff --git a/test/integration/L1OpUSDCBridgeAdapter.t.sol b/test/integration/L1OpUSDCBridgeAdapter.t.sol index 152052b6..545e0718 100644 --- a/test/integration/L1OpUSDCBridgeAdapter.t.sol +++ b/test/integration/L1OpUSDCBridgeAdapter.t.sol @@ -5,6 +5,9 @@ import {IntegrationBase} from './IntegrationBase.sol'; import {IOpUSDCBridgeAdapter} from 'interfaces/IOpUSDCBridgeAdapter.sol'; contract Integration_Bridging is IntegrationBase { + string internal constant _NAME = 'L1OpUSDCBridgeAdapter'; + string internal constant _VERSION = '1.0.0'; + /** * @notice Test the bridging process from L1 -> L2 */ @@ -92,7 +95,16 @@ contract Integration_Bridging is IntegrationBase { MAINNET_USDC.approve(address(l1Adapter), _amount); uint256 _deadline = block.timestamp + 1 days; bytes memory _signature = _generateSignature( - _signerAd, _amount, _deadline, _MIN_GAS_LIMIT, _USER_NONCE, _signerAd, _signerPk, address(l1Adapter) + _NAME, + _VERSION, + _signerAd, + _amount, + _deadline, + _MIN_GAS_LIMIT, + _USER_NONCE, + _signerAd, + _signerPk, + address(l1Adapter) ); // Different address can execute the message @@ -136,7 +148,7 @@ contract Integration_Bridging is IntegrationBase { // Changing to `to` param to _user but we call it with _signerAd uint256 _deadline = block.timestamp + 1 days; bytes memory _signature = _generateSignature( - _user, _amount, _deadline, _MIN_GAS_LIMIT, _USER_NONCE, _signerAd, _signerPk, address(l1Adapter) + _NAME, _VERSION, _user, _amount, _deadline, _MIN_GAS_LIMIT, _USER_NONCE, _signerAd, _signerPk, address(l1Adapter) ); // Cancel the signature @@ -167,7 +179,7 @@ contract Integration_Bridging is IntegrationBase { // Changing to `to` param to _user but we call it with _signerAd uint256 _deadline = block.timestamp + 1 days; bytes memory _signature = _generateSignature( - _user, _amount, _deadline, _MIN_GAS_LIMIT, _USER_NONCE, _signerAd, _signerPk, address(l1Adapter) + _NAME, _VERSION, _user, _amount, _deadline, _MIN_GAS_LIMIT, _USER_NONCE, _signerAd, _signerPk, address(l1Adapter) ); // Different address can execute the message diff --git a/test/integration/L2OpUSDCBridgeAdapter.t.sol b/test/integration/L2OpUSDCBridgeAdapter.t.sol index 5851918b..abe69458 100644 --- a/test/integration/L2OpUSDCBridgeAdapter.t.sol +++ b/test/integration/L2OpUSDCBridgeAdapter.t.sol @@ -14,6 +14,9 @@ contract dummyImplementation { } contract Integration_Bridging is IntegrationBase { + string internal constant _NAME = 'L1OpUSDCBridgeAdapter'; + string internal constant _VERSION = '1.0.0'; + using stdStorage for StdStorage; function setUp() public override { @@ -138,7 +141,16 @@ contract Integration_Bridging is IntegrationBase { bridgedUSDC.approve(address(l2Adapter), _amount); uint256 _deadline = block.timestamp + 1 days; bytes memory _signature = _generateSignature( - _signerAd, _amount, _deadline, _MIN_GAS_LIMIT, _USER_NONCE, _signerAd, _signerPk, address(l2Adapter) + _NAME, + _VERSION, + _signerAd, + _amount, + _deadline, + _MIN_GAS_LIMIT, + _USER_NONCE, + _signerAd, + _signerPk, + address(l2Adapter) ); // Different address can execute the message @@ -184,7 +196,7 @@ contract Integration_Bridging is IntegrationBase { // Changing to `to` param to _user but we call it with _signerAd uint256 _deadline = block.timestamp + 1 days; bytes memory _signature = _generateSignature( - _user, _amount, _deadline, _MIN_GAS_LIMIT, _USER_NONCE, _signerAd, _signerPk, address(l2Adapter) + _NAME, _VERSION, _user, _amount, _deadline, _MIN_GAS_LIMIT, _USER_NONCE, _signerAd, _signerPk, address(l2Adapter) ); // Cancel the signature @@ -217,7 +229,7 @@ contract Integration_Bridging is IntegrationBase { // Changing to `to` param to _user but we call it with _signerAd uint256 _deadline = block.timestamp + 1 days; bytes memory _signature = _generateSignature( - _user, _amount, _deadline, _MIN_GAS_LIMIT, _USER_NONCE, _signerAd, _signerPk, address(l2Adapter) + _NAME, _VERSION, _user, _amount, _deadline, _MIN_GAS_LIMIT, _USER_NONCE, _signerAd, _signerPk, address(l2Adapter) ); // Different address can execute the message diff --git a/test/unit/L1OpUSDCBridgeAdapter.t.sol b/test/unit/L1OpUSDCBridgeAdapter.t.sol index 3c10408e..3d3f9644 100644 --- a/test/unit/L1OpUSDCBridgeAdapter.t.sol +++ b/test/unit/L1OpUSDCBridgeAdapter.t.sol @@ -37,15 +37,19 @@ contract ForTestL1OpUSDCBridgeAdapter is L1OpUSDCBridgeAdapter { } abstract contract Base is Helpers { + string internal constant _NAME = 'L1OpUSDCBridgeAdapter'; + string internal constant _VERSION = '1.0.0'; + ForTestL1OpUSDCBridgeAdapter public adapter; bytes32 internal _salt = bytes32('1'); address internal _user = makeAddr('user'); address internal _owner = makeAddr('owner'); - address internal _signerAd; - uint256 internal _signerPk; address internal _usdc = makeAddr('opUSDC'); address internal _linkedAdapter = makeAddr('linkedAdapter'); + address internal _adapterImpl; + address internal _signerAd; + uint256 internal _signerPk; // cant fuzz this because of foundry's VM address internal _messenger = makeAddr('messenger'); @@ -60,7 +64,7 @@ abstract contract Base is Helpers { (_signerAd, _signerPk) = makeAddrAndKey('signer'); vm.etch(_messenger, 'xDomainMessageSender'); - address _adapterImpl = address(new ForTestL1OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter)); + _adapterImpl = address(new ForTestL1OpUSDCBridgeAdapter(_usdc, _messenger, _linkedAdapter)); adapter = ForTestL1OpUSDCBridgeAdapter( address(new ERC1967Proxy(_adapterImpl, abi.encodeCall(OpUSDCBridgeAdapter.initialize, _owner))) ); @@ -79,6 +83,38 @@ contract L1OpUSDCBridgeAdapter_Unit_Constructor is Base { } } +contract L1OpUSDCBridgeAdapter_Unit_Initialize is Base { + error InvalidInitialization(); + + /** + * @notice Check that the initialize function works as expected + * @dev Needs to be checked on the proxy since the initialize function is disabled on the implementation + */ + function test_initialize(address _owner) public { + vm.assume(_owner != address(0)); + + // Deploy a proxy contract setting the , and call the initialize function on it to set the owner + ForTestL1OpUSDCBridgeAdapter _newAdapter = ForTestL1OpUSDCBridgeAdapter( + address(new ERC1967Proxy(address(_adapterImpl), abi.encodeCall(OpUSDCBridgeAdapter.initialize, _owner))) + ); + + // Assert + assertEq(_newAdapter.owner(), _owner, 'Owner should be set to the provided address'); + } + + /** + * @notice Check that the initialize function reverts if it was already called + */ + function test_revertIfAlreadyInitialize(address _sender, address _owner) public { + // Expect revert with `InvalidInitialization` error + vm.expectRevert(InvalidInitialization.selector); + + // Execute + vm.prank(_sender); + adapter.initialize(_owner); + } +} + /*/////////////////////////////////////////////////////////////// MIGRATION ///////////////////////////////////////////////////////////////*/ @@ -920,8 +956,9 @@ contract L1OpUSDCBridgeAdapter_Unit_SendMessageWithSignature is Base { vm.assume(_deadline > 0); vm.warp(_deadline - 1); (address _notSignerAd, uint256 _notSignerPk) = makeAddrAndKey('notSigner'); - bytes memory _signature = - _generateSignature(_to, _amount, _deadline, _minGasLimit, _nonce, _notSignerAd, _notSignerPk, address(adapter)); + bytes memory _signature = _generateSignature( + _NAME, _VERSION, _to, _amount, _deadline, _minGasLimit, _nonce, _notSignerAd, _notSignerPk, address(adapter) + ); vm.mockCall(_usdc, abi.encodeWithSignature('isBlacklisted(address)', _to), abi.encode(false)); @@ -944,8 +981,9 @@ contract L1OpUSDCBridgeAdapter_Unit_SendMessageWithSignature is Base { vm.assume(_to != address(0)); vm.assume(_deadline > 0); vm.warp(_deadline - 1); - bytes memory _signature = - _generateSignature(_to, _amount, _deadline, _minGasLimit, _nonce, _signerAd, _signerPk, address(adapter)); + bytes memory _signature = _generateSignature( + _NAME, _VERSION, _to, _amount, _deadline, _minGasLimit, _nonce, _signerAd, _signerPk, address(adapter) + ); _mockAndExpect(_usdc, abi.encodeWithSignature('isBlacklisted(address)', _to), abi.encode(false)); _mockAndExpect( @@ -982,8 +1020,9 @@ contract L1OpUSDCBridgeAdapter_Unit_SendMessageWithSignature is Base { vm.assume(_to != address(0)); vm.assume(_deadline > 0); vm.warp(_deadline - 1); - bytes memory _signature = - _generateSignature(_to, _amount, _deadline, _minGasLimit, _nonce, _signerAd, _signerPk, address(adapter)); + bytes memory _signature = _generateSignature( + _NAME, _VERSION, _to, _amount, _deadline, _minGasLimit, _nonce, _signerAd, _signerPk, address(adapter) + ); vm.mockCall(_usdc, abi.encodeWithSignature('isBlacklisted(address)', _to), abi.encode(false)); vm.mockCall( diff --git a/test/unit/L2OpUSDCBridgeAdapter.t.sol b/test/unit/L2OpUSDCBridgeAdapter.t.sol index 665b852b..e32dfde6 100644 --- a/test/unit/L2OpUSDCBridgeAdapter.t.sol +++ b/test/unit/L2OpUSDCBridgeAdapter.t.sol @@ -35,6 +35,8 @@ contract ForTestL2OpUSDCBridgeAdapter is L2OpUSDCBridgeAdapter { } abstract contract Base is Helpers { + string internal constant _NAME = 'L1OpUSDCBridgeAdapter'; + string internal constant _VERSION = '1.0.0'; bytes4 internal constant _UPGRADE_TO_SELECTOR = 0x3659cfe6; bytes4 internal constant _UPGRADE_TO_AND_CALL_SELECTOR = 0x4f1ef286; @@ -583,8 +585,9 @@ contract L2OpUSDCBridgeAdapter_Unit_SendMessageWithSignature is Base { vm.assume(_deadline > 0); vm.warp(_deadline - 1); (address _notSignerAd, uint256 _notSignerPk) = makeAddrAndKey('notSigner'); - bytes memory _signature = - _generateSignature(_to, _amount, _deadline, _minGasLimit, _nonce, _notSignerAd, _notSignerPk, address(adapter)); + bytes memory _signature = _generateSignature( + _NAME, _VERSION, _to, _amount, _deadline, _minGasLimit, _nonce, _notSignerAd, _notSignerPk, address(adapter) + ); vm.mockCall(_usdc, abi.encodeWithSignature('isBlacklisted(address)', _to), abi.encode(false)); // Execute vm.prank(_user); @@ -605,8 +608,9 @@ contract L2OpUSDCBridgeAdapter_Unit_SendMessageWithSignature is Base { vm.assume(_to != address(0)); vm.assume(_deadline > 0); vm.warp(_deadline - 1); - bytes memory _signature = - _generateSignature(_to, _amount, _deadline, _minGasLimit, _nonce, _signerAd, _signerPk, address(adapter)); + bytes memory _signature = _generateSignature( + _NAME, _VERSION, _to, _amount, _deadline, _minGasLimit, _nonce, _signerAd, _signerPk, address(adapter) + ); _mockAndExpect(_usdc, abi.encodeWithSignature('isBlacklisted(address)', _to), abi.encode(false)); _mockAndExpect( @@ -644,8 +648,9 @@ contract L2OpUSDCBridgeAdapter_Unit_SendMessageWithSignature is Base { vm.assume(_to != address(0)); vm.assume(_deadline > 0); vm.warp(_deadline - 1); - bytes memory _signature = - _generateSignature(_to, _amount, _deadline, _minGasLimit, _nonce, _signerAd, _signerPk, address(adapter)); + bytes memory _signature = _generateSignature( + _NAME, _VERSION, _to, _amount, _deadline, _minGasLimit, _nonce, _signerAd, _signerPk, address(adapter) + ); vm.mockCall(_usdc, abi.encodeWithSignature('isBlacklisted(address)', _to), abi.encode(false)); vm.mockCall( diff --git a/test/unit/OpUSDCBridgeAdapter.t.sol b/test/unit/OpUSDCBridgeAdapter.t.sol index 62187914..6b162535 100644 --- a/test/unit/OpUSDCBridgeAdapter.t.sol +++ b/test/unit/OpUSDCBridgeAdapter.t.sol @@ -115,42 +115,12 @@ contract OpUSDCBridgeAdapter_Unit_CancelSignature is Base { } } -contract OpUSDCBridgeAdapter_Unit_Initialize is Base { - error InvalidInitialization(); - - /** - * @notice Check that the initialize function works as expected - * @dev Needs to be checked on the proxy since the initialize function is disabled on the implementation - */ - function test_initialize(address _owner) public { - // Deploy a proxy contract setting the , and call the initialize function on it to set the owner - ForTestOpUSDCBridgeAdapter _newAdapter = ForTestOpUSDCBridgeAdapter( - address(new ERC1967Proxy(address(_adapterImpl), abi.encodeCall(OpUSDCBridgeAdapter.initialize, _owner))) - ); - - // Assert - assertEq(_newAdapter.owner(), _owner, 'Owner should be set to the provided address'); - } - - /** - * @notice Check that the initialize function reverts if it was already called - */ - function test_revertIfAlreadyInitialize(address _sender, address _owner) public { - // Expect revert with `InvalidInitialization` error - vm.expectRevert(InvalidInitialization.selector); - - // Execute - vm.prank(_sender); - adapter.initialize(_owner); - } -} - contract OpUSDCBridgeAdapter_Unit_CheckSignature is Base { /** * @notice Check that the signature is valid */ function test_validSignature(IOpUSDCBridgeAdapter.BridgeMessage memory _message) public { - SigUtils _sigUtils = new SigUtils(address(adapter)); + SigUtils _sigUtils = new SigUtils(address(adapter), '', ''); vm.startPrank(_signerAd); bytes32 _hashedMessage = _sigUtils.getBridgeMessageHash(_message); diff --git a/test/utils/Helpers.sol b/test/utils/Helpers.sol index 35ba0fe9..0b0572dc 100644 --- a/test/utils/Helpers.sol +++ b/test/utils/Helpers.sol @@ -24,6 +24,8 @@ contract Helpers is Test { } function _generateSignature( + string memory _name, + string memory _version, address _to, uint256 _amount, uint256 _deadline, @@ -41,7 +43,7 @@ contract Helpers is Test { minGasLimit: uint32(_minGasLimit) }); - SigUtils _sigUtils = new SigUtils(_adapter); + SigUtils _sigUtils = new SigUtils(_adapter, _name, _version); vm.startPrank(_signerAd); bytes32 _digest = SigUtils(_sigUtils).getTypedBridgeMessageHash(_message); diff --git a/test/utils/SigUtils.sol b/test/utils/SigUtils.sol index 2626e7d1..fb864d5c 100644 --- a/test/utils/SigUtils.sol +++ b/test/utils/SigUtils.sol @@ -21,9 +21,7 @@ contract SigUtils { string private _nameFallback; string private _versionFallback; - constructor(address _adapter) { - string memory _name = 'OpUSDCBridgeAdapter'; - string memory _version = '1.0.0'; + constructor(address _adapter, string memory _name, string memory _version) { _NAME = _name.toShortStringWithFallback(_nameFallback); _VERSION = _version.toShortStringWithFallback(_versionFallback); From d5d49aab7ca24392e46a1195fc4541b5a90540a7 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Fri, 26 Jul 2024 12:18:32 -0300 Subject: [PATCH 10/11] fix: lint --- test/integration/L2OpUSDCBridgeAdapter.t.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/L2OpUSDCBridgeAdapter.t.sol b/test/integration/L2OpUSDCBridgeAdapter.t.sol index abe69458..b1241d98 100644 --- a/test/integration/L2OpUSDCBridgeAdapter.t.sol +++ b/test/integration/L2OpUSDCBridgeAdapter.t.sol @@ -14,11 +14,11 @@ contract dummyImplementation { } contract Integration_Bridging is IntegrationBase { + using stdStorage for StdStorage; + string internal constant _NAME = 'L1OpUSDCBridgeAdapter'; string internal constant _VERSION = '1.0.0'; - using stdStorage for StdStorage; - function setUp() public override { super.setUp(); From 3d5efec24840ae76d41a5136951cd5c8ce237936 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Fri, 26 Jul 2024 13:09:46 -0300 Subject: [PATCH 11/11] feat: add gap variables on l1 and l2 adapters --- src/contracts/L1OpUSDCBridgeAdapter.sol | 3 +++ src/contracts/L2OpUSDCBridgeAdapter.sol | 3 +++ src/contracts/universal/OpUSDCBridgeAdapter.sol | 2 +- 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/contracts/L1OpUSDCBridgeAdapter.sol b/src/contracts/L1OpUSDCBridgeAdapter.sol index 642606c7..05f91707 100644 --- a/src/contracts/L1OpUSDCBridgeAdapter.sol +++ b/src/contracts/L1OpUSDCBridgeAdapter.sol @@ -24,6 +24,9 @@ contract L1OpUSDCBridgeAdapter is IL1OpUSDCBridgeAdapter, OpUSDCBridgeAdapter { /// @inheritdoc IL1OpUSDCBridgeAdapter address public burnCaller; + /// @notice Reserve 50 more storage slots to be safe on future upgrades + uint256[50] private __gap; + /** * @notice Modifier to check if the sender is the linked adapter through the messenger */ diff --git a/src/contracts/L2OpUSDCBridgeAdapter.sol b/src/contracts/L2OpUSDCBridgeAdapter.sol index 1a75af62..66e11034 100644 --- a/src/contracts/L2OpUSDCBridgeAdapter.sol +++ b/src/contracts/L2OpUSDCBridgeAdapter.sol @@ -38,6 +38,9 @@ contract L2OpUSDCBridgeAdapter is IL2OpUSDCBridgeAdapter, OpUSDCBridgeAdapter { /// @inheritdoc IL2OpUSDCBridgeAdapter address public roleCaller; + /// @notice Reserve 50 more storage slots to be safe on future upgrades + uint256[50] private __gap; + /** * @notice Modifier to check if the sender is the linked adapter through the messenger */ diff --git a/src/contracts/universal/OpUSDCBridgeAdapter.sol b/src/contracts/universal/OpUSDCBridgeAdapter.sol index c0052bb0..4c59df61 100644 --- a/src/contracts/universal/OpUSDCBridgeAdapter.sol +++ b/src/contracts/universal/OpUSDCBridgeAdapter.sol @@ -26,7 +26,7 @@ abstract contract OpUSDCBridgeAdapter is UUPSUpgradeable, OwnableUpgradeable, EI address public immutable MESSENGER; /// @notice Reserve 50 storage slots to be safe on future upgrades - uint256[50] internal __gap; + uint256[50] private __gap; /// @inheritdoc IOpUSDCBridgeAdapter Status public messengerStatus;