Feat/usdc bridge #1
Conversation
|
| /// @notice Reserve extra slots (to a total of 50) in the storage layout for future upgrades. | ||
| /// A gap size of 43 was chosen here, so that the first slot used in a child contract | ||
| /// would be a multiple of 50. | ||
| uint256[43] private __gap; |
There was a problem hiding this comment.
There are 2 new storage slots being used before this. By reducing the gap, the multiple is kept to 50 as it was saying in the original comment. I don't think it matters much though
There was a problem hiding this comment.
The was actually a necessary change. If we left it at 45, then we would have caused a storage shift for other contracts that are serialized after the USDC bridge in the inheritance hierarchy (I haven't checked if there were other contracts after the USDC bridge, but changing it to 43 was the safe choice. )
It isn’t safe to simply add a state variable because it "shifts down" all of the state variables below in the inheritance chain.
There was a problem hiding this comment.
It is only important to contract changes to contracts that are already deployed though, so Sander is right that if this is the first deployment of the contract, it is not necessary. ⬆️
| // usdc has no burnFrom, so first transfer to the contract and then burn. | ||
| IERC20(_localToken).safeTransferFrom(_from, address(this), _amount); | ||
| IPartialUsdc(_localToken).burn(_amount); |
There was a problem hiding this comment.
Just for my understanding this is executed on the L2? And just to check _localToken is Circle's FiatToken? That can only by burned by the owner right? Which is this contract?
There was a problem hiding this comment.
Just for my understanding this is executed on the L2?
yes
And just to check _localToken is Circle's FiatToken?
yes
That can only by burned by the owner right?
Can only be burned by addresses that have been assigned the minter role, as is the case for the bridge.
Which is this contract?
There was a problem hiding this comment.
IGNORE, turned out to be false:
Hmmm, I don't think the code is correct.
the function starts with:
require(_isCorrectUsdcTokenPair(_localToken, _remoteToken), "Invalid token pair");
and if you look into _isCorrectUsdcTokenPair for L2UsdcBridge
function _isCorrectUsdcTokenPair(address _localToken, address _remoteToken) internal view override returns (bool) {
return _isL2Usdc(_localToken) && _isL1Usdc(_remoteToken);
}
So if the require doesn't fail, then at this point I know that
_isL2Usdc(_localToken) && _isL1Usdc(_remoteToken)
is true, so why do we have this if statement?
(_isL1Usdc(_localToken) && _isL2Usdc(_remoteToken)) when it will always be false?
..................................
Let's just think about this USDCBridge part of the L2UsdcBridge. If someone calls bridgeERC20To in our case, then someone want's to initiate an L2->L1 transfer. If someone initiated an L1->L2 transfer, then finalizeBridgeERC20 would have been called on the L2UsdcBridge and not the bridgeERC20To function.
I think similar problem exists with the code in the finalizeBridgeERC20 method, but haven't checked.
..................................
The if/else statement in the original Optimum code is used to differentiate between different token implementations I think, for example:
_initiateBridgeERC20 case
both branches initiate an L2->L1 transfer, but the first one works with 'burneable' tokens while the other token implementation might not be able to burn. For this second implementation we just transfer the local token from the user to the bridge and do a little bit of accounting in the deposit array to know how much of that non-burneable token is still there.
if (_isOptimismMintableERC20(_localToken)) {
require(
_isCorrectTokenPair(_localToken, _remoteToken),
"StandardBridge: wrong remote token for Optimism Mintable ERC20 local token"
);
OptimismMintableERC20(_localToken).burn(_from, _amount);
} else {
IERC20(_localToken).safeTransferFrom(_from, address(this), _amount);
deposits[_localToken][_remoteToken] = deposits[_localToken][_remoteToken] + _amount;
}
```~~
sander2
force-pushed
the
feat/usdc-bridge
branch
from
78af3d3
to
a80a286
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function transferUSDCRoles(address owner) external { | ||
| require(msg.sender == whitelistedTakeoverOrigin, "Unauthorized transfer"); | ||
| require(owner != address(0), "Can not transfer ownership to the zero address"); | ||
|
|
||
| // Change proxy admin | ||
| IUsdcProxy(tokenProxyAddress).changeAdmin(owner); | ||
|
|
||
| // remove minter | ||
| IMasterMinter(masterMinterAddress).removeMinter(); | ||
|
|
||
| // Transfer implementation owner | ||
| IUsdcImpl(tokenProxyAddress).transferOwnership(owner); | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
| @@ -0,0 +1,119 @@ | |||
| // SPDX-License-Identifier: MIT | |||
There was a problem hiding this comment.
Why not just use the Pausable from the openzeppelin package? How is this different from the original Pausable contract?
A bridge for USDC that complies with the bridged USDC standard
Overview
We use a modified version of the optimism StandardBridge. The StandardBridge was taken from version 1.7.1, which is newer than what we will use at launch. I tried packporting my changes to the version we'll actually be running, but the newer version actually seems to handle proxy support better (older version uses semver that is stored in variables that are marked
immutable, which is problematic in code used through a proxy).The changes compared to stock, are:
IOptimismMintableERC20requirement. Instead, the contract is initialized with a concrete pair of L1Token and L2Token addresses.Ownable. The owner will be a multisig controlled by a set of signers from different legal entities.We deploy USDC from Circle's
stablecoin-evmtoken, at commit657375471bff72afa5f625083bbab8003eb5f8c9. This includes theMasterMintercontract.We will deploy a
UsdcManagercontract that will be set as the MasterMinter owner, the USDC implementation contract owner, and the USDC proxy contract admin. This owner of this contract can whitelist an address that is owned by Circle, after which this whitelisted account can calltransferUSDCRolesto transfer the proxy and implementation ownership to a specified account. This call also removes minting priviledges from the bridge.Role summary
UsdcManagercontractUsdcManagerwill be Bob, which can whitelist one address that all UsdcManager roles can be transfered to. Once an address is whitelisted, it is locked in (i.e. it can not be changed later on), and the roles can then only be transferred to that address .