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

EVMScripts: check byte lengths, avoid returning unallocated memory, and forward error data #496

Merged
merged 31 commits into from Apr 5, 2019
Merged
Changes from 24 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
1b99fc9
EVMScriptRegistry: check script bytes
sohkai Mar 20, 2019
a6479ff
CallsScript: add script length check before access address and callda…
sohkai Mar 20, 2019
cce5b25
EVMScriptRunner: always allocate new memory for decoding return
sohkai Mar 20, 2019
1f5e488
cosmetic: fix comment spacing
sohkai Mar 20, 2019
f955b3d
EVMScriptRunner: fix copying of script's return data
sohkai Mar 26, 2019
3dcbf29
Update test/evm_script.js
facuspagnuolo Mar 26, 2019
c8438f2
cosmetic(test): clarify encodeCallScript's underflow generators
sohkai Mar 26, 2019
ae703f0
Revert "EVMScriptRunner: fix copying of script's return data"
sohkai Apr 2, 2019
8f4783c
CallsScript: cosmetic improvements
sohkai Apr 2, 2019
f73c45a
EVMScriptRunner: add comments for ABI decoding return data
sohkai Apr 2, 2019
335c8d3
CallsScript: clarify why it can only be accessed via a delegatecall
sohkai Apr 2, 2019
80d66bb
test: rename mock executor app to be an app stub
sohkai Apr 3, 2019
2c2b42e
test: EVMScripts cosmetic restructuring
sohkai Apr 3, 2019
e61eda3
test: cosmetic whitespace changes for evmscript tests
sohkai Apr 3, 2019
504da47
test: use new assertThrow module for evmscript tests
sohkai Apr 3, 2019
9a24aa7
test: add revert strings helpers
sohkai Apr 3, 2019
159bb8a
EVMScriptRegistry: reorder error string declarations
sohkai Apr 3, 2019
add1df4
test: add revert string tests to evmscript
sohkai Apr 3, 2019
e054867
test: restructure evmscript tests to separate tests for EVMScriptRegi…
sohkai Apr 3, 2019
4c911b0
Merge branch 'dev' into evmscript-bytes
sohkai Apr 3, 2019
c228edf
test: cosmetic reordering of some evmscript tests
sohkai Apr 3, 2019
caa7383
test: move creation of executionTarget in evmscripts tests to only wh…
sohkai Apr 3, 2019
b5c8731
test: rename executor app to script runner app to avoid confusion in …
sohkai Apr 3, 2019
cecb700
test: test returned bytes from evmscript runners
sohkai Apr 3, 2019
175f772
test: skip revert error checking on coverage
sohkai Apr 3, 2019
99f9865
EVMScriptRunner, CallsScript: transparently revert errors rather than…
sohkai Apr 4, 2019
594329c
CallsScript: use default revert string if call errored without any er…
sohkai Apr 4, 2019
27b0bbb
Update contracts/evmscript/executors/CallsScript.sol
izqui Apr 5, 2019
6164583
CallsScript: clarify manipulation of memory for manual error message
sohkai Apr 5, 2019
6376395
EVMScriptRunner: optimize free memory pointer placement after executo…
sohkai Apr 5, 2019
5706132
EVMScriptRunner: remove comment for extract bytes as it's not relevan…
sohkai Apr 5, 2019
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+665 −319
Diff settings

Always

Just for now

@@ -19,9 +19,12 @@ contract EVMScriptRegistry is IEVMScriptRegistry, EVMScriptRegistryConstants, Ar
// WARN: Manager can censor all votes and the like happening in an org

This comment has been minimized.

Copy link
@sohkai

sohkai Mar 20, 2019

Author Member

Note that upgrading this app for existing organizations will be a PITA; we've pinned these apps to their current implementations and so these orgs will have to reset their default EVMScriptRegistry app in the kernel to get this change (along with uninstalling the old one completely, for the frontend to not look weird).

bytes32 public constant REGISTRY_MANAGER_ROLE = 0xf7a450ef335e1892cb42c8ca72e7242359d7711924b75db5717410da3f614aa3;

uint256 internal constant SCRIPT_START_LOCATION = 4;

string private constant ERROR_INEXISTENT_EXECUTOR = "EVMREG_INEXISTENT_EXECUTOR";
string private constant ERROR_EXECUTOR_ENABLED = "EVMREG_EXECUTOR_ENABLED";
string private constant ERROR_EXECUTOR_DISABLED = "EVMREG_EXECUTOR_DISABLED";
string private constant ERROR_SCRIPT_LENGTH_TOO_SHORT = "EVMREG_SCRIPT_LENGTH_TOO_SHORT";

