From 49bb4e3fe43aeb8ab7e5abdbea0137cde420b4c5 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Mon, 25 Mar 2019 18:18:13 +0100 Subject: [PATCH 1/3] ACL: optimize checkOracle() --- contracts/acl/ACL.sol | 48 +++++++++++++++------------ contracts/common/SafeERC20.sol | 4 +-- contracts/test/TestACLInterpreter.sol | 1 + contracts/test/helpers/ACLHelper.sol | 12 +++++++ 4 files changed, 42 insertions(+), 23 deletions(-) diff --git a/contracts/acl/ACL.sol b/contracts/acl/ACL.sol index 032f556fb..2d2ff1d20 100644 --- a/contracts/acl/ACL.sol +++ b/contracts/acl/ACL.sol @@ -419,31 +419,37 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { function checkOracle(IACLOracle _oracleAddr, address _who, address _where, bytes32 _what, uint256[] _how) internal view returns (bool) { bytes4 sig = _oracleAddr.canPerform.selector; - // a raw call is required so we can return false if the call reverts, rather than reverting bytes memory checkCalldata = abi.encodeWithSelector(sig, _who, _where, _what, _how); uint256 oracleCheckGas = ORACLE_CHECK_GAS; - - bool ok; - assembly { - ok := staticcall(oracleCheckGas, _oracleAddr, add(checkCalldata, 0x20), mload(checkCalldata), 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 + let ptr := mload(0x40) // free memory pointer + + // A raw staticcall is required so we can return false if the call reverts, rather than reverting + let success := staticcall( + oracleCheckGas, // gas forwarded + _oracleAddr, // address + add(checkCalldata, 0x20), // calldata start + mload(checkCalldata), // calldata length + ptr, // write output over free memory + 0x20 // uint256 return + ) + + if gt(success, 0) { + // Check number of bytes returned from last function call + switch returndatasize + + // 32 bytes returned: check if true + case 0x20 { + // Only return success if returned data was true + // Already have output in ptr + result := eq(mload(ptr), 1) + } + + // Not sure what was returned: don't mark as success + default { } + } } return result; diff --git a/contracts/common/SafeERC20.sol b/contracts/common/SafeERC20.sol index fa11c0921..d3fe3a948 100644 --- a/contracts/common/SafeERC20.sol +++ b/contracts/common/SafeERC20.sol @@ -29,7 +29,7 @@ library SafeERC20 { add(_calldata, 0x20), // calldata start mload(_calldata), // calldata length ptr, // write output over free memory - 0x20 // uint256 return + 0x20 // write 32 bytes ) if gt(success, 0) { @@ -41,7 +41,7 @@ library SafeERC20 { ret := 1 } - // 32 bytes returned: check if non-zero + // 32 bytes returned: check if true case 0x20 { // Only return success if returned data was true // Already have output in ptr diff --git a/contracts/test/TestACLInterpreter.sol b/contracts/test/TestACLInterpreter.sol index ca909e91e..20a641d03 100644 --- a/contracts/test/TestACLInterpreter.sol +++ b/contracts/test/TestACLInterpreter.sol @@ -86,6 +86,7 @@ contract TestACLInterpreter is ACL, ACLHelper { assertEval(arr(), ORACLE_PARAM_ID, Op.EQ, uint256(new StateModifyingOracle()), false); // if returned data size is not correct, returns false assertEval(arr(), ORACLE_PARAM_ID, Op.EQ, uint256(new EmptyDataReturnOracle()), false); + assertEval(arr(), ORACLE_PARAM_ID, Op.EQ, uint256(new LargeDataReturnOracle()), false); // conditional oracle returns true if first param > 0 ConditionalOracle conditionalOracle = new ConditionalOracle(); diff --git a/contracts/test/helpers/ACLHelper.sol b/contracts/test/helpers/ACLHelper.sol index 404978572..55bcb584f 100644 --- a/contracts/test/helpers/ACLHelper.sol +++ b/contracts/test/helpers/ACLHelper.sol @@ -52,6 +52,18 @@ contract EmptyDataReturnOracle is IACLOracle { } } +contract LargeDataReturnOracle is IACLOracle { + function canPerform(address, address, bytes32, uint256[]) external view returns (bool) { + uint256[] memory largeData = new uint256[](2); + largeData[0] = 1; + largeData[1] = 2; + assembly { + // Return two uint256s + return(largeData, 0x40) + } + } +} + contract ConditionalOracle is IACLOracle { function canPerform(address, address, bytes32, uint256[] how) external view returns (bool) { return how[0] > 0; From 25d08da3c377b2bc801e85e887f9052b02b2f526 Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Wed, 27 Mar 2019 16:56:23 +0100 Subject: [PATCH 2/3] Update contracts/acl/ACL.sol Co-Authored-By: sohkai --- contracts/acl/ACL.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/acl/ACL.sol b/contracts/acl/ACL.sol index 2d2ff1d20..7f217fc89 100644 --- a/contracts/acl/ACL.sol +++ b/contracts/acl/ACL.sol @@ -437,7 +437,7 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { ) if gt(success, 0) { - // Check number of bytes returned from last function call + // Check number of bytes returned from last external call switch returndatasize // 32 bytes returned: check if true From ea30416f50028e8839bfde5b3132e6bb5c8c7c27 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Thu, 28 Mar 2019 14:04:52 +0100 Subject: [PATCH 3/3] ACL: use assembly switch instead of if --- contracts/acl/ACL.sol | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/contracts/acl/ACL.sol b/contracts/acl/ACL.sol index 7f217fc89..d1dc45433 100644 --- a/contracts/acl/ACL.sol +++ b/contracts/acl/ACL.sol @@ -427,7 +427,7 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { let ptr := mload(0x40) // free memory pointer // A raw staticcall is required so we can return false if the call reverts, rather than reverting - let success := staticcall( + result := staticcall( oracleCheckGas, // gas forwarded _oracleAddr, // address add(checkCalldata, 0x20), // calldata start @@ -436,7 +436,9 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { 0x20 // uint256 return ) - if gt(success, 0) { + // solidity-coverage fails on assembly `if` + switch result case 0 { } + default { // Check number of bytes returned from last external call switch returndatasize