Merge pull request #4 from streamr-dev/audit2-fixes

explicit tests for transferAndCall

Author: jtakalai

README.md

@@ -25,6 +25,17 @@ Some of the following features are inherited from [OpenZeppelin contracts](https
 | Minting           | mint                  | contracts/DATAv2.sol                                              |
 | Burning           | burn, burnFrom        | @openzeppelin/contracts/token/ERC20/extensions/ERC20Burnable.sol  |
 
+## Minting
+
+DATAv2 has an admin role (controlled by Streamr) that can add and remove minters, who can mint tokens tokens at will. The admin and minters will be contracts that are not controlled by a single wallet.
+
+The whole old DATA supply is minted as part of migration, and new tokens may be minted later as decided by the community process in the form of [Streamr Improvement Proposals](https://snapshot.org/#/streamr.eth), see e.g. [SIP-6](https://snapshot.org/#/streamr.eth/proposal/QmU383LMPAHdzMevcxY6UzyL5vkBaNHQhCcp2WbXw5kXS1).
+
+# ERC677 functionality
+
+DATAv2 follows the convention of [LINK](https://etherscan.io/address/0x514910771af9ca656af840dff83e8264ecf986ca#code) and other tokens with regard to the callback function `onTokenTransfer` invoked by `transferAndCall`: DATAv2 does not check the bool return value of `onTokenTransfer`. Instead `onTokenTransfer` should revert if the transfer is to be rejected by the recipient contract. If the recipient is a contract, it must implement `onTokenTransfer` or else `transferAndCall` reverts.
+
+
 ## Migration process
 
 1. We deploy the DATAv2 and DataTokenMigrator contracts

contracts/MockRecipient.sol

@@ -1,4 +1,4 @@
-//SPDX-License-Identifier: Unlicense
+// SPDX-License-Identifier: MIT
 pragma solidity 0.8.6;
 
 import "hardhat/console.sol";
@@ -16,5 +16,6 @@ contract MockRecipient is IERC677Receiver {
         console.log("Amount", _value);
         console.log("With data", string(_data));
         txCount += 1;
+        require(keccak256(_data) != keccak256("err")); // for testing: revert if passed "err"
     }
 }

contracts/MockRecipientNotERC677Receiver.sol

@@ -0,0 +1,7 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.6;
+
+/** Just a blank contract that doesnt implement onTokensTransferred */
+contract MockRecipientNotERC677Receiver {
+    uint public x = 1;
+}

contracts/MockRecipientReturnBool.sol

@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.6;
+
+import "hardhat/console.sol";
+
+/**
+ * ERC677 recipient that returns false for error instead of reverting
+ * The return value gets ignored, and transfer proceeds succesfully. This is by design.
+ */
+contract MockRecipientReturnBool {
+    uint public txCount;
+
+    function onTokenTransfer(
+        address _sender,
+        uint256 _value,
+        bytes calldata _data
+    ) external returns (bool) {
+        console.log("Received from", _sender);
+        console.log("Amount", _value);
+        console.log("With data", string(_data));
+        txCount += 1;
+        bool retval = keccak256(_data) != keccak256("err"); // for testing: return false if passed "err"
+        console.log("retval", retval);
+        return retval;
+    }
+}

package-lock.json

@@ -3,6 +3,9 @@ const { id } = require("@ethersproject/hash")
 const { expect } = require("chai")
 const { ethers } = require("hardhat")
 
+// "err" as bytes, induces a simulated error in MockRecipient.sol and MockRecipientReturnBool.sol
+const errData = "0x657272"
+
 describe("DATAv2", () => {
     it("transferAndCall triggers ERC677 callback", async () => {
         const [signer, minter] = await ethers.getSigners()
@@ -11,18 +14,40 @@ describe("DATAv2", () => {
         const recipient = await MockRecipient.deploy()
         await recipient.deployed()
 
+        const MockRecipientNotERC677Receiver = await ethers.getContractFactory("MockRecipientNotERC677Receiver")
+        const nonReceiverRecipient = await MockRecipientNotERC677Receiver.deploy()
+        await nonReceiverRecipient.deployed()
+
+        const MockRecipientReturnBool = await ethers.getContractFactory("MockRecipientReturnBool")
+        const returnBoolRecipient = await MockRecipientReturnBool.deploy()
+        await returnBoolRecipient.deployed()
+
         const DATAv2 = await ethers.getContractFactory("DATAv2")
         const token = await DATAv2.deploy()
         await token.deployed()
 
         await expect(token.grantRole(id("MINTER_ROLE"), minter.address)).to.emit(token, "RoleGranted")
-        await expect(token.connect(minter).mint(signer.address, parseEther("1"))).to.emit(token, "Transfer(address,address,uint256)")
+        await expect(token.connect(minter).mint(signer.address, parseEther("10"))).to.emit(token, "Transfer(address,address,uint256)")
 
-        const before = await recipient.txCount()
+        // revert in callback => should revert transferAndCall
+        await expect(token.transferAndCall(recipient.address, parseEther("1"), errData)).to.be.reverted
+
+        // no callback => should revert transferAndCall
+        await expect(token.transferAndCall(nonReceiverRecipient.address, parseEther("1"), "0x")).to.be.reverted
+
+        // contract that implements ERC677Receiver executes the callback
+        const txsBefore = await recipient.txCount()
         await token.transferAndCall(recipient.address, parseEther("1"), "0x6c6f6c")
-        const after = await recipient.txCount()
+        const txsAfter = await recipient.txCount()
+
+        // callback returns true or false but doesn't revert => should NOT revert
+        const txsBeforeBool = await returnBoolRecipient.txCount()
+        await token.transferAndCall(returnBoolRecipient.address, parseEther("1"), errData)
+        await token.transferAndCall(returnBoolRecipient.address, parseEther("1"), "0x")
+        const txsAfterBool = await returnBoolRecipient.txCount()
 
-        expect(after).to.equal(before.add(1))
+        expect(txsAfter).to.equal(txsBefore.add(1))
+        expect(txsAfterBool).to.equal(txsBeforeBool.add(2))
     })
 
     it("transferAndCall just does normal transfer for non-contract accounts", async () => {