From c241235f04f9e1429ad8c19f1378eb8ad0a3f5ea Mon Sep 17 00:00:00 2001
From: r4bbit <445106+0x-r4bbit@users.noreply.github.com>
Date: Wed, 20 Sep 2023 09:36:09 +0200
Subject: [PATCH] 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
https://github.com/vacp2p/minime/pull/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
---
 contracts/MiniMeToken.sol   |  7 ++++---
 test/AttackerController.sol | 30 ++++++++++++++++++++++++++++++
 test/MiniMeToken.t.sol      | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 3 deletions(-)
 create mode 100644 test/AttackerController.sol

diff --git a/contracts/MiniMeToken.sol b/contracts/MiniMeToken.sol
index e340eda..062ddb8 100644
--- a/contracts/MiniMeToken.sol
+++ b/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
diff --git a/test/AttackerController.sol b/test/AttackerController.sol
new file mode 100644
index 0000000..61e840e
--- /dev/null
+++ b/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;
+    }
+}
diff --git a/test/MiniMeToken.t.sol b/test/MiniMeToken.t.sol
index d0cc5c6..9bdeb57 100644
--- a/test/MiniMeToken.t.sol
+++ b/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);
+    }
+}