How to use Role-Based Access Control via Proxy?

Question

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
    {   }
}

Answer 1

On first read, by "Proxy", I assumed you meant a transparent proxy such as Open Zeppelin's TransparentUpgradeableProxy.sol. On second read, your approach seems more like applying modularity. Nothing wrong with that and I wouldn't recommend switching to the advanced "Proxy" pattern until you're really strong with Solidity.

tx.origin is not the preferred way to handle this, for security reasons, but to amplify what @xAffinity msg.sender said, it's the inner-most sender. So, in a chain of calls, it's not the transaction signer but rather the contract that is asking.

signer => A => B => C

C.msgSender == B

You can easily handle this by including a user in function parameters. There is no added risk if your own trusted contract is specifying the user context to another trusted contract.

contract A {
modifier checkSomething {
    bool foo = B.checkSomething(msg.sender); // from transaction signer
    require(foo, "403");
    _;
  }
}
contract B {
function checkSomething(address user) ... returns (bool) {
     return C.something(user);
  }
}
contract C {
function something(address user) ... returns (bool) {
     if (...) return true;
     return false;
  }
}

That's just a little scribble for illustrative purposes. Not meant to compile. ;-)

Hope it helps.

Answer 2

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.