fix(protocol): change to transfer and mint pattern with BridgedERC20 …

…tokens (#16796)

Co-authored-by: Daniel Wang <[email protected]>
Co-authored-by: Keszey Dániel <[email protected]>
Co-authored-by: Daniel Wang <[email protected]>

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

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;

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

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;

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