0

I have a PermissionsManager contract that uses OpenZeppelin's Role-Based Access Control (RBAC) template as a base. Currently function modifiers in my app call PermissionsManager directly to check for access rights. I'd like to have this done through a proxy instead, so that if PermissionsManager gets upgraded/redeployed, I don't have to change its address in every contract that uses RBAC, but only in a single place - its proxy.

In other words, I'd like to have PermissionsManagerProxy contract to delegate all calls to PermissionsManager. When I deploy PermissionsManager, I'm automatically an Admin, but the problem is that PermissionsManagerProxy doesn't see this, so I'm not an Admin if I ask the proxy. How can the deployer of the Proxy become an Admin?

In short, I have this flow and it works:

ADMIN_USER --calls--> testOnlyAdmin() of PermissionsTestProxy --delegates_to--> PermissionsTest --is--> Permissions --calls--> PermissionsManager --uses--> Roles library

I'd like to have this instead to allow for upgradability (new items in bold):

ADMIN_USER --calls--> testOnlyAdmin() of PermissionsTestProxy --delegates_to--> PermissionsTest --is--> Permissions --calls--> PermissionsManagerProxy --delegates_to--> PermissionsManager --uses--> Roles library

However, the call above fails, because ADMIN_USER doesn't have the Admin role according to PermissionsManagerProxy.

Simplified example code below.

pragma solidity ^0.4.25;

RBAC Library:

library Roles {
  struct Role {
    mapping (address => bool) bearer;
  }

  function add(Role storage role, address account) 
  internal
  {
    require(account != address(0));
    role.bearer[account] = true;
  }

  function remove(Role storage role, address account) 
  internal 
  {
    require(account != address(0));
    role.bearer[account] = false;
  }

  function has(Role storage role, address account)
    internal
    view
    returns (bool)
  {
    require(account != address(0));
    return role.bearer[account];
  }
}

PermissionsManager:

contract PermissionsManager {
  using Roles for Roles.Role;

  event LogAdminRoleAdded(address indexed account);
  event LogAdminRoleRemoved(address indexed account);

  Roles.Role internal adminRole;

  constructor() public {
    adminRole.add(msg.sender);
  }

  modifier onlyAdmin() {
    require(isAdmin(msg.sender));
    _;
  }

  function isAdmin(address account) 
  public
  view 
  returns (bool) 
  {
    return adminRole.has(account);
  }

  function addAdmin(address account) 
  public 
  onlyAdmin 
  {
    adminRole.add(account);
    emit LogAdminRoleAdded(account);
  }
}

Permissions contract accessing PermissionsManager's roles:

contract IPermissionsManager {  
    function isAdmin(address) public view returns (bool);
}

contract Permissions {    
    IPermissionsManager private permissionsManager;

    constructor(address _permissionsManagerAddress) public {
        permissionsManager = IPermissionsManager(_permissionsManagerAddress);
    }

    function isAdmin(address _who) 
    public
    view
    returns(bool) 
    {
        return permissionsManager.isAdmin(_who);
    }


    modifier onlyAdmin() {
    require(permissionsManager.isAdmin(msg.sender));
    _;
  }
}

Example of end-user contract:

contract PermissionsTest is Permissions {    
    event LogSuccess();

    constructor(address _permissionsManager)
    Permissions(_permissionsManager)
    public
    {    }

    function testAnyone()
    public  
    {
        emit LogSuccess();
    }

    function testOnlyAdmin()
    public
    onlyAdmin
    {
        emit LogSuccess();
    }
}

Proxy template:

contract Proxy {    
    address public _currentImplementation;

    constructor(address _initialImplementation) 
    public
    {
        _currentImplementation = _initialImplementation;
    }   

    function () 
    payable 
    public 
    {
        bool callSuccess = _currentImplementation.delegatecall(msg.data);
        if (callSuccess) {
            assembly {
                returndatacopy(0x0, 0x0, returndatasize)
                return(0x0, returndatasize)
            }
        } else {
            revert();
        }       
    }
}

Proxy of end-user's contract:

contract PermissionsTestProxy is Proxy {        
    constructor(address _initialImplementation)
    Proxy(_initialImplementation)
    public
    {   }
}

PermissionsManagerProxy:

contract PermissionsManagerProxy is Proxy { 
    constructor(address _initialImplementation)
    Proxy(_initialImplementation)
    public
    {   }
}
0

The reason this is happening is because you are checking msg.sender if it is an admin. This is as seen in your PermissionManager contract as shown below.

modifier onlyAdmin() {
  require(isAdmin(msg.sender));
  _;
}

If you do not already know about the difference between tx.origin and msg.sender, I would suggest you find out more. This could help. In addition, you should already know about delegate call and how it works since you are using it. If you haven't, please read here. In particular this snippet is useful in your case

When an account C invokes D, and D does DELEGATECALL on E, msg.sender inside E is C. That is, E has the same msg.sender and msg.value as D.

From the non-working flow that you have given as shown below:

ADMIN_USER --calls--> testOnlyAdmin() of PermissionsTestProxy --delegates_to--> PermissionsTest --is--> Permissions --calls--> PermissionsManagerProxy --delegates_to--> PermissionsManager --uses--> Roles library

In the step:

PermissionsTest --is--> Permissions --calls--> PermissionsManagerProxy --delegates_to--> PermissionsManager --uses--> Roles library

The msg.sender as seen by PermissionsManager in this case is PermissionsTest, which does not have an admin role.

To make it work, validate using tx.origin instead. tx.origin would refer to ADMIN_USER throughout your call chain and would allow your call to work.

modifier onlyAdmin() {
  require(isAdmin(tx.origin));
  _;
}

Of course, do note the risks behind using tx.origin and make sure you know what you are doing if you want to use it.

Hope this helps.

  • Hey @xAffinity, thanks for the tip. Indeed I've seen so many warnings for using tx.origin that I completely did not consider this option. While at a first glance I thought that should work, it still doesn't :(. I added all code to GitHub, so you can easily test on your end. See the ReadMe file for what works and what doesn't. Hope you can assist further - there must be a way for this to work!.. – Mike Feb 13 at 13:11
  • Hi @Mike, i have looked through the code and know what you are trying to do. However, you are using delegate call wrongly, and also the way you are testing is wrong. From the solidity docs, "only the code of the given address is used, all other aspects (storage, balance, …) are taken from the current contract." This means you need the proxy to have same storage structure as your logic contracts(PermissionManager and PermissionTest). As for tests, some fo the tests do not validate the return value of the function call, and so will always pass. – xAffinity Feb 13 at 16:04
  • Maybe this will help you in understanding how to use the upgradable proxy pattern. – xAffinity Feb 13 at 16:24
  • you have a point about the storage+delegateCall issue, but I'm just not a fan of the "eternal shared storage" approach. The eternal storage is simply not convenient and relies on the assumption that I wouldn't have to change its structure. If that happens, I would need to re-deploy the proxy too, which defeats the purpose of the proxy. I'd much rather go for the "Unstructured Upgradable Storage Proxy" as described here. – Mike Feb 20 at 15:52
  • Even then, the question remains how to ensure that the adminRole from PermissionsManager is known by (i.e. is in the storage of) PermissionsManagerProxy. Regardless whether I use the Eternal or the Unstructured storage proxy patterns, neither of them have a way of initializing the proxy contract with the correct ownership role. Or am I missing something? – Mike Feb 20 at 15:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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