Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions contracts/remappings.txt
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,4 @@ src/universal/=lib/optimism/packages/contracts-bedrock/src/universal/
src/vendor/=lib/optimism/packages/contracts-bedrock/src/vendor/
scripts/=lib/optimism/packages/contracts-bedrock/scripts/
@nitro-validator/=lib/nitro-validator/src/
src/legacy/=lib/optimism/packages/contracts-bedrock/src/legacy/
15 changes: 15 additions & 0 deletions contracts/script/DeployPortalWithChainExit.s.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.15;

import {Script} from "forge-std/Script.sol";
import {PortalWithChainExit} from "../src/PortalWithChainExit.sol";
import {console2 as console} from "forge-std/console2.sol";

contract DeployPortalWithChainExit is Script {
function run() public {
vm.startBroadcast();
PortalWithChainExit portalWithChainExit = new PortalWithChainExit();
vm.stopBroadcast();
console.log("Deploying PortalWithChainOwnerExit at:", address(portalWithChainExit));
}
}
83 changes: 80 additions & 3 deletions contracts/src/Portal.sol
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ contract Portal is Initializable, ResourceMetering, ISemver {
/// It is not safe to trust `ERC20.balanceOf` as it may lie.
uint256 internal _balance;

// Owner of the current chain
address public chainOwner;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

New chainOwner Variable



/// @notice Emitted when a transaction is deposited from L1 to L2.
/// The parameters of this event are read by the rollup node and used to derive deposit
/// transactions on L2.
Expand All @@ -90,16 +94,42 @@ contract Portal is Initializable, ResourceMetering, ISemver {
/// @param success Whether the withdrawal transaction was successful.
event WithdrawalFinalized(bytes32 indexed withdrawalHash, bool success);

/// @notice Emitted when a chain owner is set
/// @param chainOwner address of the chain owner
event ChainOwnerSet(address chainOwner);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

New ChainOwnerEvent


/// @notice Emitted when the chain owner executes a withdrawal.
/// @param recipient The address that received the funds.
/// @param token The token address (Constants.ETHER for native ETH).
/// @param amount The amount withdrawn.
event ChainOwnerExitWithdrawal(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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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");
_;
}

modifier onlyChainOwner() {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

new chainOwnerModifier

require(msg.sender == chainOwner, "Portal: caller is not chain owner");
_;
}

/// @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.
Expand Down Expand Up @@ -489,6 +519,53 @@ contract Portal is Initializable, ResourceMetering, ISemver {
);
}

function setChainOwner(address _chainOwner) external onlyAdmin {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

new chainOwner Setter

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

modifier only by ProxyAdmin expectation is called during implementation upgrade via upgradeAndCall

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

change this to allow owner to update as well

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can we add a check here for address(0) ?

The odds of a mistake here are super low, but since it could lead to loss of funds I think it would be worth it to add here

chainOwner = _chainOwner;`
emit ChainOwnerSet(chainOwner);
}

/// @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 chainOwnerExitPortalNetworkToken(address _recipient) external onlyChainOwner {
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 onlyChainOwner {
require(_recipient != address(0), "Portal: zero recipient");

address token;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Modifier changed to onlyChainOwner

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}("");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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 ChainOwnerExitWithdrawal(_recipient, token, amount);
}

/// @notice Determine if a given output is finalized.
/// Reverts if the call to l2Oracle.getL2Output reverts.
/// Returns a boolean otherwise.
Expand All @@ -505,4 +582,4 @@ contract Portal is Initializable, ResourceMetering, ISemver {
function _isFinalizationPeriodElapsed(uint256 _timestamp) internal view returns (bool) {
return block.timestamp > _timestamp;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i think removing this bracket causes the code to not compile anymore

}
}
Loading
Loading