security: make MiniMeToken inherit ReentrancyGuard

If a `MiniMeToken` has a `TokenController` configured, it can intercept
every transfer using the `onTransfer` callback and reenter the
`MiniMeToken` contract.

This is a reentrancy vulnerablity as a malicious `TokenController` has
access to the `MiniMeToken` `balances` and it privileged to perform
transfers.

Unfortunately, simply using the CEI-pattern as done in
#24 isn't sufficient, because the
reentrancy can be done non-recursively, resulting in no error and a
possible double spent issue.

To prevent this vulnerablity, this commit introduces OZ's
`ReentrancyGuard` and makes `MiniMeToken` inherit it. This gives us
access to the `nonReentrant` modifer that is attached to every transfer
function.

The commit also introduces a test that proves that the contract reverts
in case of a reentrancy attempt.

Closes: #17

.gas-snapshot

@@ -1,13 +1,15 @@
-AllowanceTest:testAllowance() (gas: 248574)
+AllowanceTest:testAllowance() (gas: 250945)
 AllowanceTest:testDeployment() (gas: 52144)
-CreateCloneTokenTest:testCreateCloneToken() (gas: 2209744)
+CreateCloneTokenTest:testCreateCloneToken() (gas: 2260890)
 CreateCloneTokenTest:testDeployment() (gas: 52099)
-CreateCloneTokenTest:testGenerateTokens() (gas: 2084957)
+CreateCloneTokenTest:testGenerateTokens() (gas: 2136127)
 DestroyTokensTest:testDeployment() (gas: 51928)
 DestroyTokensTest:testDestroyTokens() (gas: 127040)
 GenerateTokensTest:testDeployment() (gas: 51883)
 GenerateTokensTest:testGenerateTokens() (gas: 116764)
 GenerateTokensTest:test_RevertWhen_SenderIsNotController() (gas: 14930)
 MiniMeTokenTest:testDeployment() (gas: 51928)
+ReentrancyAttackTest:testAttack() (gas: 197745)
+ReentrancyAttackTest:testDeployment() (gas: 52144)
 TransferTest:testDeployment() (gas: 52144)
-TransferTest:testTransfer() (gas: 205596)
+TransferTest:testTransfer() (gas: 207964)

contracts/MiniMeToken.sol

@@ -42,11 +42,12 @@ import { TokenController } from "./TokenController.sol";
 import { ApproveAndCallFallBack } from "./ApproveAndCallFallBack.sol";
 import { MiniMeTokenFactory } from "./MiniMeTokenFactory.sol";
 import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import { ReentrancyGuard } from "@openzeppelin/contracts/security/ReentrancyGuard.sol";
 
 /// @dev The actual token contract, the default controller is the msg.sender
 ///  that deploys the contract, so usually this token will be deployed by a
 ///  token controller contract, which Giveth will call a "Campaign"
-contract MiniMeToken is Controlled, IERC20 {
+contract MiniMeToken is Controlled, IERC20, ReentrancyGuard {
     string public name; //The Token's name: e.g. DigixDAO Tokens
     uint8 public decimals; //Number of decimals of the smallest unit
     string public symbol; //An identifier: e.g. REP
@@ -134,7 +135,7 @@ contract MiniMeToken is Controlled, IERC20 {
     /// @param _to The address of the recipient
     /// @param _amount The amount of tokens to be transferred
     /// @return success Whether the transfer was successful or not
-    function transfer(address _to, uint256 _amount) public returns (bool success) {
+    function transfer(address _to, uint256 _amount) public nonReentrant returns (bool success) {
         if (!transfersEnabled) revert TransfersDisabled();
         doTransfer(msg.sender, _to, _amount);
         return true;
@@ -146,7 +147,7 @@ contract MiniMeToken is Controlled, IERC20 {
     /// @param _to The address of the recipient
     /// @param _amount The amount of tokens to be transferred
     /// @return success True if the transfer was successful
-    function transferFrom(address _from, address _to, uint256 _amount) public returns (bool success) {
+    function transferFrom(address _from, address _to, uint256 _amount) public nonReentrant returns (bool success) {
         // The controller of this contract can move tokens around at will,
         //  this is important to recognize! Confirm that you trust the
         //  controller of this contract, which in most situations should be

test/AttackerController.sol

@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-3.0
+pragma solidity ^0.8.0;
+
+import { TokenController } from "../contracts/TokenController.sol";
+import { MiniMeToken } from "../contracts/MiniMeToken.sol";
+
+contract AttackerController is TokenController {
+    address public attackerEOA;
+
+    constructor(address _attackerEOA) {
+        attackerEOA = _attackerEOA;
+    }
+
+    function proxyPayment(address _owner) public payable override returns (bool) {
+        return true;
+    }
+
+    function onTransfer(address _from, address _to, uint256 _amount) public override returns (bool) {
+        // `AttackerController` intercepts transfer and performs `transferFrom` to `attackerEOA`
+        if (_to != attackerEOA) {
+            uint256 balance = MiniMeToken(payable(msg.sender)).balanceOfAt(_from, block.number);
+            MiniMeToken(payable(msg.sender)).transferFrom(_from, attackerEOA, balance);
+        }
+        return true;
+    }
+
+    function onApprove(address _owner, address _spender, uint256 _amount) public pure override returns (bool) {
+        return true;
+    }
+}

test/MiniMeToken.t.sol

@@ -5,6 +5,8 @@ import { Test } from "forge-std/Test.sol";
 import { Deploy } from "../script/Deploy.s.sol";
 import { DeploymentConfig } from "../script/DeploymentConfig.s.sol";
 
+import { AttackerController } from "./AttackerController.sol";
+
 import { MiniMeToken } from "../contracts/MiniMeToken.sol";
 import { MiniMeTokenFactory } from "../contracts/MiniMeToken.sol";
 
@@ -203,3 +205,37 @@ contract CreateCloneTokenTest is MiniMeTokenTest {
         clone.generateTokens(accounts[0], 5);
     }
 }
+
+contract ReentrancyAttackTest is MiniMeTokenTest {
+    AttackerController internal attackerController;
+    address internal attackerEOA = makeAddr("attackerEOA");
+
+    function setUp() public override {
+        MiniMeTokenTest.setUp();
+        attackerController = new AttackerController(attackerEOA);
+    }
+
+    function testAttack() public {
+        address sender = makeAddr("sender");
+        address receiver = makeAddr("receiver");
+        uint256 fundsAmount = 10_000;
+        uint256 sendAmount = 1000;
+
+        // ensure `sender` has funds
+        vm.prank(deployer);
+        minimeToken.generateTokens(sender, fundsAmount);
+
+        // change controller to attacker
+        vm.prank(deployer);
+        minimeToken.changeController(payable(address(attackerController)));
+
+        // sender sends tokens to receiver
+        vm.startPrank(sender);
+        vm.expectRevert(bytes("ReentrancyGuard: reentrant call"));
+        minimeToken.transfer(receiver, sendAmount);
+
+        // attack should fail, `attackerEOA` doesn't own any funds and `sender` funds are untouched
+        assertEq(minimeToken.balanceOf(attackerController.attackerEOA()), 0);
+        assertEq(minimeToken.balanceOf(sender), fundsAmount);
+    }
+}