feat(protocol): change to transfer-and-burn pattern with NFT vaults

packages/protocol/contracts/tokenvault/BridgedERC1155.sol

@@ -79,11 +79,9 @@ contract BridgedERC1155 is EssentialContract, ERC1155Upgradeable {
     }
 
     /// @dev Batch burns tokens.
-    /// @param _account Address from which tokens are burned.
     /// @param _ids Array of IDs of the tokens to burn.
     /// @param _amounts Amount of tokens to burn respectively.
     function burnBatch(
-        address _account,
         uint256[] calldata _ids,
         uint256[] calldata _amounts
     )
@@ -92,7 +90,7 @@ contract BridgedERC1155 is EssentialContract, ERC1155Upgradeable {
         onlyFromNamed(LibStrings.B_ERC1155_VAULT)
         nonReentrant
     {
-        _burnBatch(_account, _ids, _amounts);
+        _burnBatch(msg.sender, _ids, _amounts);
     }
 
     /// @notice Gets the canonical token's address and chain ID.

packages/protocol/contracts/tokenvault/BridgedERC721.sol

@@ -65,19 +65,15 @@ contract BridgedERC721 is EssentialContract, ERC721Upgradeable {
     }
 
     /// @dev Burns tokens.
-    /// @param _account Address from which the token is burned.
     /// @param _tokenId ID of the token to burn.
-    function burn(
-        address _account,
-        uint256 _tokenId
-    )
+    function burn(uint256 _tokenId)
         external
         whenNotPaused
         onlyFromNamed(LibStrings.B_ERC721_VAULT)
         nonReentrant
     {
         // Check if the caller is the owner of the token.
-        if (ownerOf(_tokenId) != _account) {
+        if (ownerOf(_tokenId) != msg.sender) {
             revert BTOKEN_INVALID_BURN();
         }
         _burn(_tokenId);

packages/protocol/contracts/tokenvault/ERC1155Vault.sol

@@ -238,7 +238,10 @@ contract ERC1155Vault is BaseNFTVault, ERC1155ReceiverUpgradeable {
             CanonicalNFT storage _ctoken = bridgedToCanonical[_op.token];
             if (_ctoken.addr != address(0)) {
                 ctoken_ = _ctoken;
-                BridgedERC1155(_op.token).burnBatch(msg.sender, _op.tokenIds, _op.amounts);
+                BridgedERC1155(_op.token).safeBatchTransferFrom(
+                    msg.sender, address(this), _op.tokenIds, _op.amounts, ""
+                );
+                BridgedERC1155(_op.token).burnBatch(_op.tokenIds, _op.amounts);
             } else {
                 // is a ctoken token, meaning, it lives on this chain
                 ctoken_ = CanonicalNFT({

packages/protocol/contracts/tokenvault/ERC721Vault.sol

@@ -197,7 +197,10 @@ contract ERC721Vault is BaseNFTVault, IERC721Receiver {
             if (_ctoken.addr != address(0)) {
                 ctoken_ = _ctoken;
                 for (uint256 i; i < _op.tokenIds.length; ++i) {
-                    BridgedERC721(_op.token).burn(msg.sender, _op.tokenIds[i]);
+                    BridgedERC721(_op.token).safeTransferFrom(
+                        msg.sender, address(this), _op.tokenIds[i]
+                    );
+                    BridgedERC721(_op.token).burn(_op.tokenIds[i]);
                 }
             } else {
                 ctoken_ = CanonicalNFT({

packages/protocol/test/tokenvault/ERC1155Vault.t.sol

@@ -824,7 +824,7 @@ contract ERC1155VaultTest is TaikoTest {
         );
 
         vm.prank(Alice, Alice);
-        vm.expectRevert("ERC1155: burn amount exceeds balance");
+        vm.expectRevert("ERC1155: caller is not token owner or approved");
         destChainErc1155Vault.sendToken{ value: GAS_LIMIT }(sendOpts);
     }
 
@@ -898,4 +898,93 @@ contract ERC1155VaultTest is TaikoTest {
             fail();
         }
     }
+
+    function test_1155Vault_shall_not_be_able_to_burn_arbitrarily() public {
+        vm.prank(Alice, Alice);
+        ctoken1155.setApprovalForAll(address(erc1155Vault), true);
+
+        assertEq(ctoken1155.balanceOf(Alice, 1), 10);
+        assertEq(ctoken1155.balanceOf(address(erc1155Vault), 1), 0);
+
+        uint256[] memory tokenIds = new uint256[](1);
+        tokenIds[0] = 1;
+
+        uint256[] memory amounts = new uint256[](1);
+        amounts[0] = 1;
+
+        BaseNFTVault.BridgeTransferOp memory sendOpts = BaseNFTVault.BridgeTransferOp(
+            destChainId,
+            address(0),
+            Alice,
+            GAS_LIMIT,
+            address(ctoken1155),
+            GAS_LIMIT,
+            tokenIds,
+            amounts
+        );
+        vm.prank(Alice, Alice);
+        erc1155Vault.sendToken{ value: GAS_LIMIT }(sendOpts);
+
+        assertEq(ctoken1155.balanceOf(address(erc1155Vault), 1), 1);
+
+        // This canonicalToken is basically need to be exact same as the
+        // sendToken() puts together
+        // - here is just mocking putting it together.
+        BaseNFTVault.CanonicalNFT memory canonicalToken = BaseNFTVault.CanonicalNFT({
+            chainId: 31_337,
+            addr: address(ctoken1155),
+            symbol: "TT",
+            name: "TT"
+        });
+
+        uint64 chainId = uint64(block.chainid);
+        vm.chainId(destChainId);
+
+        destChainIdBridge.sendReceiveERC1155ToERC1155Vault(
+            canonicalToken,
+            Alice,
+            Alice,
+            tokenIds,
+            amounts,
+            bytes32(0),
+            address(erc1155Vault),
+            chainId,
+            0
+        );
+
+        // Query canonicalToBridged
+        address deployedContract =
+            destChainErc1155Vault.canonicalToBridged(chainId, address(ctoken1155));
+        // Alice bridged over 1 from tokenId 1
+        assertEq(ERC1155(deployedContract).balanceOf(Alice, 1), 1);
+
+        sendOpts = BaseNFTVault.BridgeTransferOp(
+            chainId,
+            address(0),
+            Alice,
+            GAS_LIMIT,
+            address(deployedContract),
+            GAS_LIMIT,
+            tokenIds,
+            amounts
+        );
+
+        // Alice hasn't approved the vault yet!
+        vm.prank(Alice, Alice);
+        vm.expectRevert("ERC1155: caller is not token owner or approved");
+        destChainErc1155Vault.sendToken{ value: GAS_LIMIT }(sendOpts);
+
+        // Also Vault cannot burn tokens it does not own (even if the priv key compromised)
+        uint256[] memory randomIdAndLength = new uint256[](1);
+        randomIdAndLength[0] = 20;
+        vm.prank(address(destChainErc1155Vault), address(destChainErc1155Vault));
+        vm.expectRevert("ERC1155: burn amount exceeds balance");
+        BridgedERC1155(deployedContract).burnBatch(randomIdAndLength, randomIdAndLength);
+
+        // After setApprovalForAll() ERC1155Vault can transfer and burn
+        vm.prank(Alice, Alice);
+        ERC1155(deployedContract).setApprovalForAll(address(destChainErc1155Vault), true);
+        vm.prank(Alice, Alice);
+        destChainErc1155Vault.sendToken{ value: GAS_LIMIT }(sendOpts);
+    }
 }

packages/protocol/test/tokenvault/ERC721Vault.t.sol

@@ -761,7 +761,7 @@ contract ERC721VaultTest is TaikoTest {
         );
 
         vm.prank(Alice, Alice);
-        vm.expectRevert(BridgedERC721.BTOKEN_INVALID_BURN.selector);
+        vm.expectRevert("ERC721: transfer from incorrect owner");
         destChainErc721Vault.sendToken{ value: GAS_LIMIT }(sendOpts);
     }
 
@@ -833,4 +833,86 @@ contract ERC721VaultTest is TaikoTest {
             fail();
         }
     }
+
+    function test_721Vault_shall_not_be_able_to_burn_arbitrarily() public {
+        vm.prank(Alice, Alice);
+        canonicalToken721.approve(address(erc721Vault), 1);
+        vm.prank(Alice, Alice);
+        canonicalToken721.approve(address(erc721Vault), 2);
+
+        assertEq(canonicalToken721.ownerOf(1), Alice);
+
+        uint256[] memory tokenIds = new uint256[](1);
+        tokenIds[0] = 1;
+
+        uint256[] memory amounts = new uint256[](1);
+        amounts[0] = 0;
+
+        BaseNFTVault.BridgeTransferOp memory sendOpts = BaseNFTVault.BridgeTransferOp(
+            destChainId,
+            address(0),
+            Alice,
+            GAS_LIMIT,
+            address(canonicalToken721),
+            GAS_LIMIT,
+            tokenIds,
+            amounts
+        );
+        vm.prank(Alice, Alice);
+        erc721Vault.sendToken{ value: GAS_LIMIT }(sendOpts);
+
+        assertEq(canonicalToken721.ownerOf(1), address(erc721Vault));
+
+        // This canonicalToken is basically need to be exact same as the
+        // sendToken() puts together
+        // - here is just mocking putting it together.
+        BaseNFTVault.CanonicalNFT memory canonicalToken = BaseNFTVault.CanonicalNFT({
+            chainId: 31_337,
+            addr: address(canonicalToken721),
+            symbol: "TT",
+            name: "TT"
+        });
+
+        uint64 chainId = uint64(block.chainid);
+        vm.chainId(destChainId);
+
+        destChainIdBridge.sendReceiveERC721ToERC721Vault(
+            canonicalToken, Alice, Alice, tokenIds, bytes32(0), address(erc721Vault), chainId, 0
+        );
+
+        // Query canonicalToBridged
+        address deployedContract =
+            destChainErc721Vault.canonicalToBridged(chainId, address(canonicalToken721));
+
+        // Alice bridged over tokenId 1
+        assertEq(ERC721(deployedContract).ownerOf(1), Alice);
+
+        // Alice tries to bridge back message
+        sendOpts = BaseNFTVault.BridgeTransferOp(
+            chainId,
+            address(0),
+            Alice,
+            GAS_LIMIT,
+            address(deployedContract),
+            GAS_LIMIT,
+            tokenIds,
+            amounts
+        );
+
+        // Alice hasn't approved the vault yet!
+        vm.prank(Alice, Alice);
+        vm.expectRevert("ERC721: caller is not token owner or approved");
+        destChainErc721Vault.sendToken{ value: GAS_LIMIT }(sendOpts);
+
+        // Also Vault cannot burn tokens it does not own (even if the priv key compromised)
+        vm.prank(address(destChainErc721Vault), address(destChainErc721Vault));
+        vm.expectRevert(BridgedERC721.BTOKEN_INVALID_BURN.selector);
+        BridgedERC721(deployedContract).burn(1);
+
+        // After approve() ERC721Vault can transfer and burn
+        vm.prank(Alice, Alice);
+        ERC721(deployedContract).approve(address(destChainErc721Vault), 1);
+        vm.prank(Alice, Alice);
+        destChainErc721Vault.sendToken{ value: GAS_LIMIT }(sendOpts);
+    }
 }