Add controller.genericCall (#482)

* Add controller.genericCall

Add controller.genericCall

Update gaslimit

Remove genericAction (delegate)

fix test

* Update Avatar.sol

contracts/controller/Avatar.sol

@@ -6,11 +6,6 @@ import "openzeppelin-solidity/contracts/ownership/Ownable.sol";
 import "openzeppelin-solidity/contracts/token/ERC20/StandardToken.sol";
 
 
-contract ActionInterface {
-    function action(bytes32[] _params) public returns(bool);
-}
-
-
 /**
  * @title An Avatar holds tokens, reputation and ether for a controller
  */
@@ -45,26 +40,24 @@ contract Avatar is Ownable {
     }
 
     /**
-    * @dev call an action function on an ActionInterface.
-    * This function use delegatecall and might expose the organization to security
-    * risk. Use this function only if you really knows what you are doing.
-    * @param _action the address of the contract to call.
-    * @param _params the params for the call.
-    * @return bool which represents success
+    * @dev perform a generic call to an arbitrary contract
+    * @param _contract  the contract's address to call
+    * @param _data ABI-encoded contract call to call `_contract` address.
+    * @return the return bytes of the called contract's function.
     */
-    function genericAction(address _action, bytes32[] _params)
-    public onlyOwner returns(bool)
-    {
-        emit GenericAction(_action, _params);
-        require(
-          // solium-disable-next-line security/no-low-level-calls
-            _action.delegatecall(
-                bytes4(keccak256("action(bytes32[])")),
-                uint256(32),// pointer to the length of the array
-                uint256(_params.length), // length of the array
-                _params) // array itself
-        );
-        return true;
+    function genericCall(address _contract,bytes _data) public onlyOwner {
+        // solium-disable-next-line security/no-low-level-calls
+        bool result = _contract.call(_data);
+        // solium-disable-next-line security/no-inline-assembly
+        assembly {
+        // Copy the returned data.
+        returndatacopy(0, 0, returndatasize)
+
+        switch result
+        // call returns 0 on error.
+        case 0 { revert(0, returndatasize) }
+        default { return(0, returndatasize) }
+        }
     }
 
     /**

contracts/controller/Controller.sol

@@ -23,7 +23,8 @@ contract Controller is ControllerInterface {
                              // 2nd bit: Scheme can register other schemes
                              // 3rd bit: Scheme can add/remove global constraints
                              // 4th bit: Scheme can upgrade the controller
-                             // 5th bit: Scheme can call delegatecall
+                             // 5th bit: Scheme can call genericCall on behalf of
+                             //          the organization avatar
     }
 
     struct GlobalConstraint {
@@ -67,6 +68,7 @@ contract Controller is ControllerInterface {
     event UpgradeController(address indexed _oldController,address _newController);
     event AddGlobalConstraint(address indexed _globalConstraint, bytes32 _params,GlobalConstraintInterface.CallPhase _when);
     event RemoveGlobalConstraint(address indexed _globalConstraint ,uint256 _index,bool _isPre);
+    event GenericCall(address indexed _contract,bytes _data);
 
     constructor( Avatar _avatar) public
     {
@@ -102,7 +104,7 @@ contract Controller is ControllerInterface {
         _;
     }
 
-    modifier onlyDelegateScheme() {
+    modifier onlyGenericCallScheme() {
         require(schemes[msg.sender].permissions&bytes4(16) == bytes4(16));
         _;
     }
@@ -258,7 +260,7 @@ contract Controller is ControllerInterface {
     }
 
     function getGlobalConstraintParameters(address _globalConstraint,address) external view returns(bytes32) {
-        
+
         GlobalConstraintRegister memory register = globalConstraintsRegisterPre[_globalConstraint];
 
         if (register.isRegistered) {
@@ -406,21 +408,27 @@ contract Controller is ControllerInterface {
     }
 
     /**
-    * @dev do a generic delegate call to the contract which called us.
-    * This function use delegatecall and might expose the organization to security
-    * risk. Use this function only if you really knows what you are doing.
-    * @param _params the params for the call.
-    * @return bool which represents success
+    * @dev perform a generic call to an arbitrary contract
+    * @param _contract  the contract's address to call
+    * @param _data ABI-encoded contract call to call `_contract` address.
+    * @param _avatar the controller's avatar address
+    * @return bytes32  - the return value of the called _contract's function.
     */
-    function genericAction(bytes32[] _params,address _avatar)
+    function genericCall(address _contract,bytes _data,address _avatar)
     external
-    onlyDelegateScheme
-    onlySubjectToConstraint("genericAction")
+    onlyGenericCallScheme
+    onlySubjectToConstraint("genericCall")
     isAvatarValid(_avatar)
-    returns(bool)
+    returns (bytes32)
     {
-        emit GenericAction(msg.sender, _params);
-        return avatar.genericAction(msg.sender, _params);
+        emit GenericCall(_contract, _data);
+        avatar.genericCall(_contract, _data);
+        // solium-disable-next-line security/no-inline-assembly
+        assembly {
+        // Copy the returned data.
+        returndatacopy(0, 0, returndatasize)
+        return(0, returndatasize)
+        }
     }
 
   /**

contracts/controller/ControllerInterface.sol

@@ -117,17 +117,18 @@ interface ControllerInterface {
     */
     function upgradeController(address _newController,address _avatar)
     external returns(bool);
+
     /**
-    * @dev do a generic delegate call to the contract which called us.
-    * This function use delegatecall and might expose the organization to security
-    * risk. Use this function only if you really knows what you are doing.
-    * @param _params the params for the call.
-    * @param _avatar address
-    * @return bool which represents success
+    * @dev perform a generic call to an arbitrary contract
+    * @param _contract  the contract's address to call
+    * @param _data ABI-encoded contract call to call `_contract` address.
+    * @param _avatar the controller's avatar address
+    * @return bytes32  - the return value of the called _contract's function.
     */
-    function genericAction(bytes32[] _params,address _avatar)
+    function genericCall(address _contract,bytes _data,address _avatar)
     external
-    returns(bool);
+    returns(bytes32);
+
   /**
    * @dev send some ether
    * @param _amountInWei the amount of ether (in Wei) to send

contracts/controller/UController.sol

@@ -76,6 +76,8 @@ contract UController is ControllerInterface {
     event ExternalTokenIncreaseApproval (address indexed _sender, StandardToken indexed _externalToken, address _spender, uint _value);
     event ExternalTokenDecreaseApproval (address indexed _sender, StandardToken indexed _externalToken, address _spender, uint _value);
     event UpgradeController(address indexed _oldController,address _newController,address _avatar);
+    event GenericCall(address indexed _contract,bytes _data,address indexed _avatar);
+
     event AddGlobalConstraint(
         address indexed _globalConstraint,
         bytes32 _params,
@@ -129,7 +131,7 @@ contract UController is ControllerInterface {
         _;
     }
 
-    modifier onlyDelegateScheme(address _avatar) {
+    modifier onlyGenericCallScheme(address _avatar) {
         require(organizations[_avatar].schemes[msg.sender].permissions&bytes4(16) == bytes4(16));
         _;
     }
@@ -391,21 +393,25 @@ contract UController is ControllerInterface {
     }
 
     /**
-    * @dev do a generic delegate call to the contract which called us.
-    * This function use delegatecall and might expose the organization to security
-    * risk. Use this function only if you really knows what you are doing.
-    * @param _params the params for the call.
-    * @param _avatar the organization avatar.
-    * @return bool which represents success
+    * @dev perform a generic call to an arbitrary contract
+    * @param _contract  the contract's address to call
+    * @param _data ABI-encoded contract call to call `_contract` address.
+    * @return bytes32  - the return value of the called _contract's function.
     */
-    function genericAction(bytes32[] _params,address _avatar)
+    function genericCall(address _contract,bytes _data,address _avatar)
     external
-    onlyDelegateScheme(_avatar)
-    onlySubjectToConstraint("genericAction",_avatar)
-    returns(bool)
+    onlyGenericCallScheme(_avatar)
+    onlySubjectToConstraint("genericCall",_avatar)
+    returns (bytes32)
     {
-        emit GenericAction(msg.sender, _params);
-        return (Avatar(_avatar)).genericAction(msg.sender, _params);
+        emit GenericCall(_contract, _data,_avatar);
+        (Avatar(_avatar)).genericCall(_contract, _data);
+        // solium-disable-next-line security/no-inline-assembly
+        assembly {
+        // Copy the returned data.
+        returndatacopy(0, 0, returndatasize)
+        return(0, returndatasize)
+        }
     }
 
   /**

contracts/test/ActionMock.sol

@@ -3,18 +3,13 @@ pragma solidity ^0.4.24;
 import "../controller/Avatar.sol";
 
 
-contract ActionMock is ActionInterface {
+contract ActionMock {
 
-    event Action(address _sender,bytes32 _param);
-
-    function action(bytes32[] params) public returns(bool) {
-        emit Action(msg.sender,params[0]);
-        require(params[0] != 0x1234000000000000000000000000000000000000000000000000000000000000);
-        return true;
-    }
-
-    function genericAction(Avatar avatar,bytes32[] params) public returns(bool) {
-        return avatar.genericAction(address(this),params);
+    function test(uint _a,address _b,bytes32 _c) public view returns(uint) {
+        require(_a == 7);
+        require(_b == address(this));
+        require(_c == 0x1234000000000000000000000000000000000000000000000000000000000000);
+        return _a*2;
     }
 
 }

contracts/test/UniversalSchemeMock.sol

@@ -1,10 +1,31 @@
 pragma solidity ^0.4.24;
 
 import "../universalSchemes/UniversalScheme.sol";
+import "../controller/ControllerInterface.sol";
 
 
 contract UniversalSchemeMock is UniversalScheme {
 
     constructor() public {
     }
+
+    function genericCall(address _avatar,address _contract,uint _a,address _b,bytes32 _c)
+    public returns(bytes32)
+    {
+
+        address controller = Avatar(_avatar).owner();
+        return ControllerInterface(controller).genericCall(_contract,abi.encodeWithSignature("test(uint256,address,bytes32)",_a,_b,_c),_avatar);
+    }
+
+    function genericCallDirect(address _avatar,address _contract,uint _a,address _b,bytes32 _c)
+    public returns(bytes32)
+    {
+        Avatar(_avatar).genericCall(_contract,abi.encodeWithSignature("test(uint256,address,bytes32)",_a,_b,_c));
+        // solium-disable-next-line security/no-inline-assembly
+        assembly {
+        // Copy the returned data.
+        returndatacopy(0, 0, returndatasize)
+        return(0, returndatasize)
+        }
+    }
 }

contracts/universalSchemes/VoteInOrganizationScheme.sol

@@ -8,7 +8,7 @@ import "./UniversalScheme.sol";
  * @title VoteInOrganizationScheme.
  * @dev A scheme to allow an organization to vote in a proposal.
  */
-contract VoteInOrganizationScheme is UniversalScheme, ExecutableInterface, ActionInterface {
+contract VoteInOrganizationScheme is UniversalScheme, ExecutableInterface {
     event NewVoteProposal(
         address indexed _avatar,
         bytes32 indexed _proposalId,
@@ -141,27 +141,16 @@ contract VoteInOrganizationScheme is UniversalScheme, ExecutableInterface, Actio
             }
 
             ControllerInterface controller = ControllerInterface(Avatar(_avatar).owner());
-            bytes32[] memory tmp = new bytes32[](3);
-            tmp[0] = bytes32(address(proposal.originalIntVote));
-            tmp[1] = proposal.originalProposalId;
-            tmp[2] = bytes32(param);
-            retVal = controller.genericAction(tmp,_avatar);
+            if (controller.genericCall(
+                     address(proposal.originalIntVote),
+                     abi.encodeWithSignature("vote(bytes32,uint256)",
+                     proposal.originalProposalId,
+                     uint(param)),
+                     _avatar) == bytes32(0)) {
+                retVal = false;
+            }
           }
         emit ProposalExecuted(_avatar, _proposalId,_param);
         return retVal;
     }
-
-    /**
-    * @dev do the actual voting in the other organization in behalf of the organization's avatar.
-    * @param _params array represent the voting .
-    *        _params[0] - the address of the voting machine.
-    *        _params[1] - the proposalId.
-    *        _params[2] - the voting machine params.
-    * @return bool which indicate success.
-    */
-    function action(bytes32[] _params) public returns(bool) {
-        IntVoteInterface intVote = IntVoteInterface(address(_params[0]));
-        emit VoteOnBehalf(_params);
-        return intVote.vote(_params[1], uint(_params[2]));
-    }
 }

test/avatar.js

@@ -2,7 +2,7 @@ const helpers = require('./helpers');
 const Avatar = artifacts.require("./Avatar.sol");
 const StandardTokenMock = artifacts.require('./test/StandardTokenMock.sol');
 const ActionMock = artifacts.require('./test/ActionMock.sol');
-
+const UniversalSchemeMock = artifacts.require('./test/UniversalSchemeMock.sol');
 
 let avatar,accounts;
 
@@ -14,11 +14,15 @@ const setup = async function () {
 
 contract('Avatar', function (accounts)  {
 
-    it("genericAction no owner", async () => {
+    it("genericCall no owner", async () => {
         avatar = await setup();
-        let action = await ActionMock.new();
+        let actionMock = await ActionMock.new();
+        var scheme = await UniversalSchemeMock.new();
+        let a = 7;
+        let b = actionMock.address;
+        let c = 0x1234;
         try{
-         await avatar.genericAction(action.address,[0],{ from: accounts[1] });
+         await scheme.genericCallDirect.call(avatar.address,actionMock.address,a,b,c,{from :accounts[1]});
          assert(false, "genericAction should fail due to wrong owner");
          } catch (ex) {
              helpers.assertVMException(ex);
@@ -27,20 +31,26 @@ contract('Avatar', function (accounts)  {
 
     it("generic call", async () => {
         avatar = await setup();
-        let action = await ActionMock.new();
-        await avatar.transferOwnership(action.address);
-        var tx = await action.genericAction(avatar.address,[0x4567]);
-        assert.equal(tx.logs.length, 1);
-        assert.equal(tx.logs[0].event, "Action");
-        assert.equal(tx.logs[0].args._param, "0x4567000000000000000000000000000000000000000000000000000000000000");
+        let actionMock = await ActionMock.new();
+        var scheme = await UniversalSchemeMock.new();
+        await avatar.transferOwnership(scheme.address);
+        let a = 7;
+        let b = actionMock.address;
+        let c = 0x1234;
+        var result = await scheme.genericCallDirect.call(avatar.address,actionMock.address,a,b,c);
+        assert.equal(result,a*2);
     });
 
     it("generic call should revert if action revert", async () => {
         avatar = await setup();
-        let action = await ActionMock.new();
-        await avatar.transferOwnership(action.address);
+        let actionMock = await ActionMock.new();
+        var scheme = await UniversalSchemeMock.new();
+        await avatar.transferOwnership(scheme.address);
+        let a = 7;
+        let b = actionMock.address;
+        let c = 0x4567; //the action test function require 0x1234
         try{
-           await action.genericAction(avatar.address,[0x1234]);
+           await scheme.genericCallDirect.call(avatar.address,actionMock.address,a,b,c);
            assert(false,"generic call should revert if action revert ");
           }
           catch(ex){