struct ExecutorEntry {
IEVMScriptExecutor executor;
@@ -96,6 +99,7 @@ contract EVMScriptRegistry is IEVMScriptRegistry, EVMScriptRegistryConstants, Ar
* @param _script EVMScript being inspected
*/
function getScriptExecutor(bytes _script) public view returns (IEVMScriptExecutor) {
require(_script.length >= SCRIPT_START_LOCATION, ERROR_SCRIPT_LENGTH_TOO_SHORT);
uint256 id = _script.getSpecId();

// Note that we don't need to check for an executor's existence in this case, as only
@@ -50,17 +50,25 @@ contract EVMScriptRunner is AppStorage, Initializable, EVMScriptRegistryConstant
}

/**
* @dev copies and returns last's call data. Needs to ABI decode first
* @dev Copies and returns last call's data.
* Needs to perform an ABI decode for the expected `bytes` return type of
* `executor.execScript()` as solidity will automatically ABI encode the returned bytes as:
* [ position of the first dynamic length return value = 0x20 (32 bytes) ]
* [ output length (32 bytes) ]
* [ output content (N bytes) ]
*/
function returnedDataDecoded() internal pure returns (bytes ret) {
function returnedDataDecoded() internal pure returns (bytes) {
bytes memory ret;
assembly {
let size := returndatasize
switch size
ret := mload(0x40) // free mem ptr get
mstore(0x40, add(ret, add(returndatasize, 0x20))) // free mem ptr set

switch returndatasize
case 0 {}
default {
ret := mload(0x40) // free mem ptr get
mstore(0x40, add(ret, add(size, 0x20))) // free mem ptr set
returndatacopy(ret, 0x20, sub(size, 0x20)) // copy return data
// Copy return data
// Apply ABI decode by ignoring the first 32 bytes of the return data
returndatacopy(ret, 0x20, sub(returndatasize, 0x20))

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 3, 2019

Author Member

@facuspagnuolo Confirmed using returndatasize is much cheaper; diffing f73c45a against its previous commit results in 4 less opcodes :).

}
}
return ret;
@@ -25,14 +25,17 @@ contract CallsScript is BaseEVMScriptExecutor {
* @param _script [ specId (uint32) ] many calls with this structure ->

This comment has been minimized.

Copy link
@sohkai

sohkai Mar 20, 2019

Author Member

Note that upgrading this is also a PITA, for similar reasons to EVMScriptRegistry.

* [ to (address: 20 bytes) ] [ calldataLength (uint32: 4 bytes) ] [ calldata (calldataLength bytes) ]
* @param _blacklist Addresses the script cannot call to, or will revert.
* @return always returns empty byte array
* @return Always returns empty byte array
*/
function execScript(bytes _script, bytes, address[] _blacklist) external isInitialized returns (bytes) {
uint256 location = SCRIPT_START_LOCATION; // first 32 bits are spec id
while (location < _script.length) {
// Check there's at least address + calldataLength available
require(_script.length - location >= 0x18, ERROR_INVALID_LENGTH);

address contractAddress = _script.addressAt(location);
// Check address being called is not blacklist
for (uint i = 0; i < _blacklist.length; i++) {
for (uint256 i = 0; i < _blacklist.length; i++) {
require(contractAddress != _blacklist[i], ERROR_BLACKLISTED_CALL);
}

@@ -50,11 +53,21 @@ contract CallsScript is BaseEVMScriptExecutor {

bool success;
assembly {
success := call(sub(gas, 5000), contractAddress, 0, calldataStart, calldataLength, 0, 0)
success := call(
sub(gas, 5000), // forward gas left - 5000
contractAddress, // address
0, // no value
calldataStart, // calldata start
calldataLength, // calldata length
0, // don't write output
0 // don't write output
)
}

require(success, ERROR_CALL_REVERTED);
}
// No need to allocate empty bytes for the return as this can only be called via an delegatecall
This conversation was marked as resolved by facuspagnuolo

This comment has been minimized.

Copy link
@sohkai

sohkai Mar 20, 2019

Author Member

Given the modifier checks this has, it's actually impossible to call this without going through a delegatecall.

This comment has been minimized.

Copy link
@sohkai

sohkai Mar 25, 2019

Author Member

isInitialized checks against the slot at INITIALIZATION_BLOCK_POSITION, which is left false in this executor contract (executors are deployed as-is).

We added isInitialized in an attempt to make calling these scripts harder to do outside of Aragon apps (before anyone could make a call to them), so this protects against:

  • Direct calls (imagine if this script changed some state; we would only want to do so if it was delegatecalled into)
  • Usage from uninitialized (or "petrified") Aragon apps

You could still make a contract that sets the storage slot and delegatecalls into this executor.

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Mar 25, 2019

Contributor

Thanks for clarifying :)

// (due to the isInitialized modifier)
}

function executorType() external pure returns (bytes32) {
@@ -3,21 +3,21 @@ pragma solidity 0.4.24;
import "../../apps/AragonApp.sol";


contract MockScriptExecutorApp is AragonApp {
contract AppStubScriptRunner is AragonApp {
// Initialization is required to access any of the real executors
function initialize() public {
initialized();
}

function execute(bytes script) public {
runScript(script, new bytes(0), new address[](0));
function runScript(bytes script) public returns (bytes) {
return runScript(script, new bytes(0), new address[](0));
}

function executeWithBan(bytes script, address[] memory blacklist) public {
runScript(script, new bytes(0), blacklist);
function runScriptWithBan(bytes script, address[] memory blacklist) public returns (bytes) {
return runScript(script, new bytes(0), blacklist);
}

function executeWithIO(bytes script, bytes input, address[] memory blacklist) public returns (bytes) {
function runScriptWithIO(bytes script, bytes input, address[] memory blacklist) public returns (bytes) {
return runScript(script, input, blacklist);
}

@@ -6,7 +6,12 @@ import "../../evmscript/executors/BaseEVMScriptExecutor.sol";
contract EVMScriptExecutorMock is BaseEVMScriptExecutor {
bytes32 internal constant EXECUTOR_TYPE = keccak256("MOCK_SCRIPT");

function execScript(bytes, bytes, address[]) external isInitialized returns (bytes) {
function execScript(bytes _script, bytes, address[]) external isInitialized returns (bytes) {
// Return full input script if it's more than just the spec ID, otherwise return an empty
// bytes array
if (_script.length > SCRIPT_START_LOCATION) {
return _script;
}
}

function executorType() external pure returns (bytes32) {
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.