diff --git a/packages/protocol/contracts/tokenvault/BridgedERC20.sol b/packages/protocol/contracts/tokenvault/BridgedERC20.sol index 98e40f864dc..3ef0e4fe782 100644 --- a/packages/protocol/contracts/tokenvault/BridgedERC20.sol +++ b/packages/protocol/contracts/tokenvault/BridgedERC20.sol @@ -106,14 +106,6 @@ contract BridgedERC20 is return (srcToken, srcChainId); } - function _mintToken(address _account, uint256 _amount) internal override { - return _mint(_account, _amount); - } - - function _burnToken(address _from, uint256 _amount) internal override { - return _burn(_from, _amount); - } - /// @dev For ERC20SnapshotUpgradeable and ERC20VotesUpgradeable, need to implement the following /// functions function _beforeTokenTransfer( @@ -145,7 +137,7 @@ contract BridgedERC20 is uint256 _amount ) internal - override(ERC20Upgradeable, ERC20VotesUpgradeable) + override(ERC20Upgradeable, ERC20VotesUpgradeable, BridgedERC20Base) { return super._mint(_to, _amount); } @@ -155,7 +147,7 @@ contract BridgedERC20 is uint256 _amount ) internal - override(ERC20Upgradeable, ERC20VotesUpgradeable) + override(ERC20Upgradeable, ERC20VotesUpgradeable, BridgedERC20Base) { return super._burn(_from, _amount); } diff --git a/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol b/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol index df7b834dfd2..b5bccfbf293 100644 --- a/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol +++ b/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol @@ -68,26 +68,25 @@ abstract contract BridgedERC20Base is EssentialContract, IBridgedERC20 { revert BB_PERMISSION_DENIED(); } - _mintToken(_account, _amount); + _mint(_account, _amount); } - /// @notice Burns tokens from the specified account. - /// @param _account The address of the account to burn the tokens from. + /// @notice Burns tokens in case of 'migrating out' from msg.sender (EOA) or from the ERC20Vault + /// if bridging back to canonical token. /// @param _amount The amount of tokens to burn. - function burn(address _account, uint256 _amount) external whenNotPaused nonReentrant { + function burn(uint256 _amount) external whenNotPaused nonReentrant { if (_isMigratingOut()) { - // Only the owner of the tokens himself can migrate out - if (msg.sender != _account) revert BB_PERMISSION_DENIED(); // Outbound migration - emit MigratedTo(migratingAddress, _account, _amount); + emit MigratedTo(migratingAddress, msg.sender, _amount); // Ask the new bridged token to mint token for the user. - IBridgedERC20(migratingAddress).mint(_account, _amount); + IBridgedERC20(migratingAddress).mint(msg.sender, _amount); } else if (msg.sender != resolve(LibStrings.B_ERC20_VAULT, true)) { - // Only the vault can burn tokens when not migrating out + // When user wants to burn tokens only during 'migrating out' phase is possible. If + // ERC20Vault burns the tokens, that will go through the burn(amount) function. revert RESOLVER_DENIED(); } - _burnToken(_account, _amount); + _burn(msg.sender, _amount); } /// @notice Returns the owner. @@ -96,9 +95,9 @@ abstract contract BridgedERC20Base is EssentialContract, IBridgedERC20 { return super.owner(); } - function _mintToken(address _account, uint256 _amount) internal virtual; + function _mint(address _account, uint256 _amount) internal virtual; - function _burnToken(address _from, uint256 _amount) internal virtual; + function _burn(address _from, uint256 _amount) internal virtual; function _isMigratingOut() private view returns (bool) { return migratingAddress != address(0) && !migratingInbound; diff --git a/packages/protocol/contracts/tokenvault/ERC20Vault.sol b/packages/protocol/contracts/tokenvault/ERC20Vault.sol index 3c4f5e05dcb..847a0ec1a0a 100644 --- a/packages/protocol/contracts/tokenvault/ERC20Vault.sol +++ b/packages/protocol/contracts/tokenvault/ERC20Vault.sol @@ -327,6 +327,8 @@ contract ERC20Vault is BaseVault { IERC20(token_).safeTransfer(_to, _amount); } else { token_ = _getOrDeployBridgedToken(_ctoken); + //For native bridged tokens (like USDC), the mint() signature is the same, so no need to + // check. IBridgedERC20(token_).mint(_to, _amount); } } @@ -354,7 +356,11 @@ contract ERC20Vault is BaseVault { // If it's a bridged token if (bridgedToCanonical[_token].addr != address(0)) { ctoken_ = bridgedToCanonical[_token]; - IBridgedERC20(_token).burn(msg.sender, _amount); + + // Following the "transfer and burn" pattern, as used by USDC + IERC20(_token).safeTransferFrom(_user, address(this), _amount); + IBridgedERC20(_token).burn(_amount); + balanceChange_ = _amount; } else { // If it's a canonical token diff --git a/packages/protocol/contracts/tokenvault/IBridgedERC20.sol b/packages/protocol/contracts/tokenvault/IBridgedERC20.sol index 98b13cedcbd..1f85877570e 100644 --- a/packages/protocol/contracts/tokenvault/IBridgedERC20.sol +++ b/packages/protocol/contracts/tokenvault/IBridgedERC20.sol @@ -13,12 +13,13 @@ interface IBridgedERC20 { /// @param _amount The amount of tokens to mint. function mint(address _account, uint256 _amount) external; - /// @notice Burns `amount` tokens from the `from` address. - /// @param _from The account from which the tokens will be burned. + /// @notice Burns tokens from msg.sender. This is only allowed if: + /// - 1) tokens are migrating out to a new bridged token + /// - 2) The token is burned by ERC20Vault to bridge back to the canonical chain. /// @param _amount The amount of tokens to burn. - function burn(address _from, uint256 _amount) external; + function burn(uint256 _amount) external; - /// @notice Start or stop migration to/from a specified contract. + /// @notice Starts or stops migration to/from a specified contract. /// @param _addr The address migrating 'to' or 'from'. /// @param _inbound If false then signals migrating 'from', true if migrating 'into'. function changeMigrationStatus(address _addr, bool _inbound) external; diff --git a/packages/protocol/contracts/tokenvault/adapters/USDCAdapter.sol b/packages/protocol/contracts/tokenvault/adapters/USDCAdapter.sol deleted file mode 100644 index 90726e700ea..00000000000 --- a/packages/protocol/contracts/tokenvault/adapters/USDCAdapter.sol +++ /dev/null @@ -1,55 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity 0.8.24; - -import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; -import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; -import "../BridgedERC20Base.sol"; - -/// @title IUSDC -/// @custom:security-contact security@taiko.xyz -interface IUSDC is IERC20 { - /// @notice Burns a specific amount of tokens. - /// @param _amount The amount of token to be burned. - function burn(uint256 _amount) external; - - /// @notice Mints a specific amount of new tokens to an address. - /// @param _to The address that will receive the minted tokens. - /// @param _amount The amount of tokens to mint. - function mint(address _to, uint256 _amount) external; -} - -/// @title USDCAdapter -/// @custom:security-contact security@taiko.xyz -contract USDCAdapter is BridgedERC20Base { - using SafeERC20 for IUSDC; - - /// @notice The USDC instance. - /// @dev Slot 1. - IUSDC public usdc; - uint256[49] private __gap; - - error INVALID_PARAM(); - - /// @notice Initializes the contract. - /// @param _owner The owner of this contract. - /// @param _addressManager The address of the {AddressManager} contract. - /// @param _usdc The USDC instance. - function init(address _owner, address _addressManager, IUSDC _usdc) external initializer { - if (address(_usdc) == address(0)) revert INVALID_PARAM(); - - __Essential_init(_owner, _addressManager); - usdc = _usdc; - } - - function _mintToken(address _account, uint256 _amount) internal override { - usdc.mint(_account, _amount); - } - - function _burnToken(address _from, uint256 _amount) internal override { - // We don't need to worry about - // https://github.com/crytic/slither/wiki/Detector-Documentation#arbitrary-from-in-transferfrom - // as switching from an old bridged token to a new one can only be performed by the owner. - usdc.safeTransferFrom(_from, address(this), _amount); - usdc.burn(_amount); - } -} diff --git a/packages/protocol/script/DeployUSDCAdapter.s.sol b/packages/protocol/script/DeployUSDCAdapter.s.sol deleted file mode 100644 index 6dd53548f35..00000000000 --- a/packages/protocol/script/DeployUSDCAdapter.s.sol +++ /dev/null @@ -1,83 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity 0.8.24; - -import "../contracts/tokenvault/adapters/USDCAdapter.sol"; -import "../contracts/tokenvault/ERC20Vault.sol"; -import "../test/DeployCapability.sol"; - -/// @title DeployUSDCAdapter -/// @notice This script deploys the adapter contract for USDC. -contract DeployUSDCAdapter is DeployCapability { - address public usdcProxyL2 = vm.envAddress("NATIVE_USDC_PROXY_ON_L2"); - address public usdcProxyL1 = vm.envAddress("NATIVE_USDC_PROXY_ON_L1"); - address public l2SharedAddressManager = vm.envAddress("L2_SHARED_ADDRESS_MANAGER"); - address public erc20Vault = vm.envAddress("ERC20_VAULT_ADDRESS"); - address public erc20VaultOwner = vm.envAddress("ERC20_VAULT_OWNER"); - - uint256 public deployerPrivKey = vm.envUint("ADAPTOR_DEPLOYER_PRIVATE_KEY"); - uint256 public masterMinterPrivKey = vm.envUint("MASTER_MINTER_PRIVATE_KEY"); - - uint256 public erc20VaultOwnerPrivKey = vm.envUint("ERC20_VAULT_OWNER_PRIVATE_KEY"); - - // Fixed, not changed. (From circle contracts) - // https://holesky.etherscan.io/tx/0xbfb68e7d92506498553e09ee22aba0adc23401108d508fc92f56da5e42ffc828 - bytes4 public configureMinterSelector = 0x4e44d956; - - error CANNOT_CONFIGURE_MINTER(); - error CANNOT_CHANGE_BRIDGED_TOKEN(); - - function setUp() external { } - - function run() external { - require(deployerPrivKey != 0, "invalid deplyoer priv key"); - require(masterMinterPrivKey != 0, "invalid master minter priv key"); - vm.startBroadcast(deployerPrivKey); - // Verify this contract after deployment (!) - address adapterProxy = deployProxy({ - name: "usdc_adapter", - impl: address(new USDCAdapter()), - data: abi.encodeCall( - USDCAdapter.init, (address(0), l2SharedAddressManager, IUSDC(usdcProxyL2)) - ) - }); - - USDCAdapter(adapterProxy).transferOwnership(erc20VaultOwner); - - vm.stopBroadcast(); - - // Grant the adapter the minter role by master minter - vm.startBroadcast(masterMinterPrivKey); - (bool success, bytes memory retVal) = address(usdcProxyL2).call( - abi.encodeWithSelector(configureMinterSelector, adapterProxy, type(uint256).max) - ); - - if (!success) { - console2.log("Error is:"); - console2.logBytes(retVal); - revert CANNOT_CONFIGURE_MINTER(); - } - - vm.stopBroadcast(); - - vm.startBroadcast(erc20VaultOwnerPrivKey); - ERC20Vault.CanonicalERC20 memory canonicalToken = ERC20Vault.CanonicalERC20({ - chainId: 17_000, // On mainnet, Ethereum chainID - addr: usdcProxyL1, // On mainnet, USDC contract address - decimals: 6, - symbol: "USDC", - name: "USD Coin" - }); - - (success, retVal) = erc20Vault.call( - abi.encodeCall(ERC20Vault.changeBridgedToken, (canonicalToken, adapterProxy)) - ); - - if (!success) { - console2.log("Error is:"); - console2.logBytes(retVal); - revert CANNOT_CHANGE_BRIDGED_TOKEN(); - } - - vm.stopBroadcast(); - } -} diff --git a/packages/protocol/test/tokenvault/BridgedERC20.t.sol b/packages/protocol/test/tokenvault/BridgedERC20.t.sol index 7d9e24ddbd3..689a995db40 100644 --- a/packages/protocol/test/tokenvault/BridgedERC20.t.sol +++ b/packages/protocol/test/tokenvault/BridgedERC20.t.sol @@ -44,22 +44,24 @@ contract TestBridgedERC20 is TaikoTest { vm.stopPrank(); } - function test_20Vault_migration___only_vault_can_min_burn_when_migration_off() public { + function test_20Vault_migration___only_vault_can_min__but_cannot_burn_when_migration_off() + public + { BridgedERC20 btoken = deployBridgedToken("BAR"); // only erc20_vault can brun and mint - vm.startPrank(vault); + vm.prank(vault, vault); btoken.mint(Bob, 1000); - btoken.burn(Bob, 600); - assertEq(btoken.balanceOf(Bob), 400); + //Vault cannot burn only if it owns the tokens + vm.expectRevert(); + vm.prank(Bob, Bob); + btoken.burn(600); + assertEq(btoken.balanceOf(Bob), 1000); vm.stopPrank(); // Owner cannot burn/mint - vm.startPrank(owner); vm.expectRevert(); + vm.prank(owner, owner); btoken.mint(Bob, 1000); - vm.expectRevert(); - btoken.burn(Bob, 100); - vm.stopPrank(); } function test_20Vault_migration__old_to_new() public { @@ -88,23 +90,9 @@ contract TestBridgedERC20 is TaikoTest { vm.expectRevert(); oldToken.mint(Bob, 10); - // 2. burning can NOT be done by anyone - vm.prank(randAddress()); - vm.expectRevert(); - oldToken.burn(Bob, 10); - - // including the owners - vm.prank(oldToken.owner()); - vm.expectRevert(); - oldToken.burn(Bob, 10); - - vm.prank(newToken.owner()); - vm.expectRevert(); - oldToken.burn(Bob, 10); - - // but can be done by the token owner + // but can be done by the token owner - if migrating out phase vm.prank(Bob); - oldToken.burn(Bob, 10); + oldToken.burn(10); assertEq(oldToken.balanceOf(Bob), 90); assertEq(newToken.balanceOf(Bob), 210); @@ -122,17 +110,24 @@ contract TestBridgedERC20 is TaikoTest { newToken.mint(Bob, 15); assertEq(newToken.balanceOf(Bob), 225); - // 2. Nobody can burn except the vault - vm.prank(Bob); + // Vault can only burn if it owns the tokens + vm.prank(vault); vm.expectRevert(); - newToken.burn(Bob, 10); + newToken.burn(25); + assertEq(newToken.balanceOf(Bob), 225); - vm.prank(owner); - vm.expectRevert(); - newToken.burn(Bob, 10); + // Imitate current bridge-back operation, as Bob gave approval (for bridging back) and then + // ERC20Vault does the "transfer and burn" + vm.prank(Bob); + newToken.approve(vault, 25); + // Following the "transfer and burn" pattern vm.prank(vault); - newToken.burn(Bob, 25); + newToken.transferFrom(Bob, vault, 25); + + vm.prank(vault); + newToken.burn(25); + assertEq(newToken.balanceOf(Bob), 200); }