Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Agent app: EVM #580

Merged
merged 45 commits into from
Feb 15, 2019
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
0d01aaf
Actor app: inherit Vault and test vault functionality
izqui Nov 15, 2018
1fa6386
Actor app: implement execute action
izqui Nov 16, 2018
7a4dd5b
Actor app: implement permissioned forwarding
izqui Nov 16, 2018
9830981
Actor app: improve tests
izqui Nov 16, 2018
7457eb3
Actor app: signatures
izqui Nov 19, 2018
af478c1
CI: travis wait on npm install
izqui Nov 19, 2018
ebb10aa
Actor app: lint and enable CI
izqui Nov 19, 2018
e132297
Add max gas for static calls
izqui Nov 26, 2018
3ad9920
chore: upgrade to web3-eth-abi@1.0.0-beta.38
sohkai Jan 26, 2019
ea9a5aa
Address review comments
izqui Jan 26, 2019
8e40798
Lint
izqui Feb 4, 2019
2aaf462
test: add comments, cosmetic changes, and more tests
sohkai Feb 7, 2019
c73d999
SignatureValidator: fix compile warnings
sohkai Feb 7, 2019
5f1291f
test: add context headers
sohkai Feb 7, 2019
f17a0f5
Metadata: add roles to arapp, remove webapp artifacts
izqui Feb 11, 2019
f6f66f7
Radspec strings
izqui Feb 11, 2019
eb95db8
Travis: allow actor coverage to fail, tracking fix in #658
izqui Feb 11, 2019
abf1c91
Prevent infinite loop by prohibiting setting itself as the designated…
izqui Feb 11, 2019
a10c62c
Radspec
izqui Feb 11, 2019
cc21b67
Actor -> Agent rename
izqui Feb 11, 2019
139ad8a
Add status to arapp and rename constants
izqui Feb 12, 2019
fe86092
Update ERC1271 implementation
izqui Feb 12, 2019
1849286
Add ERC1271 interfaceId
izqui Feb 12, 2019
1b7b735
Rename constants
izqui Feb 12, 2019
d960fee
Rename calldata
izqui Feb 12, 2019
39b6b7e
Refactor SignatureValidator.sol
izqui Feb 12, 2019
de9d4a6
Refactor signature validation tests and test EIP712 signatures
izqui Feb 12, 2019
474f0c1
Handle empty signatures and undefined signature modes
izqui Feb 12, 2019
345a9d8
Refactor SignatureValidator: handle ERC1271 checks in library
izqui Feb 13, 2019
499cdcb
Test ERC1271 signature wrapping
izqui Feb 13, 2019
1b76fb9
cosmetic: fix whitespace
sohkai Feb 13, 2019
98369d1
Merge branch 'next' into actor-app
sohkai Feb 13, 2019
a0bddef
Agent: always allow execute to transfer ETH (#651)
sohkai Feb 13, 2019
e8eb268
cosmetic: fix whitespace and add EIPs hash
sohkai Feb 13, 2019
2225408
chore: upgrade aragonOS and use test-helper's contracts
sohkai Feb 13, 2019
2ed4a2a
cosmetic: fix compile errors
sohkai Feb 13, 2019
c482981
cosmetic: fix linter
sohkai Feb 13, 2019
cd9614b
chore: fix travis allowed to fail matrix
sohkai Feb 13, 2019
970431a
test: ignore all test contracts from coverage
sohkai Feb 13, 2019
cff0689
Actor: reuse Vault tests (#668)
sohkai Feb 14, 2019
905c32d
Move agent from future-apps/ to apps/
izqui Feb 15, 2019
0d3e2ec
Refactor popFirstByte to copy to a new array
izqui Feb 15, 2019
5b7a5fe
Address last review comments
izqui Feb 15, 2019
2a293d1
Lint
izqui Feb 15, 2019
687e29d
Address very last review comments
izqui Feb 15, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,19 @@ env:
- TASK=test:token-manager:app
- TASK=test:vault
- TASK=test:voting
- TASK=test:actor
- TASK=coverage:finance
- TASK=coverage:survey
- TASK=coverage:token-manager
- TASK=coverage:vault
- TASK=coverage:voting
- TASK=coverage:actor
- TASK=test:shared
matrix:
allow_failures:
- env: TASK=coverage:finance
install:
- travis_wait 60 npm install
before_script:
- npm prune
script:
Expand Down
9 changes: 9 additions & 0 deletions future-apps/actor/.solcover.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module.exports = {
norpc: true,
copyPackages: ['@aragon/os', '@aragon/apps-vault'],
skipFiles: [
'test/TestImports.sol',
'test/mocks/ExecutionTarget.sol',
'test/mocks/FinanceMock.sol',
]
}
1 change: 1 addition & 0 deletions future-apps/actor/.soliumignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
contracts/test
23 changes: 23 additions & 0 deletions future-apps/actor/.soliumrc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{
"extends": "solium:all",
"rules": {
"imports-on-top": ["error"],
"variable-declarations": ["error"],
"array-declarations": ["error"],
"operator-whitespace": ["error"],
"lbrace": ["error"],
"mixedcase": 0,
"camelcase": ["error"],
"uppercase": 0,
"no-empty-blocks": ["error"],
"no-unused-vars": ["error"],
"quotes": ["error"],
"indentation": 0,
"whitespace": ["error"],
"deprecated-suicide": ["error"],
"arg-overflow": ["error", 8],
"pragma-on-top": ["error"],
"security/enforce-explicit-visibility": ["error"],
"error-reason": 0
}
}
33 changes: 33 additions & 0 deletions future-apps/actor/arapp.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
{
"environments": {
"default": {
"registry": "0x5f6f7e8cc7346a11ca2def8f827b7a0b612c56a1",
"appName": "actor.aragonpm.eth",
"network": "rpc"
},
"rinkeby": {
"registry": "0x98Df287B6C145399Aaa709692c8D308357bC085D",
"appName": "actor.aragonpm.eth",
"network": "rinkeby"
},
"staging": {
"registry": "0xfe03625ea880a8cba336f9b5ad6e15b0a3b5a939",
"appName": "actor.aragonpm.eth",
"network": "rinkeby"
},
"mainnet": {
"registry": "0x314159265dd8dbb310642f98f50c066173c1259b",
"appName": "actor.aragonpm.eth",
"network": "mainnet"
},
"rinkeby-old": {
"registry": "0xfbae32d1cde62858bc45f51efc8cc4fa1415447e",
"appName": "actor.aragonpm.eth",
"network": "rinkeby"
}
},
"roles": [
izqui marked this conversation as resolved.
Show resolved Hide resolved
TODO
],
"path": "contracts/Actor.sol"
}
1 change: 1 addition & 0 deletions future-apps/actor/contracts/.npmignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/test
184 changes: 184 additions & 0 deletions future-apps/actor/contracts/Actor.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
/*
* SPDX-License-Identitifer: GPL-3.0-or-later
*/

pragma solidity 0.4.24;

import "./SignatureValidator.sol";
import "./standards/IERC165.sol";
import "./standards/IERC1271.sol";

import "@aragon/apps-vault/contracts/Vault.sol";

import "@aragon/os/contracts/common/IForwarder.sol";


contract Actor is Vault, IERC165, IERC1271, IForwarder {
izqui marked this conversation as resolved.
Show resolved Hide resolved
bytes32 public constant EXECUTE_ROLE = keccak256("EXECUTE_ROLE");
bytes32 public constant RUN_SCRIPT_ROLE = keccak256("RUN_SCRIPT_ROLE");
bytes32 public constant PRESIGN_HASH_ROLE = keccak256("PRESIGN_HASH_ROLE");
izqui marked this conversation as resolved.
Show resolved Hide resolved
bytes32 public constant DESIGNATE_SIGNER_ROLE = keccak256("DESIGNATE_SIGNER_ROLE");

bytes4 public constant ISVALIDSIG_INTERFACE_ID = 0xabababab; // TODO: Add actual interfaceId

string private constant ERROR_EXECUTE_ETH_NO_DATA = "VAULT_EXECUTE_ETH_NO_DATA";
izqui marked this conversation as resolved.
Show resolved Hide resolved
string private constant ERROR_EXECUTE_TARGET_NOT_CONTRACT = "VAULT_EXECUTE_TARGET_NOT_CONTRACT";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is 33 chars long, I seem to recall we were using up to 32 to avoid using extra storage.


uint256 internal constant STATICCALL_MAX_GAS = 50000;
sohkai marked this conversation as resolved.
Show resolved Hide resolved

mapping (bytes32 => bool) public isPresigned;
address public designatedSigner;

event Execute(address indexed sender, address indexed target, uint256 ethValue, bytes data);
event PresignHash(address indexed sender, bytes32 indexed hash);
event SetDesignatedSigner(address indexed sender, address indexed oldSigner, address indexed newSigner);

// TODO: requires the @decodeData helper to be implemented in radspec
/**
* @notice Execute `@decodeData(target, data)` on `target` `ethValue == 0 ? '' : '(Sending' + @formatToken(ethValue, ETH) + ')'`.
izqui marked this conversation as resolved.
Show resolved Hide resolved
* @param _target Address where the action is being executed
* @param _ethValue Amount of ETH from the contract that is sent with the action
* @param _data Calldata for the action
* @return Exits call frame forwarding the return data of the executed call (either error or success data)
*/
function execute(address _target, uint256 _ethValue, bytes _data)
external // This function MUST always be external as the function performs a low level return, exiting the Actor app execution context
authP(EXECUTE_ROLE, arr(_target, _ethValue, uint256(getSig(_data)))) // TODO: Test that sig bytes are the least significant bytes
{
require(_ethValue == 0 || _data.length > 0, ERROR_EXECUTE_ETH_NO_DATA); // if ETH value is sent, there must be data
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this correctly, allowing no data to be passed is to explicitly allow this contract to call a fallback function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, although I am considering removing this

require(isContract(_target), ERROR_EXECUTE_TARGET_NOT_CONTRACT);
Copy link

@jvluso jvluso Jan 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we don't want the actor app to be able to send eth to someone without calling a function? Anyone able to call the execute function will be able to send funds anyway, and this makes compatibility with external dapps slightly more difficult.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to Actor inheriting from Vault, that can be done using the transfer function. I just tried to separate role concerns here.


bool result = _target.call.value(_ethValue)(_data);

if (result) {
emit Execute(msg.sender, _target, _ethValue, _data);
}

assembly {
let size := returndatasize
let ptr := mload(0x40)
returndatacopy(ptr, 0, size)

// revert instead of invalid() bc if the underlying call failed with invalid() it already wasted gas.
// if the call returned error data, forward it
switch result case 0 { revert(ptr, size) }
default { return(ptr, size) }
}
}

function setDesignatedSigner(address _designatedSigner)
external
authP(DESIGNATE_SIGNER_ROLE, arr(_designatedSigner))
{
address oldDesignatedSigner = designatedSigner;
designatedSigner = _designatedSigner;

emit SetDesignatedSigner(msg.sender, oldDesignatedSigner, _designatedSigner);
}

function presignHash(bytes32 _hash)
external
authP(PRESIGN_HASH_ROLE, arr(_hash))
{
isPresigned[_hash] = true;
sohkai marked this conversation as resolved.
Show resolved Hide resolved

emit PresignHash(msg.sender, _hash);
}

function isForwarder() external pure returns (bool) {
return true;
}

function supportsInterface(bytes4 interfaceId) external pure returns (bool) {
return interfaceId == ISVALIDSIG_INTERFACE_ID;
izqui marked this conversation as resolved.
Show resolved Hide resolved
}

function forward(bytes _evmScript)
public
authP(RUN_SCRIPT_ROLE, arr(getScriptACLParam(_evmScript)))
{
bytes memory input = ""; // no input
address[] memory blacklist = new address[](0); // no addr blacklist, can interact with anything
runScript(_evmScript, input, blacklist);
// We don't need to emit an event here as EVMScriptRunner will emit ScriptResult if successful
izqui marked this conversation as resolved.
Show resolved Hide resolved
}

function isValidSignature(bytes data, bytes signature) public view returns (bool) {
return isValidSignature(keccak256(data), signature);
}

function isValidSignature(bytes32 hash, bytes signature) public view returns (bool) {
// Short-circuit in case the hash was presigned. Optimization as performing calls
// and ecrecover is more expensive than an SLOAD.
if (isPresigned[hash]) {
return true;
}

// Checks if designatedSigner is a contract, and if it supports the isValidSignature interface
if (safeSupportsInterface(IERC165(designatedSigner), ISVALIDSIG_INTERFACE_ID)) {
// designatedSigner.isValidSignature(hash, signature) as a staticall
IERC1271 signerContract = IERC1271(designatedSigner);
bytes memory calldata = abi.encodeWithSelector(signerContract.isValidSignature.selector, hash, signature);
return safeBoolStaticCall(signerContract, calldata);
}

// `safeSupportsInterface` returns false if designatedSigner is a contract but it
// doesn't support the interface. Here we check the validity of the ECDSA sig
// which will always fail if designatedSigner is not an EOA

return SignatureValidator.isValidSignature(hash, designatedSigner, signature);
}

function canForward(address sender, bytes evmScript) public view returns (bool) {
uint256[] memory params = new uint256[](1);
params[0] = getScriptACLParam(evmScript);
return canPerform(sender, RUN_SCRIPT_ROLE, params);
}

function safeSupportsInterface(IERC165 target, bytes4 interfaceId) internal view returns (bool) {
if (!isContract(target)) {
return false;
}

bytes memory calldata = abi.encodeWithSelector(target.supportsInterface.selector, interfaceId);
return safeBoolStaticCall(target, calldata);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EIP165 says we should make two more calls (to be extra, extra safe): algorithm, sample implementation.

Also specifies gas limit to be 30k (safeBoolStaticCall() could add a gas limit argument).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know about the two first calls, it really seems like a bit of an overkill to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the gas limit though

}

function safeBoolStaticCall(address target, bytes calldata) internal view returns (bool) {
uint256 gasLeft = gasleft();

uint256 callGas = gasLeft > STATICCALL_MAX_GAS ? STATICCALL_MAX_GAS : gasLeft;
bool ok;
assembly {
ok := staticcall(callGas, target, add(calldata, 0x20), mload(calldata), 0, 0)
}

if (!ok) {
return false;
}

uint256 size;
assembly { size := returndatasize }
if (size != 32) {
return false;
}

bool result;
assembly {
let ptr := mload(0x40) // get next free memory ptr
returndatacopy(ptr, 0, size) // copy return from above `staticcall`
result := mload(ptr) // read data at ptr and set it to result
mstore(ptr, 0) // set pointer memory to 0 so it still is the next free ptr
izqui marked this conversation as resolved.
Show resolved Hide resolved
}

return result;
}

function getScriptACLParam(bytes evmScript) internal pure returns (uint256) {
return uint256(keccak256(abi.encodePacked(evmScript)));
}

function getSig(bytes data) internal pure returns (bytes4 sig) {
assembly { sig := add(data, 0x20) }
}
}
43 changes: 43 additions & 0 deletions future-apps/actor/contracts/SignatureValidator.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
pragma solidity ^0.4.18;
izqui marked this conversation as resolved.
Show resolved Hide resolved

// From github.com/DexyProject/protocol
// This should probably be moved into aOS: https://github.com/aragon/aragonOS/pull/442


library SignatureValidator {
// Maybe use 0x's IDs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think this makes sense, especially in regards to their ILLEGAL and INVALID types. We should also check the length using the same enum trick they do.

enum SignatureMode {
EIP712,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there something special we need to do for EIP 712 contracts? This contract doesn't seem to implement a case for this.

Copy link
Contributor Author

@izqui izqui Jan 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does implement the case, by not modifying the hash as it does for the other two modes

GETH,
TREZOR
}

/// @dev Validates that a hash was signed by a specified signer.
/// @param hash Hash which was signed.
/// @param signer Address of the signer.
/// @param signature ECDSA signature along with the mode (0 = EIP712, 1 = Geth, 2 = Trezor) {mode}{v}{r}{s}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we should order it as {r}{s}{v} to be more compatible with existing tools that use this order? I agree mode should stay at the front given the possible variable length of {v}.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion on this, but given that the mode needs to be prefixed anyway, special tooling will be needed regardless.

/// @return Returns whether signature is from a specified user.
function isValidSignature(bytes32 hash, address signer, bytes signature) internal pure returns (bool) {
if (signature.length != 66) {
return false;
}

SignatureMode mode = SignatureMode(uint8(signature[0]));

uint8 v = uint8(signature[1]);
bytes32 r;
bytes32 s;
assembly {
r := mload(add(signature, 34))
s := mload(add(signature, 66))
}
izqui marked this conversation as resolved.
Show resolved Hide resolved

if (mode == SignatureMode.GETH) {
hash = keccak256("\x19Ethereum Signed Message:\n32", hash);
} else if (mode == SignatureMode.TREZOR) {
Copy link
Contributor

@sohkai sohkai Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that Trezor implements the same signing prefix as geth / sign_eth now: ethereum/go-ethereum#14794 (comment)

hash = keccak256("\x19Ethereum Signed Message:\n\x20", hash);
}

return ecrecover(hash, v, r, s) == signer;
}
}
7 changes: 7 additions & 0 deletions future-apps/actor/contracts/standards/IERC1271.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
pragma solidity 0.4.24;


interface IERC1271 {
// TODO: Standard is not finalized, discussion: https://github.com/ethereum/EIPs/issues/1271
function isValidSignature(bytes32 hash, bytes signature) public view returns (bool);
}
6 changes: 6 additions & 0 deletions future-apps/actor/contracts/standards/IERC165.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pragma solidity 0.4.24;


interface IERC165 {
function supportsInterface(bytes4 interfaceId) external pure returns (bool);
}
28 changes: 28 additions & 0 deletions future-apps/actor/contracts/test/TestImports.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
pragma solidity 0.4.24;

import "@aragon/os/contracts/acl/ACL.sol";
import "@aragon/os/contracts/apps/AppProxyBase.sol";
import "@aragon/os/contracts/factory/DAOFactory.sol";
import "@aragon/os/contracts/kernel/Kernel.sol";
import "@aragon/os/contracts/kernel/KernelProxy.sol";

import "@aragon/apps-shared-migrations/contracts/Migrations.sol";

// You might think this file is a bit odd, but let me explain.
// We only use some contracts in our tests, which means Truffle
// will not compile it for us, because it is from an external
// dependency.
//
// We are now left with three options:
// - Copy/paste these contracts
// - Run the tests with `truffle compile --all` on
// - Or trick Truffle by claiming we use it in a Solidity test
//
// You know which one I went for.


contract TestImports {
constructor() public {
// to avoid lint error
}
}
Loading