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

Conversation

Projects
None yet
4 participants
@sohkai
Copy link
Member

sohkai commented Mar 20, 2019

Add length checks to bytes passed to EVMScriptRegistry and CallsScript and makes sure EVMScriptRunner doesn't return an unallocated bytes array if no return data is received from execScript().

Also transparently forwards any error data if a script executor or call script failed.

Bytecode diff (after removing the error strings):

                              CODE DEPOSIT COST    DEPLOYED BYTES     INITIALIZATION BYTES
CallsScript.json              30400 more gas       +152               0
EVMScriptExecutorMock.json    13200 more gas       +66                0
EVMScriptRegistry.json        3400 more gas        +17                0
EVMScriptRegistryFactory.json Same                 0                  +169
ExecutionTarget.json          84600 more gas       +423               0

sohkai added some commits Mar 20, 2019

@sohkai sohkai added the security label Mar 20, 2019

@sohkai sohkai requested review from bingen, facuspagnuolo and izqui and removed request for bingen Mar 20, 2019

@@ -19,8 +19,11 @@ 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).

@@ -25,11 +25,14 @@ 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.

@facuspagnuolo
Copy link
Contributor

facuspagnuolo left a comment

LGTM!

Show resolved Hide resolved contracts/evmscript/EVMScriptRunner.sol Outdated
Show resolved Hide resolved test/evm_script.js Outdated
Show resolved Hide resolved test/evm_script.js Outdated
@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 20, 2019

Coverage Status

Coverage decreased (-0.0007%) to 99.54% when pulling 6376395 on evmscript-bytes into d7e79ea on dev.

sohkai and others added some commits Mar 26, 2019

Update test/evm_script.js
Co-Authored-By: sohkai <qisheng.brett.sun@gmail.com>

@sohkai sohkai force-pushed the evmscript-bytes branch from ed41553 to c228edf Apr 3, 2019

sohkai added some commits Apr 3, 2019

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 :).

@@ -1,26 +1,38 @@
function assertError(error, s, message) {

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 3, 2019

Author Member

Pulled from aragon/aragon-apps#741.

@sohkai

This comment has been minimized.

Copy link
Member Author

sohkai commented Apr 3, 2019

Added tests for the returned bytes in cecb700.

Polished the tests:

  • Separated the EVMScriptRegistryFactory() usage into its own test
  • Uses the new assertThrows module from aragon/aragon-apps#741
  • Some other bits of clean up or renaming to clarify the existing tests

@sohkai sohkai force-pushed the evmscript-bytes branch from c234a8a to 175f772 Apr 3, 2019

sohkai added some commits Apr 4, 2019

@sohkai sohkai changed the title EVMScript: check byte lengths and fix returning unallocated memory EVMScripts: check byte lengths, avoid returning unallocated memory, and forward return data Apr 4, 2019

@sohkai sohkai changed the title EVMScripts: check byte lengths, avoid returning unallocated memory, and forward return data EVMScripts: check byte lengths, avoid returning unallocated memory, and forward error data Apr 4, 2019

@izqui

izqui approved these changes Apr 4, 2019

return output;
}
output := mload(0x40) // free mem ptr get
mstore(0x40, add(output, add(returndatasize, 0x20))) // free mem ptr set

This comment has been minimized.

Copy link
@izqui

izqui Apr 4, 2019

Member

This was already in the code, but I think we are allocating 32 bytes more of memory than we need when the call is successful, as we only copy returndatasize - 0x20 bytes.

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Apr 5, 2019

Contributor

Agree, and I think it actually makes sense only to update it when the call succeeds.

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 5, 2019

Author Member

Actually it turns out we were allocating 64 bytes more:

  • Assume the current free mem ptr is at 0x80
  • returndatasize is 0x60
  • We copy 0x40 (0x60 - 0x20) bits onto 0x80
  • Next free pointer should be 0xc0 (0x80, 0xa0 are taken by the copy), which is 0x80 + 0x40

See 6376395.

Show resolved Hide resolved contracts/evmscript/executors/CallsScript.sol Outdated
@facuspagnuolo
Copy link
Contributor

facuspagnuolo left a comment

LGTM! Left a minor comment

// [ output content (N bytes) ]
//
// Perform the ABI decode by ignoring the first 32 bytes of the return data
returndatacopy(output, 0x20, sub(returndatasize, 0x20))

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Apr 5, 2019

Contributor

Nice that you added this comment here 👏

return output;
}
output := mload(0x40) // free mem ptr get
mstore(0x40, add(output, add(returndatasize, 0x20))) // free mem ptr set

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Apr 5, 2019

Contributor

Agree, and I think it actually makes sense only to update it when the call succeeds.

mstore(add(ptr, 0x40), 0x0000001645564d43414c4c535f43414c4c5f5245564552544544000000000000)
mstore(add(ptr, 0x60), 0)

revert(ptr, 100)

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Apr 5, 2019

Contributor

It also may be clearer using correctly padded data with fixed indexes, it is more readable I think, and it somehow mimics what the EVM actually does.

mstore(ptr, 0x08c379a000000000000000000000000000000000000000000000000000000000)         // error identifier
mstore(add(ptr, 0x04), 0x0000000000000000000000000000000000000000000000000000000000000020) // starting offset
mstore(add(ptr, 0x24), 0x0000000000000000000000000000000000000000000000000000000000000016) // reason length
mstore(add(ptr, 0x44), 0x45564d43414c4c535f43414c4c5f524556455254454400000000000000000000) // reason

izqui and others added some commits Apr 5, 2019

Update contracts/evmscript/executors/CallsScript.sol
Co-Authored-By: sohkai <qisheng.brett.sun@gmail.com>

@sohkai sohkai force-pushed the evmscript-bytes branch from 9a448d4 to 6376395 Apr 5, 2019

let copysize := sub(returndatasize, 0x20)
returndatacopy(output, 0x20, copysize)

mstore(0x40, add(output, copysize)) // free mem ptr set

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 5, 2019

Author Member

Note that this will fail the new tests if sub(copysize, 0x20) is used.

@sohkai sohkai merged commit e8a483b into dev Apr 5, 2019

1 of 4 checks passed

License Compliance FOSSA is analyzing this commit
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
license/cla Contributor License Agreement is signed.
Details

@sohkai sohkai deleted the evmscript-bytes branch Apr 5, 2019

sohkai added a commit that referenced this pull request Apr 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.