onlyOwner check fails using Proxy Contract

Question

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?

Answer 1

Disclaimer: I haven't played around with this or tested this theory but I think I see a flaw in your thinking.

You have this in the proxy contract:

contract TestProxy {
    address public targetAddress;

If I'm not mistaken, that will put an address in slot 0 of the proxy because it is the first variable mentioned.

Then, proxy delegateCalls to:

contract TestLogic {
    ...
    address public owner;

Here is another address, also in slot 0 because it is mentioned first. But, this contract will run in the context of the proxy and use the proxy's state.

If I'm not mistaken, this organization will collide with the proxy's organization. Nothing good can come from inadvertently overwriting important information. The overwrite will happen in the context of TestLogic's constructor. Things will probably get weird.

This was likely to happen with any implementation contract because of the way your proxy stores important information. It happened when you added onlyOwner because it was the first thing you tried that stored something in the implementation contract's state - the first thing that writes.

I think you can resolve this by storing your proxy's information (the implementation contract) in safe, collision-resistant locations. You can do that with a little sleight of hand.

Generate a collision-resistant location and write directly to that slot. You have to use assembly to "take over" management of your storage layout.

contract TestProxy {
    // address public targetAddress; // NO!
    bytes32 private constant IMPL_ADDRESS_KEY = keccak256("Implementation address key");

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

    function setTargetAddress(address _address) public {
        require(_address != address(0));
        // targetAddress = _address;  // No!
        bytes32 implAddressStorageKey = IMPL_ADDRESS_KEY;
        address a = _address; // not sure this is strictly necessary
        //solium-disable-next-line security/no-inline-assembly
        assembly {
            sstore(implAddressStorageKey, a)
        }
    }

In case it isn't clear, that dispenses with the typical storage layout that will collide with the implementation contract's typical storage layout. We pick a slot more or less at random and then use assembly to write to the slot.

Want it back?

function implAddress() public view returns(address) {
    address i;
    bytes32 implAddressKey = IMPL_ADDRESS_KEY;
    //solium-disable-next-line security/no-inline-assembly
    assembly {
        i := sload(implAddressKey)
    }
    require(i != UNDEFINED, "Internal error. The implementation is undefined.");
    return i;
}

In case I botched my edits to fit it to your scenario, an example you can use to get some ideas: https://github.com/rob-Hitchens/TrustlessUpgrades/blob/master/contracts/Proxy.sol

The main difference is the use of a registry to hold implementation contracts. It's the same problem deep down. The proxy needs a way to hold the contract info that won't get creamed by implementation contract storage layouts.

Hope it helps.

Answer 2

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().