Beware of using tx.origin to verify user

According to Solidity doc,

msg.sender (address): sender of the message (current call)

tx.origin (address): sender of the transaction (full call chain)

tx.origin means the first account that initial transaction. However, the msg.sender is exactly the account that triggers the function.

Example

Let's take a look at today's drama.

Starring:

😐: 0x5B3, user A, victim. 

😋: 0xAb8, user B, A's friend

👾: 0x4B2, a hacker

Two contracts:

Wallet contract: 0xD91, deployed by user A😐.

PhishingWallet Contract: 0x9EC, deployed hacker👾.

Contract Analysis

There are three functions in the Wallet contract.

1. balance(): get the contract balance
2. deposit(): deposit ether into the contract
3. transfer(): transfer ether from contract to others.

 1// SPDX-License-Identifier: MIT
 2pragma solidity ^0.8.10;
 3
 4contract Wallet {
 5    address public owner;
 6
 7    constructor() {
 8        owner = msg.sender;
 9    }
10
11    function balance() public view returns (uint) {
12        return address(this).balance;
13    }
14
15    function deposit() payable public {
16
17    }
18
19    function transfer(address payable _to, uint _amount) public {
20        require(tx.origin == owner, "Only owner can transfer");
21
22        (bool sent, ) = _to.call{value: _amount}("");
23        require(sent, "Failed");
24    }
25}

The scenario is:

User A (😐 0x5B3) deploys this Wallet contract(0xD91) and deposits 10 ethers into it. Then he transfers 1 ether to User B(😋: 0xAb8). It works fine! Wallet contract balance: 9 ethers User B(😋: 0xAb8) balance: 1 ether.

On the hacker side:

The hacker(👾, 0x4B2) deploys a PhishingWallet contract(0x9EC) and cheats User A (😐 0x5B3) to interact with it.

Let's step into the PhishingWallet contract's transfer function. It calls the original Wallet contract's transfer function to transfer all ethers to hacker.

 1contract PhishingWallet {
 2    Wallet wallet;
 3    address public owner;
 4
 5    constructor(address payable _wallet_addr) {
 6        wallet = Wallet(_wallet_addr);
 7        owner = msg.sender;
 8    }
 9
10    function transfer(address payable _to, uint _amount) public {
11        wallet.transfer(payable(owner), address(wallet).balance);
12    }
13
14}

How can it happen?

The problem is in the Wallet contract's transfer function. The function verify ownership by tx.origin. Since user A (😐 0x5B3) invoked the PhishingWallet contract, the tx.origin value in the Wallet contract was still user A(😐 0x5B3) even the Wallet contract(0xD91) is invoked by the PhishingWallet contract(0x9EC). So this requirement check require(tx.origin == owner, "Only owner can transfer"); was passed.

Solutions

If we want to verify user, we should use msg.sender instead.

1function transfer(address payable _to, uint _amount) public {
2    // require(tx.origin == owner, "Only owner can transfer");
3    require(msg.sender == owner, "Only owner can transfer");
4
5    (bool sent, ) = _to.call{value: _amount}("");
6    require(sent, "Failed to send Ether");
7}

Even more, we can restrict only EOA(Externally Owned Account) can invoke the transfer function.

1function transfer(address payable _to, uint _amount) public {
2    require(tx.origin == msg.sender, "Only EOA can transfer");
3    //require(tx.origin == owner, "Only owner can transfer");
4    require(msg.sender == owner, "Only owner can transfer");
5
6    (bool sent, ) = _to.call{value: _amount}("");
7    require(sent, "Failed to send Ether");
8}