From f59a182dbf3851c05451ac951e81c153ee036913 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Keszey=20D=C3=A1niel?= Date: Wed, 8 May 2024 16:10:23 +0200 Subject: [PATCH] Change to transfer and burn pattern --- .../contracts/tokenvault/BridgedERC1155.sol | 10 +- .../contracts/tokenvault/BridgedERC721.sol | 8 +- .../contracts/tokenvault/ERC1155Vault.sol | 5 +- .../contracts/tokenvault/ERC721Vault.sol | 5 +- .../test/tokenvault/ERC1155Vault.t.sol | 91 ++++++++++++++++++- .../test/tokenvault/ERC721Vault.t.sol | 84 ++++++++++++++++- 6 files changed, 190 insertions(+), 13 deletions(-) diff --git a/packages/protocol/contracts/tokenvault/BridgedERC1155.sol b/packages/protocol/contracts/tokenvault/BridgedERC1155.sol index 62b19080b8..a5e4d0aeee 100644 --- a/packages/protocol/contracts/tokenvault/BridgedERC1155.sol +++ b/packages/protocol/contracts/tokenvault/BridgedERC1155.sol @@ -26,6 +26,7 @@ contract BridgedERC1155 is EssentialContract, ERC1155Upgradeable { error BTOKEN_INVALID_PARAMS(); error BTOKEN_INVALID_TO_ADDR(); + error BTOKEN_INVALID_BURN(); /// @notice Initializes the contract. /// @param _owner The owner of this contract. msg.sender will be used if this value is zero. @@ -79,11 +80,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 +91,12 @@ contract BridgedERC1155 is EssentialContract, ERC1155Upgradeable { onlyFromNamed(LibStrings.B_ERC1155_VAULT) nonReentrant { - _burnBatch(_account, _ids, _amounts); + for (uint256 i; i < _ids.length; i++) { + if (balanceOf(msg.sender, _ids[i]) < _amounts[i]) { + revert BTOKEN_INVALID_BURN(); + } + } + _burnBatch(msg.sender, _ids, _amounts); } /// @notice Gets the canonical token's address and chain ID. diff --git a/packages/protocol/contracts/tokenvault/BridgedERC721.sol b/packages/protocol/contracts/tokenvault/BridgedERC721.sol index afca5a3209..4aaf0f14ed 100644 --- a/packages/protocol/contracts/tokenvault/BridgedERC721.sol +++ b/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); diff --git a/packages/protocol/contracts/tokenvault/ERC1155Vault.sol b/packages/protocol/contracts/tokenvault/ERC1155Vault.sol index 4a264c54a2..1443c4f8b0 100644 --- a/packages/protocol/contracts/tokenvault/ERC1155Vault.sol +++ b/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({ diff --git a/packages/protocol/contracts/tokenvault/ERC721Vault.sol b/packages/protocol/contracts/tokenvault/ERC721Vault.sol index f5673b6801..4fa1800b93 100644 --- a/packages/protocol/contracts/tokenvault/ERC721Vault.sol +++ b/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({ diff --git a/packages/protocol/test/tokenvault/ERC1155Vault.t.sol b/packages/protocol/test/tokenvault/ERC1155Vault.t.sol index 30309dd9b2..1beb318e13 100644 --- a/packages/protocol/test/tokenvault/ERC1155Vault.t.sol +++ b/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(BridgedERC1155.BTOKEN_INVALID_BURN.selector); + 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); + } } diff --git a/packages/protocol/test/tokenvault/ERC721Vault.t.sol b/packages/protocol/test/tokenvault/ERC721Vault.t.sol index fa5a4f19fd..c43ed5fde0 100644 --- a/packages/protocol/test/tokenvault/ERC721Vault.t.sol +++ b/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); + } }