0

I was testing delegate calls with the proxy pattern for the upgradable contract. Things were going good till the time I added onlyOwner modifier.

My Contracts

pragma solidity 0.5.8;

contract TestLogicInterface {
    function getX() public view returns (uint);
    function setX(uint _newX) public;
    function owner() public view returns(address);
    function setOwner(address _owner) public;
    function testMessageSender() public view returns(address, address);
}

contract TestState {
    uint x = 12;

    function getX() public view returns (uint) {
        return x;
    }

    function setX(uint _newX) public {
        x = _newX;
    }
}

contract TestLogic {

    TestState public _dataLayer;

    address public owner;

    constructor(address _dl, address _owner) public {
        _dataLayer = TestState(_dl);
        owner = _owner;
    }

    modifier onlyOwner {
        require(msg.sender == owner, "Only owner is allowed");
        _;
    }

    function setOwner(address _owner) public {
        owner = _owner;
    }

    function setX(uint _newX) public onlyOwner {
        _dataLayer.setX(_newX);
    }

    function getX() public view returns (uint) {
        return _dataLayer.getX();
    }

    function testMessageSender() public view returns(address, address){
        return (msg.sender, owner);
    }
}

contract TestProxy {
    address public targetAddress;

    constructor(address _lc) public {
        setTargetAddress(_lc);
    }

    function setTargetAddress(address _address) public {
        require(_address != address(0));
        targetAddress = _address;
    }

    function () external {
        address contractAddr = targetAddress;
        assembly {
            let ptr := mload(0x40)
            calldatacopy(ptr, 0, calldatasize)
            let result := delegatecall(gas, contractAddr, ptr, calldatasize, 0, 0)
            let size := returndatasize
            returndatacopy(ptr, 0, size)

            switch result
            case 0 { revert(ptr, size) }
            default { return(ptr, size) }
        }

    }
}

Test Cases

const TestState = artifacts.require("TestState")
const TestLogic = artifacts.require("TestLogic")
const TestProxy = artifacts.require("TestProxy")
const LogicInterface = artifacts.require("TestLogicInterface")

let proxy, states, logic, proxyInterfaced

contract('test upgrade', async(accounts) => {

    beforeEach(async() =>{
        state = await TestState.deployed()
        logic = await TestLogic.deployed()
        proxy = await TestProxy.deployed()
        proxyInterfaced = await LogicInterface.at(proxy.address);
        await proxyInterfaced.setOwner(accounts[0])
    })

    it("logic_state", async() => {
        let x = (await proxyInterfaced.getX()).toNumber()
        console.log({x})

        let msgSender = await proxyInterfaced.testMessageSender()
        console.log({msgSender})

        await proxyInterfaced.setX(11, {gas: 300000})
    })

})

I added another function testMessageSender to check if msg.sender is correct and matches the owner. The result of the function was:

msg.sender: '0x4c256B6945a3FFCbf93463D8c0ff914C533bC0Aa',
owner: '0x4c256B6945a3FFCbf93463D8c0ff914C533bC0Aa'

Clearly they both are the same, then why is my test case not running. I am getting an error:

Error: Returned error: VM Exception while processing transaction: revert Only owner is allowed -- Reason given: Only owner is allowed.

If I remove the modifier, everything works perfectly. What am I missing?

Edit 1:

I updated my setX function and added event to check if my assertion is correct.

 event test(address _owner, address _sender);
 function setX(uint _newX) public {
    _dataLayer.setX(_newX);
    emit test(owner, msg.sender);
 }

and I can see when I call this function from tests, 2 events are fired instead of one with following data.

    test(_owner: 0x4c256B6945a3FFCbf93463D8c0ff914C533bC0Aa (address), _sender: 0xC63268D4082Dd102cC2730cD606B0f9EedC8B9E2 (address))
   test(_owner: 0x4c256B6945a3FFCbf93463D8c0ff914C533bC0Aa (address), _sender: 0x4c256B6945a3FFCbf93463D8c0ff914C533bC0Aa (address))

The second one says both msg.sender and owner are same while first one says they are different. I am more puzzled now.

Why 2 events are fired instead of one?

Edit 2:

If I don't use State Contract and keep states in Logic Contract itself, everything works perfect.

I noticed another absurd behavior. If I change methods of my TestState from getX to getterX. The test cases fail, and If I rename them back to getX and setX, everything works again.

Is there a corelation between names of function of state and logic contract?

1
  • How are contracts initialized?
    – Ismael
    Oct 21, 2019 at 15:23

3 Answers 3

0

I think delegatecall is an evil, becasue it is counterintuitive. When you set owner with the code await proxyInterfaced.setOwner(accounts[0]), the value is stored in TestProxy instead of TestLogic, and so does the getter function: when you run the code await proxyInterfaced.setX(11, {gas: 300000}), the setX function read _dataLayer first in the TestProxy contract context! The code read the first slot of contract TestProxy and it's value is addr_TestLogic! not addr_TestState! That explains why the event have been tiggered twice.

You can add a function in TestLogicInterface function _dataLayer() public view returns(address); And then check the value :await proxyInterfaced._dataLayer().

0

msg.sender is the address of the contract that calls the function. If you want to refer the user that calls the main function, you must use tx.origin.

0

Updating this answer for others who come across it.

I don't see any event emitters, so I'm unsure what is meant by "2 events". Possibly internal transactions, I suppose.

The problem is the use of a constructor to set the owner in the logic/implementation contract.

This will not work, owing to "context" and delegatecall.

delegatecall executes the logic in the context of the proxy, meaning the msg.sender is whoever called the proxy, and the state is the proxy's state.

The owner will be in slot0 because it's the first state variable declared in the implementation.

In the implementation context, the constructor fires when the contract is deployed, so the owner is written to slot0.

When the proxy is used to run the implementation code, it reads from slot0 because that's what the code tells it to do. But slot0 in the proxy hasn't been updated because nothing told it to, so the owner is 0x0 which cannot be the sender, so "only owner, bu-bye".

When dealing with proxies, use an initializer. Migrate all of the constructor concerns to an initialize function and make sure you call it from the proxy (and make sure you call it exactly once).

Approximately:

contract Implementation {

  address public owner;

  modifier onlyInitializing() {
    require (owner == address(0), "already initialized");
    _;
  }

  // constructor() { }  NO - this will write the implementation's storage

  function init(address owner_) external onlyInitializing {
    owner = owner_; // called from proxy, this writes to proxy's storage
  }
{

You'll see this pattern in @openZeppelin/contracts-upgradeable where the contracts are very similar to @openzeppelin/contracts but they initialize with context in mind.

While the following is not good description of the implementation details, it's a good heuristic for a developer have intuition about what's going on. It runs "as though" the proxy contract is executing and it has imported executable code from the implementation and blindly run it in place - reading/writing to/from its own storage as the executable code tells it to do. Therefore, whatever was written by the constructor when the implementation contract was deployed is totally ignored.

immutables can be set by implementation constructors because they modify the bytecode itself, therefore "sticky".

Hope it helps.

1
  • Thanks, @Rob for detailed answer. I am trying to get my head around your theory, but that doesn't explain 2 events being triggered. Also, see my second edit. If I change function names of State Contract to anything else, test cases break. And if I keep state in Logic Contract everything works even with onlyOwner modifier. Oct 22, 2019 at 7:14

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.