0

https://github.com/vporton/test-web3-read-int128 produces a wrong value. Why? How to correct it?

npx buidler run scripts/mytest.js
...
-23309975763188660897098281009735482885

(should be 2).

pragma solidity ^0.7.0;

contract Files {
    int128 public arToETHCoefficient = 2;
}

pragma solidity ^0.7.0;

import './Relay.sol';

contract FilesRelayer is Relay {
    constructor(address initAddr) Relay(initAddr) { }
}

pragma solidity ^0.7.0;

import './Proxy.sol';

contract Relay is Proxy {
    address public currentVersion;
    address public owner;

    modifier onlyOwner() {
        require(msg.sender == owner);
        _;
    }

    constructor(address initAddr) {
        currentVersion = initAddr;
        owner = msg.sender; // this owner may be another contract with multisig, not a single contract owner
    }

    function changeContract(address newVersion) external
        onlyOwner()
    {
        currentVersion = newVersion;
    }

    function changeRelayer(address _owner) external
        onlyOwner()
    {
        owner = _owner;
    }

    function _implementation() internal override view returns (address) {
        return currentVersion;
    }
}

/**
 * License: MIT
 *
 * Copyright (c) 2018 zOS Global Limited.
 *
 * Permission is hereby granted, free of charge, to any person obtaining a copy
 * of this software and associated documentation files (the "Software"), to deal
 * in the Software without restriction, including without limitation the rights
 * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
 * copies of the Software, and to permit persons to whom the Software is
 * furnished to do so, subject to the following conditions:
 *
 * The above copyright notice and this permission notice shall be included in
 * copies or substantial portions of the Software.
 *
 * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
 * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
 * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
 * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
 * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 * SOFTWARE.
 */

pragma solidity ^0.7.1;

/**
 * @notice Implements delegation of calls to other contracts, with proper
 * forwarding of return values and bubbling of failures.
 * It defines a fallback function that delegates all calls to the address
 * returned by the abstract _implementation() internal function.
 * @dev Forked from https://github.com/zeppelinos/zos-lib/blob/8a16ef3ad17ec7430e3a9d2b5e3f39b8204f8c8d/contracts/upgradeability/Proxy.sol
 * Modifications:
 * 1. Reformat and conform to Solidity 0.6 syntax (5/13/20)
 */
abstract contract Proxy {
    /**
     * @dev Fallback function.
     * Implemented entirely in `_fallback`.
     */
    fallback() external payable {
        _fallback();
    }

    /**
     * @return The Address of the implementation.
     */
    function _implementation() internal virtual view returns (address);

    /**
     * @dev Delegates execution to an implementation contract.
     * This is a low level function that doesn't return to its internal call site.
     * It will return to the external caller whatever the implementation returns.
     * @param implementation Address to delegate.
     */
    function _delegate(address implementation) internal {
        assembly {
            // Copy msg.data. We take full control of memory in this inline assembly
            // block because it will not return to Solidity code. We overwrite the
            // Solidity scratch pad at memory position 0.
            calldatacopy(0, 0, calldatasize())

            // Call the implementation.
            // out and outsize are 0 because we don't know the size yet.
            let result := delegatecall(
                gas(),
                implementation,
                0,
                calldatasize(),
                0,
                0
            )

            // Copy the returned data.
            returndatacopy(0, 0, returndatasize())

            switch result
                // delegatecall returns 0 on error.
                case 0 {
                    revert(0, returndatasize())
                }
                default {
                    return(0, returndatasize())
                }
        }
    }

    /**
     * @dev Function that is run as the first thing in the fallback function.
     * Can be redefined in derived contracts to add functionality.
     * Redefinitions must call super._willFallback().
     */
    function _willFallback() internal virtual {}

    /**
     * @dev fallback implementation.
     * Extracted to enable manual triggering.
     */
    function _fallback() internal {
        _willFallback();
        _delegate(_implementation());
    }
}
const bre = require("@nomiclabs/buidler");

async function main() {
  // Buidler always runs the compile task when running scripts through it. 
  // If this runs in a standalone fashion you may want to call compile manually 
  // to make sure everything is compiled
  // await bre.run('compile');

  // We get the contract to deploy
  const Files = await ethers.getContractFactory("Files");
  const files = await Files.deploy();

  await files.deployed();

  console.log("Files deployed to:", files.address);

  const FilesRelayer = await ethers.getContractFactory("FilesRelayer");
  const deployResult = await FilesRelayer.deploy(files.address);
  if (deployResult.newlyDeployed) {
    console.log(`contract FilesRelayer deployed at ${deployResult.address} in block ${deployResult.receipt.blockNumber} using ${deployResult.receipt.gasUsed} gas`);
  }

  console.log("web3 version:", web3.version);
//  const interface = [{inputs: [], name: "arToETHCoefficient", outputs: [{internalType: "int128", name: "", type: "int128"}], stateMutability: "view", type: "function"}]
  const interface = JSON.parse(require('fs').readFileSync('artifacts/Files.json')).abi;
  contractInstance = new web3.eth.Contract(interface, deployResult.address);
  console.log(await contractInstance.methods.arToETHCoefficient().call());
}

