feat: upgrade bridge to allow for transfer of all token assets#103
feat: upgrade bridge to allow for transfer of all token assets#103
Conversation
| amount = IERC20(token).balanceOf(address(this)); | ||
| if (amount > 0) { | ||
| (address gasToken,) = gasPayingToken(); | ||
| if (token == gasToken) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
if safe transfer reverts the entire transaction reverts
| /// @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.
i think removing this bracket causes the code to not compile anymore
contracts/test/Portal.t.sol
Outdated
| // 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.
I don't think the test suit is testing anything at the moment. We reference the emergencyWithdraw() function here, but the function we want to test is called chainOwnerExitPortal()
| if (token == Constants.ETHER) { | ||
| amount = address(this).balance; | ||
| if (amount > 0) { | ||
| (bool success,) = _recipient.call{value: amount}(""); |
There was a problem hiding this comment.
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
| /// @notice Reverts if caller is not the proxy admin. | ||
| modifier onlyAdmin() { | ||
| address admin; | ||
| bytes32 adminSlot = 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103; |
There was a problem hiding this comment.
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
contracts/src/Portal.sol
Outdated
| /// @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.
nit: function selectors clashing not a problem, but do we want to name this something more explicit?
maybe something like chainOwnerExitPortalGasToken()
contracts/src/Portal.sol
Outdated
| /// 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.
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
There was a problem hiding this comment.
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
| ); | ||
| } | ||
|
|
||
| function setChainOwner(address _chainOwner) external onlyAdmin { |
There was a problem hiding this comment.
New chain owner setter.
Created a setter rather than setting on initialize for simplicity since the proxy will have already initialized the other fields.
There was a problem hiding this comment.
Only Admin can call setChainOwner. The expected pattern is to call when using ProxyAdmin's upgradeAndCall method
| uint256 internal _balance; | ||
|
|
||
| // Owner of the current chain | ||
| address public chainOwner; |
There was a problem hiding this comment.
New chainOwner variable.
The ProxyAdmin contract only have two methods upgrade and upgradeAndCall.
Since ResolvingProxy will not delegate any calls from the ProxyAdmin you have to reset the implementation contract each time if the chain owner had multiple assets it wished to exit.
|
|
||
| /// @notice Emitted when a chain owner is set | ||
| /// @param chainOwner address of the chain owner | ||
| event ChainOwnerSet(address chainOwner); |
| /// @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); |
There was a problem hiding this comment.
renamed chainOwnerExitWithdrawal event
| _; | ||
| } | ||
|
|
||
| modifier onlyChainOwner() { |
| 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"); | ||
|
|
There was a problem hiding this comment.
Both functions now role restricted via modifier to chain owner
| uint256 internal _balance; | ||
|
|
||
| // Owner of the current chain | ||
| address public chainOwner; |
|
|
||
| /// @notice Emitted when a chain owner is set | ||
| /// @param chainOwner address of the chain owner | ||
| event ChainOwnerSet(address chainOwner); |
contracts/src/Portal.sol
Outdated
| ); | ||
| } | ||
|
|
||
| function setChainOwner(address _chainOwner) external onlyAdmin { |
There was a problem hiding this comment.
modifier only by ProxyAdmin expectation is called during implementation upgrade via upgradeAndCall
There was a problem hiding this comment.
change this to allow owner to update as well
There was a problem hiding this comment.
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
contracts/src/Portal.sol
Outdated
| 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; |
There was a problem hiding this comment.
Modifier changed to onlyChainOwner
contracts/src/Portal.sol
Outdated
| _; | ||
| } | ||
|
|
||
| modifier onlyChainOwner() { |
| token = new MockERC20(); | ||
|
|
||
| // Deploy Portal implementation | ||
| portalImpl = new PortalWithChainExit(); |
There was a problem hiding this comment.
Test expect contract to be named PortalWithChainExit
4 participants
Upgrade the optimism portal contract to support withdrawal of all locked funds.