-
Notifications
You must be signed in to change notification settings - Fork 69
feat: upgrade bridge to allow for transfer of all token assets #103
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
59f88b0
c3cfd54
1ff1b41
f004b73
75e4ab0
11b66b1
97452bd
7980767
d3a9285
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,16 +90,33 @@ contract Portal is Initializable, ResourceMetering, ISemver { | |
| /// @param success Whether the withdrawal transaction was successful. | ||
| event WithdrawalFinalized(bytes32 indexed withdrawalHash, bool success); | ||
|
|
||
| /// @notice Emitted when an emergency withdrawal is executed. | ||
| /// @param recipient The address that received the funds. | ||
| /// @param token The token address (Constants.ETHER for native ETH). | ||
| /// @param amount The amount withdrawn. | ||
| event EmergencyWithdrawal(address indexed recipient, address indexed token, uint256 amount); | ||
|
|
||
| /// @notice Reverts when paused. | ||
| modifier whenNotPaused() { | ||
| if (paused()) revert CallPaused(); | ||
| _; | ||
| } | ||
|
|
||
| /// @notice Reverts if caller is not the proxy admin. | ||
| modifier onlyAdmin() { | ||
| address admin; | ||
| bytes32 adminSlot = 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. double checked, admin slot matches the one found in constants.sol https://github.com/ethereum-optimism/optimism/blob/3056348b21a07e584225310bbc27f1adce5ca2a9/packages/contracts-bedrock/src/libraries/Constants.sol#L29-L31 follows transparent proxy pattern, where admin was already set on proxy itself, looks good |
||
| assembly { | ||
| admin := sload(adminSlot) | ||
| } | ||
| require(msg.sender == admin, "Portal: caller is not admin"); | ||
| _; | ||
| } | ||
|
|
||
| /// @notice Semantic version. | ||
| /// @custom:semver 1.0.0 | ||
| /// @custom:semver 1.1.0 | ||
| function version() public pure virtual returns (string memory) { | ||
| return "1.0.0"; | ||
| return "1.1.0"; | ||
| } | ||
|
|
||
| /// @notice Constructs the OptimismPortal contract. | ||
|
|
@@ -489,6 +506,48 @@ contract Portal is Initializable, ResourceMetering, ISemver { | |
| ); | ||
| } | ||
|
|
||
| /// @notice Allows owner to withdraw the gas paying token held by the portal. | ||
| /// Can be called regardless of pause state. | ||
| /// @param _recipient The address to receive the withdrawn funds. | ||
| function chainOwnerExitPortal(address _recipient) external onlyAdmin { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: function selectors clashing not a problem, but do we want to name this something more explicit? maybe something like |
||
| chainOwnerExitPortal(address(0), _recipient); | ||
| } | ||
|
|
||
| /// @notice Allows owner to withdraw all tokens held by the portal. | ||
| /// Can be called regardless of pause state. | ||
| /// @param _asset The token address to withdraw, or address(0) for the gas paying token. | ||
| /// @param _recipient The address to receive the withdrawn funds. | ||
| function chainOwnerExitPortal(address _asset, address _recipient) public onlyAdmin { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: assumption is that once this is called, the chain is effectively "dead". Just need to make sure users who still have funds on the l3 is made aware and at that point, it'd be up to the recipient to properly distribute funds
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, it is on the onus of the team to ensure their users are aware they are taking control and will distribute the funds properly |
||
| require(_recipient != address(0), "Portal: zero recipient"); | ||
|
|
||
| address token; | ||
| if (_asset != address(0)) { | ||
| token = _asset; | ||
| } else { | ||
| (token,) = gasPayingToken(); | ||
| } | ||
|
|
||
| uint256 amount; | ||
| if (token == Constants.ETHER) { | ||
| amount = address(this).balance; | ||
| if (amount > 0) { | ||
| (bool success,) = _recipient.call{value: amount}(""); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: we give external call + execution control to recipient address here. However, its assumed the recipient is a trusted address and wouldn't try to do something malicious like reenter somewhere else in the code |
||
| require(success, "Portal: ETH transfer failed"); | ||
| } | ||
| } else { | ||
| amount = IERC20(token).balanceOf(address(this)); | ||
| if (amount > 0) { | ||
| (address gasToken,) = gasPayingToken(); | ||
| if (token == gasToken) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably a nit but seems like we should be doing this after the safe transfer? Cant recall if safeTransfer reverts or returns a bool as well
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if safe transfer reverts the entire transaction reverts |
||
| _balance = 0; | ||
| } | ||
| IERC20(token).safeTransfer(_recipient, amount); | ||
| } | ||
| } | ||
|
|
||
| emit EmergencyWithdrawal(_recipient, token, amount); | ||
| } | ||
|
|
||
| /// @notice Determine if a given output is finalized. | ||
| /// Reverts if the call to l2Oracle.getL2Output reverts. | ||
| /// Returns a boolean otherwise. | ||
|
|
@@ -504,5 +563,4 @@ contract Portal is Initializable, ResourceMetering, ISemver { | |
| /// @return Whether or not the finalization period has elapsed. | ||
| function _isFinalizationPeriodElapsed(uint256 _timestamp) internal view returns (bool) { | ||
| return block.timestamp > _timestamp; | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think removing this bracket causes the code to not compile anymore |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,278 @@ | ||
| // SPDX-License-Identifier: MIT | ||
| pragma solidity ^0.8.15; | ||
|
|
||
| import {Test} from "forge-std/Test.sol"; | ||
| import {ProxyAdmin} from "@eth-optimism-bedrock/src/universal/ProxyAdmin.sol"; | ||
| import {Proxy} from "@eth-optimism-bedrock/src/universal/Proxy.sol"; | ||
| import {ISuperchainConfig} from "@eth-optimism-bedrock/src/L1/interfaces/ISuperchainConfig.sol"; | ||
| import {ISystemConfig} from "@eth-optimism-bedrock/src/L1/interfaces/ISystemConfig.sol"; | ||
| import {IResourceMetering} from "@eth-optimism-bedrock/src/L1/interfaces/IResourceMetering.sol"; | ||
| import {Constants} from "@eth-optimism-bedrock/src/libraries/Constants.sol"; | ||
| import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; | ||
|
|
||
| import {Portal} from "../src/Portal.sol"; | ||
| import {OutputOracle} from "../src/OutputOracle.sol"; | ||
|
|
||
| /// @notice Mock ERC20 token for testing | ||
| contract MockERC20 is ERC20 { | ||
| constructor() ERC20("Mock Token", "MCK") {} | ||
|
|
||
| function mint(address to, uint256 amount) external { | ||
| _mint(to, amount); | ||
| } | ||
|
|
||
| function decimals() public pure override returns (uint8) { | ||
| return 18; | ||
| } | ||
| } | ||
|
|
||
| /// @notice Mock SuperchainConfig for testing | ||
| contract MockSuperchainConfig is ISuperchainConfig { | ||
| bool internal _paused; | ||
| address internal _guardian; | ||
|
|
||
| function setPaused(bool paused_) external { | ||
| _paused = paused_; | ||
| } | ||
|
|
||
| function paused() external view returns (bool) { | ||
| return _paused; | ||
| } | ||
|
|
||
| function guardian() external view returns (address) { | ||
| return _guardian; | ||
| } | ||
|
|
||
| function GUARDIAN_SLOT() external pure returns (bytes32) { | ||
| return bytes32(0); | ||
| } | ||
|
|
||
| function PAUSED_SLOT() external pure returns (bytes32) { | ||
| return bytes32(0); | ||
| } | ||
|
|
||
| function initialize(address, bool) external {} | ||
|
|
||
| function pause(string memory) external {} | ||
|
|
||
| function unpause() external {} | ||
|
|
||
| function version() external pure returns (string memory) { | ||
| return "1.0.0"; | ||
| } | ||
| } | ||
|
|
||
| /// @notice Mock SystemConfig for testing | ||
| contract MockSystemConfig { | ||
| address internal _gasPayingToken; | ||
|
|
||
| function setGasPayingToken(address token) external { | ||
| _gasPayingToken = token; | ||
| } | ||
|
|
||
| function gasPayingToken() external view returns (address, uint8) { | ||
| if (_gasPayingToken == address(0)) { | ||
| return (Constants.ETHER, 18); | ||
| } | ||
| return (_gasPayingToken, 18); | ||
| } | ||
|
|
||
| function resourceConfig() external pure returns (IResourceMetering.ResourceConfig memory) { | ||
| return IResourceMetering.ResourceConfig({ | ||
| maxResourceLimit: 20_000_000, | ||
| elasticityMultiplier: 10, | ||
| baseFeeMaxChangeDenominator: 8, | ||
| minimumBaseFee: 1 gwei, | ||
| systemTxMaxGas: 1_000_000, | ||
| maximumBaseFee: type(uint128).max | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| contract PortalTest is Test { | ||
| Portal internal portalImpl; | ||
| Portal internal portal; | ||
| ProxyAdmin internal admin; | ||
| Proxy internal proxy; | ||
| MockSuperchainConfig internal superchainConfig; | ||
| MockSystemConfig internal systemConfig; | ||
| MockERC20 internal token; | ||
|
|
||
| address internal recipient = makeAddr("recipient"); | ||
| address internal nonAdmin = makeAddr("nonAdmin"); | ||
|
|
||
| /// @notice EIP-1967 admin slot | ||
| bytes32 internal constant ADMIN_SLOT = 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103; | ||
|
|
||
| /// @notice Emitted when an emergency withdrawal is executed. | ||
| event EmergencyWithdrawal(address indexed recipient, address indexed token, uint256 amount); | ||
|
|
||
| function setUp() public { | ||
| // Deploy mocks | ||
| superchainConfig = new MockSuperchainConfig(); | ||
| systemConfig = new MockSystemConfig(); | ||
| token = new MockERC20(); | ||
|
|
||
| // Deploy Portal implementation | ||
| portalImpl = new Portal(); | ||
|
|
||
| // Deploy proxy with admin | ||
| admin = new ProxyAdmin(address(this)); | ||
| proxy = new Proxy(address(admin)); | ||
|
|
||
| // Upgrade proxy to Portal implementation | ||
| admin.upgrade(payable(address(proxy)), address(portalImpl)); | ||
|
|
||
| // Get Portal interface on proxy | ||
| portal = Portal(payable(address(proxy))); | ||
|
|
||
| // Initialize portal | ||
| portal.initialize({ | ||
| _l2Oracle: OutputOracle(address(0)), | ||
| _systemConfig: ISystemConfig(address(systemConfig)), | ||
| _superchainConfig: ISuperchainConfig(address(superchainConfig)) | ||
| }); | ||
| } | ||
|
|
||
| function test_emergencyWithdraw_ETH_success() public { | ||
| // Fund the portal with ETH | ||
| uint256 amount = 10 ether; | ||
| vm.deal(address(portal), amount); | ||
|
|
||
| uint256 recipientBalanceBefore = recipient.balance; | ||
|
|
||
| // Admin withdraws (admin is ProxyAdmin, which is owned by address(this)) | ||
| // But the actual proxy admin is the ProxyAdmin contract, so we need to call from it | ||
| // Actually, the admin slot stores the ProxyAdmin address, so we need to prank as ProxyAdmin | ||
| vm.prank(address(admin)); | ||
| portal.emergencyWithdraw(recipient); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the test suit is testing anything at the moment. We reference the |
||
|
|
||
| assertEq(address(portal).balance, 0); | ||
| assertEq(recipient.balance, recipientBalanceBefore + amount); | ||
| } | ||
|
|
||
| function test_emergencyWithdraw_ERC20_success() public { | ||
| // Set up custom gas token | ||
| systemConfig.setGasPayingToken(address(token)); | ||
|
|
||
| // Fund the portal with ERC20 tokens | ||
| uint256 amount = 1000 ether; | ||
| token.mint(address(portal), amount); | ||
|
|
||
| uint256 recipientBalanceBefore = token.balanceOf(recipient); | ||
|
|
||
| // Admin withdraws | ||
| vm.prank(address(admin)); | ||
| portal.emergencyWithdraw(recipient); | ||
|
|
||
| assertEq(token.balanceOf(address(portal)), 0); | ||
| assertEq(token.balanceOf(recipient), recipientBalanceBefore + amount); | ||
| } | ||
|
|
||
| function test_emergencyWithdraw_onlyAdmin_reverts() public { | ||
| // Fund the portal | ||
| vm.deal(address(portal), 10 ether); | ||
|
|
||
| // Non-admin tries to withdraw | ||
| vm.prank(nonAdmin); | ||
| vm.expectRevert("Portal: caller is not admin"); | ||
| portal.emergencyWithdraw(recipient); | ||
| } | ||
|
|
||
| function test_emergencyWithdraw_zeroRecipient_reverts() public { | ||
| // Fund the portal | ||
| vm.deal(address(portal), 10 ether); | ||
|
|
||
| // Admin tries to withdraw to zero address | ||
| vm.prank(address(admin)); | ||
| vm.expectRevert("Portal: zero recipient"); | ||
| portal.emergencyWithdraw(address(0)); | ||
| } | ||
|
|
||
| function test_emergencyWithdraw_worksWhenPaused() public { | ||
| // Fund the portal | ||
| uint256 amount = 10 ether; | ||
| vm.deal(address(portal), amount); | ||
|
|
||
| // Pause the superchain config | ||
| superchainConfig.setPaused(true); | ||
| assertTrue(portal.paused()); | ||
|
|
||
| uint256 recipientBalanceBefore = recipient.balance; | ||
|
|
||
| // Admin can still withdraw even when paused | ||
| vm.prank(address(admin)); | ||
| portal.emergencyWithdraw(recipient); | ||
|
|
||
| assertEq(address(portal).balance, 0); | ||
| assertEq(recipient.balance, recipientBalanceBefore + amount); | ||
| } | ||
|
|
||
| function test_emergencyWithdraw_emitsEvent() public { | ||
| // Fund the portal | ||
| uint256 amount = 10 ether; | ||
| vm.deal(address(portal), amount); | ||
|
|
||
| // Expect the EmergencyWithdrawal event | ||
| vm.expectEmit(true, true, false, true); | ||
| emit EmergencyWithdrawal(recipient, Constants.ETHER, amount); | ||
|
|
||
| vm.prank(address(admin)); | ||
| portal.emergencyWithdraw(recipient); | ||
| } | ||
|
|
||
| function test_emergencyWithdraw_emitsEvent_ERC20() public { | ||
| // Set up custom gas token | ||
| systemConfig.setGasPayingToken(address(token)); | ||
|
|
||
| // Fund the portal | ||
| uint256 amount = 1000 ether; | ||
| token.mint(address(portal), amount); | ||
|
|
||
| // Expect the EmergencyWithdrawal event | ||
| vm.expectEmit(true, true, false, true); | ||
| emit EmergencyWithdrawal(recipient, address(token), amount); | ||
|
|
||
| vm.prank(address(admin)); | ||
| portal.emergencyWithdraw(recipient); | ||
| } | ||
|
|
||
| function test_emergencyWithdraw_zeroBalance_ETH() public { | ||
| // Portal has no ETH | ||
| assertEq(address(portal).balance, 0); | ||
|
|
||
| uint256 recipientBalanceBefore = recipient.balance; | ||
|
|
||
| // Admin withdraws (should succeed with 0 amount) | ||
| vm.expectEmit(true, true, false, true); | ||
| emit EmergencyWithdrawal(recipient, Constants.ETHER, 0); | ||
|
|
||
| vm.prank(address(admin)); | ||
| portal.emergencyWithdraw(recipient); | ||
|
|
||
| assertEq(recipient.balance, recipientBalanceBefore); | ||
| } | ||
|
|
||
| function test_emergencyWithdraw_zeroBalance_ERC20() public { | ||
| // Set up custom gas token | ||
| systemConfig.setGasPayingToken(address(token)); | ||
|
|
||
| // Portal has no tokens | ||
| assertEq(token.balanceOf(address(portal)), 0); | ||
|
|
||
| uint256 recipientBalanceBefore = token.balanceOf(recipient); | ||
|
|
||
| // Admin withdraws (should succeed with 0 amount) | ||
| vm.expectEmit(true, true, false, true); | ||
| emit EmergencyWithdrawal(recipient, address(token), 0); | ||
|
|
||
| vm.prank(address(admin)); | ||
| portal.emergencyWithdraw(recipient); | ||
|
|
||
| assertEq(token.balanceOf(recipient), recipientBalanceBefore); | ||
| } | ||
|
|
||
| function test_version() public view { | ||
| assertEq(portal.version(), "1.1.0"); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.