From e715502da124801940a71427485dc4605b9be3e7 Mon Sep 17 00:00:00 2001 From: Ricardo Guilherme Schmidt <3esmit@gmail.com> Date: Fri, 22 Sep 2023 15:25:23 -0300 Subject: [PATCH] Check for reentrancy double spend on onTransfer (#29) * chore: add missing trailing slash in remapping This was not causing any compilation issues, but the solidity language server gets confused by this and complains about incorrect import statements otherwise. * TokenController interactions after balance updates This test demonstrates that all transfer methods are vulnerable to callback reentrancy attacks if the controller of the `MiniMeToken` is malicious. --------- Co-authored-by: r4bbit <445106+0x-r4bbit@users.noreply.github.com> --- .gas-report | 12 +++--- .gas-snapshot | 1 + contracts/MiniMeToken.sol | 14 +++---- remappings.txt | 2 +- test/reentrancy/AttackAccount.sol | 17 +++++++++ test/reentrancy/AttackController.sol | 37 +++++++++++++++++++ test/reentrancy/ReentrancyTest.t.sol | 55 ++++++++++++++++++++++++++++ 7 files changed, 124 insertions(+), 14 deletions(-) create mode 100644 test/reentrancy/AttackAccount.sol create mode 100644 test/reentrancy/AttackController.sol create mode 100644 test/reentrancy/ReentrancyTest.t.sol diff --git a/.gas-report b/.gas-report index 6791aba..e539892 100644 --- a/.gas-report +++ b/.gas-report @@ -3,16 +3,16 @@ | Deployment Cost | Deployment Size | | | | | | 1788057 | 9919 | | | | | | Function Name | min | avg | median | max | # calls | -| allowance | 808 | 808 | 808 | 808 | 2 | -| approve | 31708 | 31708 | 31708 | 31708 | 1 | -| balanceOf | 2361 | 3441 | 2361 | 5601 | 9 | +| allowance | 808 | 808 | 808 | 808 | 3 | +| approve | 30781 | 31244 | 31244 | 31708 | 2 | +| balanceOf | 2361 | 3138 | 2361 | 5601 | 13 | | balanceOfAt | 1142 | 2585 | 2363 | 3603 | 26 | -| changeController | 758 | 758 | 758 | 758 | 4 | +| changeController | 758 | 1318 | 758 | 3558 | 5 | | controller | 2447 | 2447 | 2447 | 2447 | 6 | | createCloneToken | 1832796 | 1832796 | 1832796 | 1832796 | 2 | | decimals | 294 | 294 | 294 | 294 | 6 | | destroyTokens | 8956 | 8956 | 8956 | 8956 | 1 | -| generateTokens | 2541 | 80795 | 94453 | 95751 | 10 | +| generateTokens | 2541 | 82036 | 94453 | 95751 | 11 | | name | 3253 | 3253 | 3253 | 3253 | 6 | | parentSnapShotBlock | 284 | 284 | 284 | 284 | 7 | | parentToken | 305 | 305 | 305 | 305 | 7 | @@ -20,7 +20,7 @@ | totalSupply | 1911 | 2917 | 1911 | 4930 | 6 | | totalSupplyAt | 1995 | 3029 | 3003 | 4606 | 7 | | transfer | 75187 | 75187 | 75187 | 75187 | 1 | -| transferFrom | 74194 | 74194 | 74194 | 74194 | 1 | +| transferFrom | 3495 | 48093 | 66590 | 74194 | 3 | | contracts/MiniMeTokenFactory.sol:MiniMeTokenFactory contract | | | | | | diff --git a/.gas-snapshot b/.gas-snapshot index 38eabab..f25e9ad 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -9,5 +9,6 @@ GenerateTokensTest:testDeployment() (gas: 45553) GenerateTokensTest:testGenerateTokens() (gas: 114564) GenerateTokensTest:test_RevertWhen_SenderIsNotController() (gas: 14930) MiniMeTokenTest:testDeployment() (gas: 45598) +ReentrancyTest:testAttack() (gas: 229331) TransferTest:testDeployment() (gas: 45814) TransferTest:testTransfer() (gas: 201218) \ No newline at end of file diff --git a/contracts/MiniMeToken.sol b/contracts/MiniMeToken.sol index 53093cc..b490654 100644 --- a/contracts/MiniMeToken.sol +++ b/contracts/MiniMeToken.sol @@ -184,13 +184,6 @@ contract MiniMeToken is Controlled, IERC20 { if (previousBalanceFrom < _amount) revert NotEnoughBalance(); - // Alerts the token controller of the transfer - if (isContract(controller)) { - if (!TokenController(controller).onTransfer(_from, _to, _amount)) { - revert ControllerRejected(); - } - } - // First update the balance array with the new value for the address // sending the tokens updateValueAtNow(balances[_from], previousBalanceFrom - _amount); @@ -201,6 +194,13 @@ contract MiniMeToken is Controlled, IERC20 { if (previousBalanceTo + _amount < previousBalanceTo) revert Overflow(); // Check for overflow updateValueAtNow(balances[_to], previousBalanceTo + _amount); + // Alerts the token controller of the transfer + if (isContract(controller)) { + if (!TokenController(controller).onTransfer(_from, _to, _amount)) { + revert ControllerRejected(); + } + } + // An event to make the transfer easy to find on the blockchain emit Transfer(_from, _to, _amount); } diff --git a/remappings.txt b/remappings.txt index 105f968..094529f 100644 --- a/remappings.txt +++ b/remappings.txt @@ -1,2 +1,2 @@ forge-std/=lib/forge-std/src/ -@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts \ No newline at end of file +@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/ diff --git a/test/reentrancy/AttackAccount.sol b/test/reentrancy/AttackAccount.sol new file mode 100644 index 0000000..12ff7bd --- /dev/null +++ b/test/reentrancy/AttackAccount.sol @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.0; + +import { MiniMeToken } from "../../contracts/MiniMeToken.sol"; + +contract AttackAccount { + address public owner = msg.sender; + MiniMeToken public token; + + constructor(MiniMeToken _token) { + token = _token; + } + + function attack(address _from, address _to, uint256 _amount) external { + token.transferFrom(_from, _to, _amount); + } +} diff --git a/test/reentrancy/AttackController.sol b/test/reentrancy/AttackController.sol new file mode 100644 index 0000000..9dbb811 --- /dev/null +++ b/test/reentrancy/AttackController.sol @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.0; + +import { TokenController } from "../../contracts/TokenController.sol"; +import { MiniMeToken } from "../../contracts/MiniMeToken.sol"; +import { AttackAccount } from "./AttackAccount.sol"; + +contract AttackController is TokenController { + address public attackerEOA; + AttackAccount public attackAccount; + + constructor(address _attackerEOA, AttackAccount _attackAccount) { + attackAccount = _attackAccount; + attackerEOA = _attackerEOA; + } + + function proxyPayment(address) public payable override returns (bool) { + return true; + } + + function onTransfer(address _from, address, uint256) public override returns (bool) { + uint256 allowance = MiniMeToken(payable(msg.sender)).allowance(_from, address(attackAccount)); + uint256 balance = MiniMeToken(payable(msg.sender)).balanceOf(_from); + if (allowance > 0) { + if (allowance > balance) { + attackAccount.attack(_from, attackerEOA, balance); + } else { + attackAccount.attack(_from, attackerEOA, allowance); + } + } + return true; + } + + function onApprove(address, address, uint256) public pure override returns (bool) { + return true; + } +} diff --git a/test/reentrancy/ReentrancyTest.t.sol b/test/reentrancy/ReentrancyTest.t.sol new file mode 100644 index 0000000..260e728 --- /dev/null +++ b/test/reentrancy/ReentrancyTest.t.sol @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.0; + +import { Test } from "forge-std/Test.sol"; +import { Deploy } from "../../script/Deploy.s.sol"; +import { DeploymentConfig } from "../../script/DeploymentConfig.s.sol"; +import { AttackController } from "./AttackController.sol"; +import { AttackAccount } from "./AttackAccount.sol"; +import { MiniMeToken } from "../../contracts/MiniMeToken.sol"; + +contract ReentrancyTest is Test { + AttackController internal attackController; + AttackAccount internal attackAccount; + MiniMeToken internal minimeToken; + DeploymentConfig internal deploymentConfig; + address internal deployer; + address internal attackerEOA = makeAddr("attackerEOA"); + + function setUp() public { + Deploy deployment = new Deploy(); + (deploymentConfig,, minimeToken) = deployment.run(); + (deployer,,,,,,) = deploymentConfig.activeNetworkConfig(); + + vm.prank(attackerEOA); + attackAccount = new AttackAccount(minimeToken); + attackController = new AttackController(attackerEOA, attackAccount); + } + + function testAttack() public { + address sender = makeAddr("sender"); + address receiver = address(attackAccount); + + uint256 fundsAmount = 10_000; + uint256 allowanceAmount = fundsAmount * 6; + uint256 sendAmount = fundsAmount; + + // ensure `sender` has funds + vm.prank(deployer); + minimeToken.generateTokens(sender, fundsAmount); + + // change controller to AttackController + vm.prank(deployer); + minimeToken.changeController(payable(address(attackController))); + + // sender sends tokens to receiver + vm.prank(sender); + minimeToken.approve(receiver, allowanceAmount); + + attackAccount.attack(sender, receiver, sendAmount); + + assertEq(minimeToken.balanceOf(attackController.attackerEOA()), 0, "Attacker EOA should not receive any funds"); + assertEq(minimeToken.balanceOf(sender), fundsAmount - sendAmount, "Sender should have expected funds"); + assertEq(minimeToken.balanceOf(receiver), sendAmount, "Receiver should have expected funds"); + } +}