Skip to content

Commit

Permalink
Change to transfer and burn pattern
Browse files Browse the repository at this point in the history
  • Loading branch information
Keszey Dániel authored and Keszey Dániel committed May 8, 2024
1 parent fe49ed8 commit f59a182
Show file tree
Hide file tree
Showing 6 changed files with 190 additions and 13 deletions.
10 changes: 7 additions & 3 deletions packages/protocol/contracts/tokenvault/BridgedERC1155.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
)
Expand All @@ -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.
Expand Down
8 changes: 2 additions & 6 deletions packages/protocol/contracts/tokenvault/BridgedERC721.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
5 changes: 4 additions & 1 deletion packages/protocol/contracts/tokenvault/ERC1155Vault.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
5 changes: 4 additions & 1 deletion packages/protocol/contracts/tokenvault/ERC721Vault.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
91 changes: 90 additions & 1 deletion packages/protocol/test/tokenvault/ERC1155Vault.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
}
}
84 changes: 83 additions & 1 deletion packages/protocol/test/tokenvault/ERC721Vault.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
}
}

0 comments on commit f59a182

Please sign in to comment.