mirror of https://github.com/vacp2p/minime.git
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>
This commit is contained in:
parent
85ca91ccc9
commit
e715502da1
12
.gas-report
12
.gas-report
|
@ -3,16 +3,16 @@
|
||||||
| Deployment Cost | Deployment Size | | | | |
|
| Deployment Cost | Deployment Size | | | | |
|
||||||
| 1788057 | 9919 | | | | |
|
| 1788057 | 9919 | | | | |
|
||||||
| Function Name | min | avg | median | max | # calls |
|
| Function Name | min | avg | median | max | # calls |
|
||||||
| allowance | 808 | 808 | 808 | 808 | 2 |
|
| allowance | 808 | 808 | 808 | 808 | 3 |
|
||||||
| approve | 31708 | 31708 | 31708 | 31708 | 1 |
|
| approve | 30781 | 31244 | 31244 | 31708 | 2 |
|
||||||
| balanceOf | 2361 | 3441 | 2361 | 5601 | 9 |
|
| balanceOf | 2361 | 3138 | 2361 | 5601 | 13 |
|
||||||
| balanceOfAt | 1142 | 2585 | 2363 | 3603 | 26 |
|
| balanceOfAt | 1142 | 2585 | 2363 | 3603 | 26 |
|
||||||
| changeController | 758 | 758 | 758 | 758 | 4 |
|
| changeController | 758 | 1318 | 758 | 3558 | 5 |
|
||||||
| controller | 2447 | 2447 | 2447 | 2447 | 6 |
|
| controller | 2447 | 2447 | 2447 | 2447 | 6 |
|
||||||
| createCloneToken | 1832796 | 1832796 | 1832796 | 1832796 | 2 |
|
| createCloneToken | 1832796 | 1832796 | 1832796 | 1832796 | 2 |
|
||||||
| decimals | 294 | 294 | 294 | 294 | 6 |
|
| decimals | 294 | 294 | 294 | 294 | 6 |
|
||||||
| destroyTokens | 8956 | 8956 | 8956 | 8956 | 1 |
|
| destroyTokens | 8956 | 8956 | 8956 | 8956 | 1 |
|
||||||
| generateTokens | 2541 | 80795 | 94453 | 95751 | 10 |
|
| generateTokens | 2541 | 82036 | 94453 | 95751 | 11 |
|
||||||
| name | 3253 | 3253 | 3253 | 3253 | 6 |
|
| name | 3253 | 3253 | 3253 | 3253 | 6 |
|
||||||
| parentSnapShotBlock | 284 | 284 | 284 | 284 | 7 |
|
| parentSnapShotBlock | 284 | 284 | 284 | 284 | 7 |
|
||||||
| parentToken | 305 | 305 | 305 | 305 | 7 |
|
| parentToken | 305 | 305 | 305 | 305 | 7 |
|
||||||
|
@ -20,7 +20,7 @@
|
||||||
| totalSupply | 1911 | 2917 | 1911 | 4930 | 6 |
|
| totalSupply | 1911 | 2917 | 1911 | 4930 | 6 |
|
||||||
| totalSupplyAt | 1995 | 3029 | 3003 | 4606 | 7 |
|
| totalSupplyAt | 1995 | 3029 | 3003 | 4606 | 7 |
|
||||||
| transfer | 75187 | 75187 | 75187 | 75187 | 1 |
|
| transfer | 75187 | 75187 | 75187 | 75187 | 1 |
|
||||||
| transferFrom | 74194 | 74194 | 74194 | 74194 | 1 |
|
| transferFrom | 3495 | 48093 | 66590 | 74194 | 3 |
|
||||||
|
|
||||||
|
|
||||||
| contracts/MiniMeTokenFactory.sol:MiniMeTokenFactory contract | | | | | |
|
| contracts/MiniMeTokenFactory.sol:MiniMeTokenFactory contract | | | | | |
|
||||||
|
|
|
@ -9,5 +9,6 @@ GenerateTokensTest:testDeployment() (gas: 45553)
|
||||||
GenerateTokensTest:testGenerateTokens() (gas: 114564)
|
GenerateTokensTest:testGenerateTokens() (gas: 114564)
|
||||||
GenerateTokensTest:test_RevertWhen_SenderIsNotController() (gas: 14930)
|
GenerateTokensTest:test_RevertWhen_SenderIsNotController() (gas: 14930)
|
||||||
MiniMeTokenTest:testDeployment() (gas: 45598)
|
MiniMeTokenTest:testDeployment() (gas: 45598)
|
||||||
|
ReentrancyTest:testAttack() (gas: 229331)
|
||||||
TransferTest:testDeployment() (gas: 45814)
|
TransferTest:testDeployment() (gas: 45814)
|
||||||
TransferTest:testTransfer() (gas: 201218)
|
TransferTest:testTransfer() (gas: 201218)
|
|
@ -184,13 +184,6 @@ contract MiniMeToken is Controlled, IERC20 {
|
||||||
|
|
||||||
if (previousBalanceFrom < _amount) revert NotEnoughBalance();
|
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
|
// First update the balance array with the new value for the address
|
||||||
// sending the tokens
|
// sending the tokens
|
||||||
updateValueAtNow(balances[_from], previousBalanceFrom - _amount);
|
updateValueAtNow(balances[_from], previousBalanceFrom - _amount);
|
||||||
|
@ -201,6 +194,13 @@ contract MiniMeToken is Controlled, IERC20 {
|
||||||
if (previousBalanceTo + _amount < previousBalanceTo) revert Overflow(); // Check for overflow
|
if (previousBalanceTo + _amount < previousBalanceTo) revert Overflow(); // Check for overflow
|
||||||
updateValueAtNow(balances[_to], previousBalanceTo + _amount);
|
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
|
// An event to make the transfer easy to find on the blockchain
|
||||||
emit Transfer(_from, _to, _amount);
|
emit Transfer(_from, _to, _amount);
|
||||||
}
|
}
|
||||||
|
|
|
@ -1,2 +1,2 @@
|
||||||
forge-std/=lib/forge-std/src/
|
forge-std/=lib/forge-std/src/
|
||||||
@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts
|
@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/
|
||||||
|
|
|
@ -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);
|
||||||
|
}
|
||||||
|
}
|
|
@ -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;
|
||||||
|
}
|
||||||
|
}
|
|
@ -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");
|
||||||
|
}
|
||||||
|
}
|
Loading…
Reference in New Issue