evm: don't use burnFrom as it's not supported by some tokens (#237)

Author: kcsongor

evm/src/NttManager/NttManager.sol

@@ -259,27 +259,20 @@ contract NttManager is INttManager, NttManagerState {
 
         {
             // Lock/burn tokens before checking rate limits
-            if (mode == Mode.LOCKING) {
-                {
-                    // use transferFrom to pull tokens from the user and lock them
-                    // query own token balance before transfer
-                    uint256 balanceBefore = _getTokenBalanceOf(token, address(this));
+            // use transferFrom to pull tokens from the user and lock them
+            // query own token balance before transfer
+            uint256 balanceBefore = _getTokenBalanceOf(token, address(this));
 
-                    // transfer tokens
-                    IERC20(token).safeTransferFrom(msg.sender, address(this), amount);
+            // transfer tokens
+            IERC20(token).safeTransferFrom(msg.sender, address(this), amount);
 
-                    // query own token balance after transfer
-                    uint256 balanceAfter = _getTokenBalanceOf(token, address(this));
+            // query own token balance after transfer
+            uint256 balanceAfter = _getTokenBalanceOf(token, address(this));
 
-                    // correct amount for potential transfer fees
-                    amount = balanceAfter - balanceBefore;
-                }
-            } else if (mode == Mode.BURNING) {
+            // correct amount for potential transfer fees
+            amount = balanceAfter - balanceBefore;
+            if (mode == Mode.BURNING) {
                 {
-                    // query sender's token balance before burn
-                    uint256 balanceBefore = _getTokenBalanceOf(token, msg.sender);
-
-                    // call the token's burn function to burn the sender's token
                     // NOTE: We don't account for burn fees in this code path.
                     // We verify that the user's change in balance is equal to the amount that's burned.
                     // Accounting for burn fees can be non-trivial, since there
@@ -291,18 +284,14 @@ contract NttManager is INttManager, NttManagerState {
                     // Since there is no standard way to query for burn fee amounts with burnable tokens,
                     // and NTT would be used on a per-token basis, implementing this functionality
                     // is left to integrating projects who may need to account for burn fees on their tokens.
-                    ERC20Burnable(token).burnFrom(msg.sender, amount);
-
-                    // query sender's token balance after transfer
-                    uint256 balanceAfter = _getTokenBalanceOf(token, msg.sender);
+                    ERC20Burnable(token).burn(amount);
 
-                    uint256 balanceDiff = balanceBefore - balanceAfter;
-                    if (balanceDiff != amount) {
-                        revert BurnAmountDifferentThanBalanceDiff(amount, balanceDiff);
+                    // tokens held by the contract after the operation should be the same as before
+                    uint256 balanceAfterBurn = _getTokenBalanceOf(token, address(this));
+                    if (balanceBefore != balanceAfterBurn) {
+                        revert BurnAmountDifferentThanBalanceDiff(balanceBefore, balanceAfterBurn);
                     }
                 }
-            } else {
-                revert InvalidMode(uint8(mode));
             }
         }
 

evm/test/mocks/DummyToken.sol

@@ -18,18 +18,22 @@ contract DummyToken is ERC20 {
     }
 
     function burnFrom(address, uint256) public virtual {
-        revert("Locking nttManager should not call 'burnFrom()'");
+        revert("No nttManager should call 'burnFrom()'");
+    }
+
+    function burn(address, uint256) public virtual {
+        revert("Locking nttManager should not call 'burn()'");
     }
 }
 
 contract DummyTokenMintAndBurn is DummyToken {
     function mint(address to, uint256 amount) public override {
-        // TODO - add access control here
+        // TODO - add access control here?
         _mint(to, amount);
     }
 
-    function burnFrom(address to, uint256 amount) public override {
-        // TODO - add access control here
-        _burn(to, amount);
+    function burn(uint256 amount) public {
+        // TODO - add access control here?
+        _burn(msg.sender, amount);
     }
 }