Skip to content

Commit

Permalink
ACL: Multiple enhancements (#584)
Browse files Browse the repository at this point in the history 
* acl: optimize internal calls

* acl: allow knowing original user when set to any

* acl: move grant permission modifier to internal method

* ACL: Apply suggestions from @sohkai

Co-authored-by: Brett Sun <qisheng.brett.sun@gmail.com>

* acl: move grant permission modifier to external methods

* acl: clarify ACL oracle interface

* acl: improve internal logic eval functions naming

* acl: inline internal oracle helper function

* acl: add more test cases for oracles

* acl: add previous acl oracle interface

* ACL: update complex ACLOracle tests to be separate tests

Co-authored-by: Brett Sun <qisheng.brett.sun@gmail.com>
  • Loading branch information
@facuspagnuolo @sohkai
facuspagnuolo and sohkai committed Jul 1, 2020
1 parent d068879 commit a70d806
Show file tree
Hide file tree
Showing 7 changed files with 317 additions and 143 deletions.
260 changes: 165 additions & 95 deletions contracts/acl/ACL.sol
View file

Large diffs are not rendered by default.

4 changes: 1 addition & 3 deletions contracts/acl/IACL.sol
View file
Expand Up @@ -8,7 +8,5 @@ pragma solidity ^0.4.24;
interface IACL {
function initialize(address permissionsCreator) external;

// TODO: this should be external
// See https://github.com/ethereum/solidity/issues/4832
function hasPermission(address who, address where, bytes32 what, bytes how) public view returns (bool);
function hasPermission(address who, address where, bytes32 what, bytes how) external view returns (bool);
}
16 changes: 15 additions & 1 deletion contracts/acl/IACLOracle.sol
View file
Expand Up @@ -5,6 +5,20 @@
pragma solidity ^0.4.24;


/**
* @title ACL Oracle interface
* @dev This interface simply defines a check method that must be implemented by smart contracts to be plugged in as ACL oracles.
* ACL oracles are the most suitable way to have external contracts validating ACL permissions with custom logic.
*/
interface IACLOracle {
function canPerform(address who, address where, bytes32 what, uint256[] how) external view returns (bool);
/**
* @dev Tells whether `user` can execute `what`(`how`) in `where` if it's currently set up for `who`
* @param user Entity trying to execute `what` in `where`
* @param who Entity to which `what` is granted based on the current ACL permissions configuration
* @param where Entity where `what` is trying to be executed
* @param what Identifier of the action willing to be executed in `where`
* @param how Can be used to define a set of arguments to give more context about `what` is trying to be executed in `where`
* @return True if the user is allowed to execute the requested action for the given context, false otherwise
*/
function canPerform(address user, address who, address where, bytes32 what, uint256[] how) external view returns (bool);
}
15 changes: 15 additions & 0 deletions contracts/acl/IACLOracleV1.sol
View file
@@ -0,0 +1,15 @@
/*
* SPDX-License-Identifier: MIT
*/

pragma solidity ^0.4.24;


/**
* @title Previous version of the ACL Oracle interface
* @dev This interface simply defines a check method that must be implemented by smart contracts to be plugged in as ACL oracles.
* ACL oracles are the most suitable way to have external contracts validating ACL permissions with custom logic.
*/
interface IACLOracleV1 {
function canPerform(address who, address where, bytes32 what, uint256[] how) external view returns (bool);
}
28 changes: 20 additions & 8 deletions contracts/test/helpers/ACLHelper.sol
View file
Expand Up @@ -15,27 +15,27 @@ contract ACLHelper {


contract AcceptOracle is IACLOracle {
function canPerform(address, address, bytes32, uint256[]) external view returns (bool) {
function canPerform(address, address, address, bytes32, uint256[]) external view returns (bool) {
return true;
}
}


contract RejectOracle is IACLOracle {
function canPerform(address, address, bytes32, uint256[]) external view returns (bool) {
function canPerform(address, address, address, bytes32, uint256[]) external view returns (bool) {
return false;
}
}


contract RevertOracle is IACLOracle {
function canPerform(address, address, bytes32, uint256[]) external view returns (bool) {
function canPerform(address, address, address, bytes32, uint256[]) external view returns (bool) {
revert();
}
}

contract AssertOracle is IACLOracle {
function canPerform(address, address, bytes32, uint256[]) external view returns (bool) {
function canPerform(address, address, address, bytes32, uint256[]) external view returns (bool) {
assert(false);
}
}
Expand All @@ -44,28 +44,28 @@ contract AssertOracle is IACLOracle {
contract StateModifyingOracle /* is IACLOracle */ {
bool modifyState;

function canPerform(address, address, bytes32, uint256[]) external returns (bool) {
function canPerform(address, address, address, bytes32, uint256[]) external returns (bool) {
modifyState = true;
return true;
}
}

contract EmptyDataReturnOracle is IACLOracle {
function canPerform(address, address, bytes32, uint256[]) external view returns (bool) {
function canPerform(address, address, address, bytes32, uint256[]) external view returns (bool) {
assembly {
return(0, 0)
}
}
}

contract ConditionalOracle is IACLOracle {
function canPerform(address, address, bytes32, uint256[] how) external view returns (bool) {
function canPerform(address, address, address, bytes32, uint256[] how) external view returns (bool) {
return how[0] > 0;
}
}

contract OverGasLimitOracle is IACLOracle {
function canPerform(address, address, bytes32, uint256[]) external view returns (bool) {
function canPerform(address, address, address, bytes32, uint256[]) external view returns (bool) {
while (true) {
// Do an SLOAD to increase the per-loop gas costs
uint256 loadFromStorage;
Expand All @@ -74,3 +74,15 @@ contract OverGasLimitOracle is IACLOracle {
return true;
}
}

contract OnlyOwnerOracle is IACLOracle {
address owner;

constructor (address _owner) public {
owner = _owner;
}

function canPerform(address user, address, address, bytes32, uint256[]) external view returns (bool) {
return user == owner;
}
}
72 changes: 62 additions & 10 deletions contracts/test/tests/TestACLInterpreter.sol
View file
Expand Up @@ -39,7 +39,7 @@ contract TestACLInterpreter is ACL, ACLHelper {
assertEval(arr(msg.sender), 0, Op.NEQ, uint256(this), true);
}

function testGreatherThan() public {
function testGreaterThan() public {
assertEval(arr(uint256(10), 11), 0, Op.GT, 9, true);
assertEval(arr(uint256(10), 11), 0, Op.GT, 10, false);
assertEval(arr(uint256(10), 11), 1, Op.GT, 10, true);
Expand All @@ -51,7 +51,7 @@ contract TestACLInterpreter is ACL, ACLHelper {
assertEval(arr(uint256(10), 11), 1, Op.LT, 10, false);
}

function testGreatherThanOrEqual() public {
function testGreaterThanOrEqual() public {
assertEval(arr(uint256(10), 11), 0, Op.GTE, 9, true);
assertEval(arr(uint256(10), 11), 0, Op.GTE, 10, true);
assertEval(arr(uint256(10), 11), 1, Op.GTE, 12, false);
Expand Down Expand Up @@ -87,9 +87,13 @@ contract TestACLInterpreter is ACL, ACLHelper {

// conditional oracle returns true if first param > 0
ConditionalOracle conditionalOracle = new ConditionalOracle();

assertEval(arr(uint256(1)), ORACLE_PARAM_ID, Op.EQ, uint256(conditionalOracle), true);
assertEval(arr(uint256(0), uint256(1)), ORACLE_PARAM_ID, Op.EQ, uint256(conditionalOracle), false);

// only owner oracle returns true if sender is the defined owner
OnlyOwnerOracle onlyOwnerOracle = new OnlyOwnerOracle(msg.sender);
assertEval(arr(), ORACLE_PARAM_ID, Op.EQ, uint256(onlyOwnerOracle), msg.sender, ANY_ENTITY, true);
assertEval(arr(), ORACLE_PARAM_ID, Op.EQ, uint256(onlyOwnerOracle), address(this), ANY_ENTITY, false);
}

function testReturn() public {
Expand Down Expand Up @@ -127,10 +131,51 @@ contract TestACLInterpreter is ACL, ACLHelper {
params[5] = Param(0, uint8(Op.LT), uint240(10));
params[6] = Param(PARAM_VALUE_PARAM_ID, uint8(Op.RET), 0);

// must return true for arg = 10
assertEval(params, arr(uint256(10)), true);
}

function testAnotherComplexCombination() public {
// if (oracle and block number > block number - 1) then arg 0 < 10 and oracle else false
Param[] memory params = new Param[](7);
params[0] = Param(LOGIC_OP_PARAM_ID, uint8(Op.IF_ELSE), encodeIfElse(1, 4, 6));
params[1] = Param(LOGIC_OP_PARAM_ID, uint8(Op.AND), encodeOperator(2, 3));
params[2] = Param(ORACLE_PARAM_ID, uint8(Op.EQ), uint240(new AcceptOracle()));
params[3] = Param(BLOCK_NUMBER_PARAM_ID, uint8(Op.GT), uint240(block.number - 1));
params[4] = Param(LOGIC_OP_PARAM_ID, uint8(Op.AND), encodeOperator(5, 2));
params[5] = Param(0, uint8(Op.LT), uint240(10));
params[6] = Param(PARAM_VALUE_PARAM_ID, uint8(Op.RET), 0);

// Requires 0 < 10 AND oracle, so 10 should fail and 9 should pass
assertEval(params, arr(uint256(10)), false);
assertEval(params, arr(uint256(9)), true);
}

function testComplexCombinationWithOnlyOwnerOracle () public {
// Oracle only returns true for msg.sender
OnlyOwnerOracle onlyOwnerOracle = new OnlyOwnerOracle(msg.sender);

// if (oracle and block number > block number - 1) then arg 0 < 10 and oracle else false
Param[] memory params = new Param[](7);
params[0] = Param(LOGIC_OP_PARAM_ID, uint8(Op.IF_ELSE), encodeIfElse(1, 4, 6));
params[1] = Param(LOGIC_OP_PARAM_ID, uint8(Op.AND), encodeOperator(2, 3));
params[2] = Param(ORACLE_PARAM_ID, uint8(Op.EQ), uint240(onlyOwnerOracle));
params[3] = Param(BLOCK_NUMBER_PARAM_ID, uint8(Op.GT), uint240(block.number - 1));
params[4] = Param(LOGIC_OP_PARAM_ID, uint8(Op.AND), encodeOperator(5, 2));
params[5] = Param(0, uint8(Op.LT), uint240(10));
params[6] = Param(PARAM_VALUE_PARAM_ID, uint8(Op.RET), 0);

// Requires 0 < 10 AND oracle, so 10 should always fail
assertEval(params, arr(uint256(10)), address(this), ANY_ENTITY, false);
assertEval(params, arr(uint256(10)), msg.sender, ANY_ENTITY, false);
assertEval(params, arr(uint256(10)), address(this), address(0), false);
assertEval(params, arr(uint256(10)), msg.sender, address(0), false);

// 9 only passes if the caller is msg.sender
assertEval(params, arr(uint256(9)), address(this), ANY_ENTITY, false);
assertEval(params, arr(uint256(9)), msg.sender, ANY_ENTITY, true);
assertEval(params, arr(uint256(9)), address(this), address(0), false);
assertEval(params, arr(uint256(9)), msg.sender, address(0), true);
}

function testParamOutOfBoundsFail() public {
Expand Down Expand Up @@ -233,26 +278,33 @@ contract TestACLInterpreter is ACL, ACLHelper {
assertEval(params, false);
}


function assertEval(uint256[] memory args, uint8 argId, Op op, uint256 value, bool expected) internal {
function assertEval(uint256[] memory args, uint8 argId, Op op, uint256 value, address user, address who, bool expected) internal {
Param[] memory params = new Param[](1);
params[0] = Param(argId, uint8(op), uint240(value));
assertEval(params, args, expected);
assertEval(params, args, user, who, expected);
}

function assertEval(uint256[] memory args, uint8 argId, Op op, uint256 value, bool expected) internal {
assertEval(args, argId, op, value, address(0), address(0), expected);
}

function assertEval(Param[] memory params, bool expected) internal {
assertEval(params, new uint256[](0), expected);
assertEval(params, new uint256[](0), address(0), address(0), expected);
}

function assertEval(Param[] memory params, uint256[] memory args, bool expected) internal {
bytes32 paramHash = encodeAndSaveParams(params);
bool allow = _evalParam(paramHash, 0, address(0), address(0), bytes32(0), args);
assertEval(params, args, address(0), address(0), expected);
}

function assertEval(Param[] memory params, uint256[] memory args, address user, address who, bool expected) internal {
bytes32 paramHash = _encodeAndSaveParams(params);
bool allow = _evalParam(paramHash, 0, user, who, address(0), bytes32(0), args);

Assert.equal(allow, expected, "eval got unexpected result");
}

event LogParam(bytes32 param);
function encodeAndSaveParams(Param[] memory params) internal returns (bytes32) {
function _encodeAndSaveParams(Param[] memory params) internal returns (bytes32) {
uint256[] memory encodedParams = new uint256[](params.length);

for (uint256 i = 0; i < params.length; i++) {
Expand Down

0 comments on commit a70d806

Please sign in to comment.