From 0de447cd56c4eb2bb1a2a9a3af6ebc4883cf3177 Mon Sep 17 00:00:00 2001 From: Daniel Wang Date: Mon, 22 Apr 2024 14:13:45 +0800 Subject: [PATCH 01/11] Delete ERC20 token adaptor --- .../contracts/tokenvault/BridgedERC20.sol | 12 +-- .../contracts/tokenvault/BridgedERC20Base.sol | 8 +- .../tokenvault/adapters/USDCAdapter.sol | 55 ------------ .../protocol/script/DeployUSDCAdapter.s.sol | 83 ------------------- 4 files changed, 6 insertions(+), 152 deletions(-) delete mode 100644 packages/protocol/contracts/tokenvault/adapters/USDCAdapter.sol delete mode 100644 packages/protocol/script/DeployUSDCAdapter.s.sol diff --git a/packages/protocol/contracts/tokenvault/BridgedERC20.sol b/packages/protocol/contracts/tokenvault/BridgedERC20.sol index 10c15e1b960..65e9630c25f 100644 --- a/packages/protocol/contracts/tokenvault/BridgedERC20.sol +++ b/packages/protocol/contracts/tokenvault/BridgedERC20.sol @@ -145,14 +145,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( @@ -184,7 +176,7 @@ contract BridgedERC20 is uint256 _amount ) internal - override(ERC20Upgradeable, ERC20VotesUpgradeable) + override(ERC20Upgradeable, ERC20VotesUpgradeable, BridgedERC20Base) { return super._mint(_to, _amount); } @@ -194,7 +186,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..e6324a6cabb 100644 --- a/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol +++ b/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol @@ -68,7 +68,7 @@ abstract contract BridgedERC20Base is EssentialContract, IBridgedERC20 { revert BB_PERMISSION_DENIED(); } - _mintToken(_account, _amount); + _mint(_account, _amount); } /// @notice Burns tokens from the specified account. @@ -87,7 +87,7 @@ abstract contract BridgedERC20Base is EssentialContract, IBridgedERC20 { revert RESOLVER_DENIED(); } - _burnToken(_account, _amount); + _burn(_account, _amount); } /// @notice Returns the owner. @@ -96,9 +96,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/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(); - } -} From 724962df67ee8cebace793777f03116bfa940eb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Keszey=20D=C3=A1niel?= Date: Mon, 22 Apr 2024 11:40:10 +0200 Subject: [PATCH 02/11] modify ERC20Vault to work with custom token burns --- .../protocol/contracts/common/LibStrings.sol | 3 ++ .../contracts/tokenvault/ERC20Vault.sol | 47 ++++++++++++++++++- 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/packages/protocol/contracts/common/LibStrings.sol b/packages/protocol/contracts/common/LibStrings.sol index 16a05bf8826..689f829721e 100644 --- a/packages/protocol/contracts/common/LibStrings.sol +++ b/packages/protocol/contracts/common/LibStrings.sol @@ -34,6 +34,9 @@ library LibStrings { /// @notice bytes32 representation of the string "bridged_erc20". bytes32 internal constant B_BRIDGED_ERC20 = bytes32("bridged_erc20"); + /// @notice bytes32 representation of the string "bridged_native_usdc". + bytes32 internal constant B_BRIDGED_NATIVE_USDC = bytes32("bridged_native_usdc"); + /// @notice bytes32 representation of the string "erc1155_vault". bytes32 internal constant B_ERC1155_VAULT = bytes32("erc1155_vault"); diff --git a/packages/protocol/contracts/tokenvault/ERC20Vault.sol b/packages/protocol/contracts/tokenvault/ERC20Vault.sol index 35579106c1e..2ac626932d0 100644 --- a/packages/protocol/contracts/tokenvault/ERC20Vault.sol +++ b/packages/protocol/contracts/tokenvault/ERC20Vault.sol @@ -5,6 +5,7 @@ import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import "../libs/LibAddress.sol"; +import "../common/LibStrings.sol"; import "./BridgedERC20.sol"; import "./BaseVault.sol"; @@ -133,9 +134,11 @@ contract ERC20Vault is BaseVault { error VAULT_BTOKEN_BLACKLISTED(); error VAULT_CTOKEN_MISMATCH(); + error VAULT_ERROR_IN_SPECIAL_OPS(); error VAULT_INVALID_TOKEN(); error VAULT_INVALID_AMOUNT(); error VAULT_INVALID_NEW_BTOKEN(); + error VAULT_NOT_POSSIBLE_SPECIAL_OPERATION(); error VAULT_INVALID_TO(); error VAULT_NOT_SAME_OWNER(); @@ -328,6 +331,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); } } @@ -355,7 +360,17 @@ 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); + + bytes[] memory encodedCalls = _getBurnLowLevelCalls(_token, _user, _amount); + if (encodedCalls.length != 0) { + for (uint256 i; i < encodedCalls.length; ++i) { + (bool result,) = _token.call(encodedCalls[i]); + + if (!result) revert VAULT_ERROR_IN_SPECIAL_OPS(); + } + } else { + IBridgedERC20(_token).burn(msg.sender, _amount); + } balanceChange_ = _amount; } else { // If it's a canonical token @@ -429,4 +444,34 @@ contract ERC20Vault is BaseVault { ctokenDecimal: ctoken.decimals }); } + + /// @dev If a token requires specific (e.g.: native USDC) operations to be burnt, then return + /// the low level calls. + /// @param toBeBurnt Token to be burnt. + /// @param user User trying to bridge. + /// @param amount Token amount to be burnt. + /// @return encodedCalls Address of the bridged token contract. + function _getBurnLowLevelCalls( + address toBeBurnt, + address user, + uint256 amount + ) + private + returns (bytes[] memory encodedCalls) + { + if (toBeBurnt == resolve(LibStrings.B_BRIDGED_NATIVE_USDC, true)) { + // If this is a native USDC, then we need to: + // 1.transfer tokens from user to ERC20Vault + // 2. burn (supposing ERC20Vault has minterRole) + encodedCalls = new bytes[](2); + // 1. transfer to ERC20Vault + encodedCalls[0] = abi.encodeWithSignature( + "transferFrom(address,address,uint)", user, address(this), amount + ); + // 2. burn + encodedCalls[1] = abi.encodeWithSignature("burn(uint)", amount); + } + // In case we will support other native tokens, custom logic can go here.. + //else if() {...} + } } From 46033226da36cce3312ab69197d6041e8ab2add3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Keszey=20D=C3=A1niel?= Date: Mon, 22 Apr 2024 11:41:10 +0200 Subject: [PATCH 03/11] fix resolve can return 0x0 --- packages/protocol/contracts/tokenvault/ERC20Vault.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/protocol/contracts/tokenvault/ERC20Vault.sol b/packages/protocol/contracts/tokenvault/ERC20Vault.sol index 2ac626932d0..77d9cac3ba1 100644 --- a/packages/protocol/contracts/tokenvault/ERC20Vault.sol +++ b/packages/protocol/contracts/tokenvault/ERC20Vault.sol @@ -459,7 +459,7 @@ contract ERC20Vault is BaseVault { private returns (bytes[] memory encodedCalls) { - if (toBeBurnt == resolve(LibStrings.B_BRIDGED_NATIVE_USDC, true)) { + if (toBeBurnt == resolve(LibStrings.B_BRIDGED_NATIVE_USDC, false)) { // If this is a native USDC, then we need to: // 1.transfer tokens from user to ERC20Vault // 2. burn (supposing ERC20Vault has minterRole) From 4606c5f2c773196c052acba037e44619dfc1206c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Keszey=20D=C3=A1niel?= Date: Mon, 22 Apr 2024 13:38:25 +0200 Subject: [PATCH 04/11] change to transfer and mint pattern with BridgedERC20 tokens --- .../contracts/tokenvault/BridgedERC20Base.sol | 16 +++++++ .../contracts/tokenvault/ERC20Vault.sol | 44 ++----------------- .../contracts/tokenvault/IBridgedERC20.sol | 5 +++ 3 files changed, 25 insertions(+), 40 deletions(-) diff --git a/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol b/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol index e6324a6cabb..19792b00b27 100644 --- a/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol +++ b/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol @@ -29,6 +29,7 @@ abstract contract BridgedERC20Base is EssentialContract, IBridgedERC20 { error BB_PERMISSION_DENIED(); error BB_INVALID_PARAMS(); + error BB_INVALID_BURN_AMOUNT(); error BB_MINT_DISALLOWED(); /// @notice Start or stop migration to/from a specified contract. @@ -90,6 +91,21 @@ abstract contract BridgedERC20Base is EssentialContract, IBridgedERC20 { _burn(_account, _amount); } + /// @notice Burns tokens from msg.sender, if this is the ERC20Vault. Otherwise do not allow + /// anyone to burn (e.g. USDC) or other coins, as it would be changing the supply (on other + /// chains too). + /// @param _amount The amount of tokens to burn. + function burn(uint256 _amount) + external + whenNotPaused + nonReentrant + onlyFromNamed(LibStrings.B_ERC20_VAULT) + { + if (_amount == 0) revert BB_INVALID_BURN_AMOUNT(); + //All the necessary other checks (balance, etc.) are handled in the ERC20Upgradeable._burn() + _burn(msg.sender, _amount); + } + /// @notice Returns the owner. /// @return The address of the owner. function owner() public view override(IBridgedERC20, OwnableUpgradeable) returns (address) { diff --git a/packages/protocol/contracts/tokenvault/ERC20Vault.sol b/packages/protocol/contracts/tokenvault/ERC20Vault.sol index 77d9cac3ba1..d54ab1b6a48 100644 --- a/packages/protocol/contracts/tokenvault/ERC20Vault.sol +++ b/packages/protocol/contracts/tokenvault/ERC20Vault.sol @@ -361,16 +361,10 @@ contract ERC20Vault is BaseVault { if (bridgedToCanonical[_token].addr != address(0)) { ctoken_ = bridgedToCanonical[_token]; - bytes[] memory encodedCalls = _getBurnLowLevelCalls(_token, _user, _amount); - if (encodedCalls.length != 0) { - for (uint256 i; i < encodedCalls.length; ++i) { - (bool result,) = _token.call(encodedCalls[i]); - - if (!result) revert VAULT_ERROR_IN_SPECIAL_OPS(); - } - } else { - 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 @@ -444,34 +438,4 @@ contract ERC20Vault is BaseVault { ctokenDecimal: ctoken.decimals }); } - - /// @dev If a token requires specific (e.g.: native USDC) operations to be burnt, then return - /// the low level calls. - /// @param toBeBurnt Token to be burnt. - /// @param user User trying to bridge. - /// @param amount Token amount to be burnt. - /// @return encodedCalls Address of the bridged token contract. - function _getBurnLowLevelCalls( - address toBeBurnt, - address user, - uint256 amount - ) - private - returns (bytes[] memory encodedCalls) - { - if (toBeBurnt == resolve(LibStrings.B_BRIDGED_NATIVE_USDC, false)) { - // If this is a native USDC, then we need to: - // 1.transfer tokens from user to ERC20Vault - // 2. burn (supposing ERC20Vault has minterRole) - encodedCalls = new bytes[](2); - // 1. transfer to ERC20Vault - encodedCalls[0] = abi.encodeWithSignature( - "transferFrom(address,address,uint)", user, address(this), amount - ); - // 2. burn - encodedCalls[1] = abi.encodeWithSignature("burn(uint)", amount); - } - // In case we will support other native tokens, custom logic can go here.. - //else if() {...} - } } diff --git a/packages/protocol/contracts/tokenvault/IBridgedERC20.sol b/packages/protocol/contracts/tokenvault/IBridgedERC20.sol index 98b13cedcbd..f802c556f98 100644 --- a/packages/protocol/contracts/tokenvault/IBridgedERC20.sol +++ b/packages/protocol/contracts/tokenvault/IBridgedERC20.sol @@ -18,6 +18,11 @@ interface IBridgedERC20 { /// @param _amount The amount of tokens to burn. function burn(address _from, uint256 _amount) external; + /// @notice Burns `amount` tokens from the `msg.sender` address - if the caller is the + /// ERC20Vault. + /// @param _amount The amount of tokens to burn. + function burn(uint256 _amount) external; + /// @notice Start or stop 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'. From 0e21f3cd5678a8a385f4d87b623dc3c1835f4c7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Keszey=20D=C3=A1niel?= Date: Mon, 22 Apr 2024 14:29:38 +0200 Subject: [PATCH 05/11] bridge cannot burn only if owns --- .../contracts/tokenvault/BridgedERC20Base.sol | 7 ++++--- .../protocol/test/tokenvault/BridgedERC20.t.sol | 15 ++++++++++----- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol b/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol index 19792b00b27..4999ee05dca 100644 --- a/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol +++ b/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol @@ -83,9 +83,10 @@ abstract contract BridgedERC20Base is EssentialContract, IBridgedERC20 { emit MigratedTo(migratingAddress, _account, _amount); // Ask the new bridged token to mint token for the user. IBridgedERC20(migratingAddress).mint(_account, _amount); - } else if (msg.sender != resolve(LibStrings.B_ERC20_VAULT, true)) { - // Only the vault can burn tokens when not migrating out - revert RESOLVER_DENIED(); + } else { + // When user wants to burn tokens only during 'migrating out' phase is possible. If + // ERC20Vault burns the tokens, that will go thru the burn(amount) function. + revert BB_MINT_DISALLOWED(); } _burn(_account, _amount); diff --git a/packages/protocol/test/tokenvault/BridgedERC20.t.sol b/packages/protocol/test/tokenvault/BridgedERC20.t.sol index 7d9e24ddbd3..42c26bc646c 100644 --- a/packages/protocol/test/tokenvault/BridgedERC20.t.sol +++ b/packages/protocol/test/tokenvault/BridgedERC20.t.sol @@ -44,13 +44,17 @@ 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); btoken.mint(Bob, 1000); + //Vault cannot burn only if it owns the tokens + vm.expectRevert(); btoken.burn(Bob, 600); - assertEq(btoken.balanceOf(Bob), 400); + assertEq(btoken.balanceOf(Bob), 1000); vm.stopPrank(); // Owner cannot burn/mint @@ -131,9 +135,10 @@ contract TestBridgedERC20 is TaikoTest { vm.expectRevert(); newToken.burn(Bob, 10); - vm.prank(vault); - newToken.burn(Bob, 25); - assertEq(newToken.balanceOf(Bob), 200); + // Vault cannot burn as itself only if owns the tokens. + // vm.prank(vault); + // newToken.burn(Bob, 25); + // assertEq(newToken.balanceOf(Bob), 200); } function deployBridgedToken(string memory name) internal returns (BridgedERC20) { From 2a2b73b3b6e230acdf53fbcb936027a40e59785f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Keszey=20D=C3=A1niel?= Date: Mon, 22 Apr 2024 17:53:50 +0200 Subject: [PATCH 06/11] remove old burn interface --- .../contracts/tokenvault/BridgedERC20Base.sol | 31 +++-------- .../contracts/tokenvault/IBridgedERC20.sol | 10 ++-- .../test/tokenvault/BridgedERC20.t.sol | 54 ++++++++----------- 3 files changed, 32 insertions(+), 63 deletions(-) diff --git a/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol b/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol index 4999ee05dca..8138b88a57e 100644 --- a/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol +++ b/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol @@ -72,38 +72,21 @@ abstract contract BridgedERC20Base is EssentialContract, IBridgedERC20 { _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 teh 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); - } else { + IBridgedERC20(migratingAddress).mint(msg.sender, _amount); + } else if (msg.sender != resolve(LibStrings.B_ERC20_VAULT, true)) { // When user wants to burn tokens only during 'migrating out' phase is possible. If // ERC20Vault burns the tokens, that will go thru the burn(amount) function. - revert BB_MINT_DISALLOWED(); + revert RESOLVER_DENIED(); } - _burn(_account, _amount); - } - - /// @notice Burns tokens from msg.sender, if this is the ERC20Vault. Otherwise do not allow - /// anyone to burn (e.g. USDC) or other coins, as it would be changing the supply (on other - /// chains too). - /// @param _amount The amount of tokens to burn. - function burn(uint256 _amount) - external - whenNotPaused - nonReentrant - onlyFromNamed(LibStrings.B_ERC20_VAULT) - { - if (_amount == 0) revert BB_INVALID_BURN_AMOUNT(); - //All the necessary other checks (balance, etc.) are handled in the ERC20Upgradeable._burn() _burn(msg.sender, _amount); } diff --git a/packages/protocol/contracts/tokenvault/IBridgedERC20.sol b/packages/protocol/contracts/tokenvault/IBridgedERC20.sol index f802c556f98..9d1ad319c5a 100644 --- a/packages/protocol/contracts/tokenvault/IBridgedERC20.sol +++ b/packages/protocol/contracts/tokenvault/IBridgedERC20.sol @@ -13,13 +13,9 @@ 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. - /// @param _amount The amount of tokens to burn. - function burn(address _from, uint256 _amount) external; - - /// @notice Burns `amount` tokens from the `msg.sender` address - if the caller is the - /// ERC20Vault. + /// @notice Burns `amount` tokens from the `msg.sender` address - either if tokens are migrating + /// out to a new bridged token or if the caller is the or if the caller is ERC20Vault (during + /// "bridging back"). /// @param _amount The amount of tokens to burn. function burn(uint256 _amount) external; diff --git a/packages/protocol/test/tokenvault/BridgedERC20.t.sol b/packages/protocol/test/tokenvault/BridgedERC20.t.sol index 42c26bc646c..689a995db40 100644 --- a/packages/protocol/test/tokenvault/BridgedERC20.t.sol +++ b/packages/protocol/test/tokenvault/BridgedERC20.t.sol @@ -49,21 +49,19 @@ contract TestBridgedERC20 is TaikoTest { { BridgedERC20 btoken = deployBridgedToken("BAR"); // only erc20_vault can brun and mint - vm.startPrank(vault); + vm.prank(vault, vault); btoken.mint(Bob, 1000); //Vault cannot burn only if it owns the tokens vm.expectRevert(); - btoken.burn(Bob, 600); + 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 { @@ -92,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); @@ -126,19 +110,25 @@ 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.transferFrom(Bob, vault, 25); + + vm.prank(vault); + newToken.burn(25); - // Vault cannot burn as itself only if owns the tokens. - // vm.prank(vault); - // newToken.burn(Bob, 25); - // assertEq(newToken.balanceOf(Bob), 200); + assertEq(newToken.balanceOf(Bob), 200); } function deployBridgedToken(string memory name) internal returns (BridgedERC20) { From 6782c8a039096dc30c2cd825f4a07b004ea1911b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Keszey=20D=C3=A1niel?= Date: Mon, 22 Apr 2024 17:54:41 +0200 Subject: [PATCH 07/11] remove constant string --- packages/protocol/contracts/common/LibStrings.sol | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/protocol/contracts/common/LibStrings.sol b/packages/protocol/contracts/common/LibStrings.sol index 689f829721e..16a05bf8826 100644 --- a/packages/protocol/contracts/common/LibStrings.sol +++ b/packages/protocol/contracts/common/LibStrings.sol @@ -34,9 +34,6 @@ library LibStrings { /// @notice bytes32 representation of the string "bridged_erc20". bytes32 internal constant B_BRIDGED_ERC20 = bytes32("bridged_erc20"); - /// @notice bytes32 representation of the string "bridged_native_usdc". - bytes32 internal constant B_BRIDGED_NATIVE_USDC = bytes32("bridged_native_usdc"); - /// @notice bytes32 representation of the string "erc1155_vault". bytes32 internal constant B_ERC1155_VAULT = bytes32("erc1155_vault"); From 1ec3a84f87cc71f37d69dd2f4b7789659aee2836 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Keszey=20D=C3=A1niel?= Date: Mon, 22 Apr 2024 17:58:38 +0200 Subject: [PATCH 08/11] remove unnecessary leftover --- packages/protocol/contracts/tokenvault/BridgedERC20Base.sol | 1 - packages/protocol/contracts/tokenvault/ERC20Vault.sol | 3 --- 2 files changed, 4 deletions(-) diff --git a/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol b/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol index 8138b88a57e..134994c5325 100644 --- a/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol +++ b/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol @@ -29,7 +29,6 @@ abstract contract BridgedERC20Base is EssentialContract, IBridgedERC20 { error BB_PERMISSION_DENIED(); error BB_INVALID_PARAMS(); - error BB_INVALID_BURN_AMOUNT(); error BB_MINT_DISALLOWED(); /// @notice Start or stop migration to/from a specified contract. diff --git a/packages/protocol/contracts/tokenvault/ERC20Vault.sol b/packages/protocol/contracts/tokenvault/ERC20Vault.sol index d54ab1b6a48..0af5fe89f35 100644 --- a/packages/protocol/contracts/tokenvault/ERC20Vault.sol +++ b/packages/protocol/contracts/tokenvault/ERC20Vault.sol @@ -5,7 +5,6 @@ import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import "../libs/LibAddress.sol"; -import "../common/LibStrings.sol"; import "./BridgedERC20.sol"; import "./BaseVault.sol"; @@ -134,11 +133,9 @@ contract ERC20Vault is BaseVault { error VAULT_BTOKEN_BLACKLISTED(); error VAULT_CTOKEN_MISMATCH(); - error VAULT_ERROR_IN_SPECIAL_OPS(); error VAULT_INVALID_TOKEN(); error VAULT_INVALID_AMOUNT(); error VAULT_INVALID_NEW_BTOKEN(); - error VAULT_NOT_POSSIBLE_SPECIAL_OPERATION(); error VAULT_INVALID_TO(); error VAULT_NOT_SAME_OWNER(); From ce32fecdc6f1fd337fa94adc6fa7a61f1284edf3 Mon Sep 17 00:00:00 2001 From: Daniel Wang <99078276+dantaik@users.noreply.github.com> Date: Tue, 23 Apr 2024 11:41:27 +0800 Subject: [PATCH 09/11] Update packages/protocol/contracts/tokenvault/BridgedERC20Base.sol --- packages/protocol/contracts/tokenvault/BridgedERC20Base.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol b/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol index 134994c5325..e3a9fb82d37 100644 --- a/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol +++ b/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol @@ -71,7 +71,7 @@ abstract contract BridgedERC20Base is EssentialContract, IBridgedERC20 { _mint(_account, _amount); } - /// @notice Burns tokens in case of 'migrating out' from msg.sender (EOA) or from teh ERC20Vault + /// @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(uint256 _amount) external whenNotPaused nonReentrant { From 879a78d923f16ac6fdfd822f9fadf8081c7743de Mon Sep 17 00:00:00 2001 From: Daniel Wang Date: Tue, 23 Apr 2024 11:46:02 +0800 Subject: [PATCH 10/11] Update IBridgedERC20.sol --- packages/protocol/contracts/tokenvault/IBridgedERC20.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/protocol/contracts/tokenvault/IBridgedERC20.sol b/packages/protocol/contracts/tokenvault/IBridgedERC20.sol index 9d1ad319c5a..1f85877570e 100644 --- a/packages/protocol/contracts/tokenvault/IBridgedERC20.sol +++ b/packages/protocol/contracts/tokenvault/IBridgedERC20.sol @@ -13,13 +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 `msg.sender` address - either if tokens are migrating - /// out to a new bridged token or if the caller is the or if the caller is ERC20Vault (during - /// "bridging back"). + /// @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(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; From 1235ccfddd8f52602b745e3b2f435b5ce90a81ca Mon Sep 17 00:00:00 2001 From: Daniel Wang Date: Tue, 23 Apr 2024 11:46:49 +0800 Subject: [PATCH 11/11] Update BridgedERC20Base.sol --- packages/protocol/contracts/tokenvault/BridgedERC20Base.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol b/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol index e3a9fb82d37..b5bccfbf293 100644 --- a/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol +++ b/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol @@ -82,7 +82,7 @@ abstract contract BridgedERC20Base is EssentialContract, IBridgedERC20 { IBridgedERC20(migratingAddress).mint(msg.sender, _amount); } else if (msg.sender != resolve(LibStrings.B_ERC20_VAULT, true)) { // When user wants to burn tokens only during 'migrating out' phase is possible. If - // ERC20Vault burns the tokens, that will go thru the burn(amount) function. + // ERC20Vault burns the tokens, that will go through the burn(amount) function. revert RESOLVER_DENIED(); }