Check depositor when burning ERC721 tokens (#687)

* burn fix * Release v2.1.4 * Fix tests --------- Co-authored-by: Kirill Pisarev <[email protected]>
ChainSafe · Feb 3, 2023 · b929c98 · b929c98
1 parent 2f29dd7
commit b929c98
Show file tree

Hide file tree

Showing 4 changed files with 15 additions and 5 deletions.
diff --git a/contracts/ERC721Safe.sol b/contracts/ERC721Safe.sol
@@ -55,8 +55,9 @@ contract ERC721Safe {
         @param tokenAddress Address of ERC721 to burn.
         @param tokenID ID of token to burn.
      */
-    function burnERC721(address tokenAddress, uint256 tokenID) internal {
+    function burnERC721(address tokenAddress, address owner, uint256 tokenID) internal {
         ERC721MinterBurnerPauser erc721 = ERC721MinterBurnerPauser(tokenAddress);
+        require(erc721.ownerOf(tokenID) == owner, 'Burn not from owner');
         erc721.burn(tokenID);
     }
 

diff --git a/contracts/handlers/ERC721Handler.sol b/contracts/handlers/ERC721Handler.sol
@@ -58,7 +58,7 @@ contract ERC721Handler is IDepositExecute, HandlerHelpers, ERC721Safe {
         }
 
         if (_burnList[tokenAddress]) {
-            burnERC721(tokenAddress, tokenID);
+            burnERC721(tokenAddress, depositer, tokenID);
         } else {
             lockERC721(tokenAddress, depositer, address(this), tokenID);
         }

diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@chainsafe/chainbridge-contracts",
-  "version": "2.1.3",
+  "version": "2.1.4",
   "description": "",
   "main": "dist/index.js",
   "repository": "https://github.com/ChainSafe/chainbridge-solidity.git",

diff --git a/test/handlers/erc721/depositBurn.js b/test/handlers/erc721/depositBurn.js
@@ -2,7 +2,7 @@
  * Copyright 2020 ChainSafe Systems
  * SPDX-License-Identifier: LGPL-3.0-only
  */
- 
+
  const TruffleAssert = require('truffle-assertions');
 
  const Helpers = require('../../helpers');
@@ -48,7 +48,7 @@ contract('ERC721Handler - [Deposit Burn ERC721]', async (accounts) => {
             ERC721HandlerContract.new(BridgeInstance.address).then(instance => ERC721HandlerInstance = instance),
             ERC721MintableInstance1.mint(depositerAddress, tokenID, "")
         ]);
-            
+
         await Promise.all([
             ERC721MintableInstance1.approve(ERC721HandlerInstance.address, tokenID, { from: depositerAddress }),
             BridgeInstance.adminSetResource(ERC721HandlerInstance.address, resourceID1, ERC721MintableInstance1.address),
@@ -89,4 +89,13 @@ contract('ERC721Handler - [Deposit Burn ERC721]', async (accounts) => {
             ERC721MintableInstance1.ownerOf(tokenID),
             'ERC721: owner query for nonexistent token');
     });
+    it('depositAmount of ERC721MintableInstance1 tokens should NOT burn from NOT token owner', async () => {
+        await TruffleAssert.reverts(BridgeInstance.deposit(
+                domainID,
+                resourceID1,
+                depositData,
+                { from: accounts[3] }
+            ),
+            'Burn not from owner');
+    });
 });