// We recommend this pattern to be able to use async/await everywhere
// and properly handle errors.
main()
  .then(() => process.exit(0))
  .catch(error => {
    console.error(error);
    process.exit(1);
  });

1 Answer 1

2

You have probably guessed you have a fundamental issue with the storage. I will try to explain.

The whole idea of a "proxy" is that the proxy is running, so everything that happens unfolds in the context of the proxy contract and uses the proxy contract for storage. The "implementation" contract provides code that proxy injests and runs on-demand, so one can write contracts in approximately the usual way but there are some deadly mistakes to avoid.

This will be more a conceptual description so you get the idea. Internally, the EVM uses logical "slots" in a mapped namespace. It uses the contract address (for uniqueness) plus the slots the bytecode (compiler) provides to compute a hash which is an absolute logical location in the EVM.

Consider:

contract A {
  uint a; // slot 0
  uint b; // slot 1
}

Setting aside that optimizing etc. might rearrange things, let's just stay with this simple example and say that the compiler worked out slots 0 and 1 for the named variables.

Now, if Proxy delegates to contract A, then contract A will scribble over Proxy's slots 0 amd 1 because that's what the bytecode says to do. But, if Proxy was using those slots, say:

contract Proxy {
  address x; // slot 0
  address y; // slot 1
}

Well, then x & y get creamed when A decides to store something or the opposite. The contracts will cast the bits they find in the types they expect but it might be quite a mess. Nothing good can come from unwanted overwrites.

Two takeaways:

  1. You cannot safely store any value in a normal state variable in a Proxy contract, which is what your Relay actually does (resolving the implementation address). It will probably get stomped on and then the next call will be delegated to somewhere inappropriate.
  2. You can't assign a stored value in an implementation contract using a constructor because it will execute in the wrong context, therefore not the Proxy's storage, so it will behave as though the constructor never ran when you access it from the Proxy.

That's a bit of a dilema because a Proxy probably needs an implementation address and an admin address. How to store that stuff?

Workarounds:

This example is from the OpenZeppelin UpgradeableProxy.sol contract.

    /**
     * @dev Emitted when the implementation is upgraded.
     */
    event Upgraded(address indexed implementation);

    /**
     * @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 private constant _IMPLEMENTATION_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc;

    /**
     * @dev Returns the current implementation address.
     */
    function _implementation() internal override view returns (address impl) {
        bytes32 slot = _IMPLEMENTATION_SLOT;
        // solhint-disable-next-line no-inline-assembly
        assembly {
            impl := sload(slot)
        }
    }

It's using assembler to get a value from a specific place that is hopefully collision-resistant. Here is the setter:

    /**
     * @dev Stores a new address in the EIP1967 implementation slot.
     */
    function _setImplementation(address newImplementation) private {
        require(Address.isContract(newImplementation), "UpgradeableProxy: new implementation is not a contract");

        bytes32 slot = _IMPLEMENTATION_SLOT;

        // solhint-disable-next-line no-inline-assembly
        assembly {
            sstore(slot, newImplementation)
        }
    }

In essence, you have do that for anything you want to persist at the Proxy level so it is safe from overwrite by implementation contracts.

Constructors

Implementations should not set any values in the constructor. Wait for the contract to be deployed and they invoke a function, usually init() through the Proxy so whatever it does executes in the context of the Proxy and writes values to the Proxy's storage.

Just to be clear:

WHen outside a function, this uint x = 99; silently transforms to:

uint x;
constructor() ... {
  x = 99;

at compile time so it will not work as expected with the Proxy. You want something like

function init() ... { // invoke through proxy
  x = 99; // goes to proxy storage
}

Hope it helps.

3
  • You advised to use "keccak-256 hash of "eip1967.proxy.implementation" subtracted by 1". Isn't it even better to use a GUID (with 256 bytes) as a slot?
    – porton
    Sep 29, 2020 at 2:34
  • 1
    You're not wrong. You can indeed use something like bytes32 SLOT_IMPLEMENTATION = keccak256("my implementation slot") and it should work. EIP-1167 provides more certainty about collision avoidance so it's probably advisable to go with the flow in this case. github.com/ethereum/EIPs/pull/1967/… Sep 29, 2020 at 2:50
  • 1
    For why NOT to change the SLOT_IMPLEMENTATION value, the EIP states: "This EIP standardises the storage slot for the logic contract address, instead of a public method on the proxy contract as DelegateProxy (EIP897) does. The rationale for this is that proxies should never expose functions to end users that could potentially clash with those of the logic contract."
    – Cliff Hall
    Aug 21, 2021 at 15:29

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.