-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Lowered Safety Checker Gas #218
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
Merged
Merged
Changes from all commits
Commits
Show all changes
46 commits
Select commit
Hold shift + click to select a range
87cb988
remove seenJump
K-Ho 0303e9b
refactor safetyChecker for new compiler
K-Ho fdd9cea
clean up tests
K-Ho 8e00928
add synthetix safety check tests
K-Ho bf6203c
add Synthetix JSON files
K-Ho 6cf4717
Remove dead code. Do one less mload per loop.
geohot aa99e3b
Simplify if condition
geohot a2bf833
reduce synthetix gas usage to 6214111 and 5832426
geohot 638558a
reduce to 5549638 and 5208992
geohot f290e73
reduce to 5011551 and 4704242
geohot 4bcc0f3
reduce to 4197882 and 3940293
geohot dd41a38
reduce to 3990362 and 3745388
geohot c783592
reduce to 3659856 and 3435162
geohot add75fd
reduce to 3535842 and 3318815
geohot 432387d
reduce to 3101140 and 2913437
geohot bb8247e
reduce to 3075908 and 2889707
geohot 5aab3b6
reduce to 2990322 and 2808585, WINNING
geohot 6932028
cleanup to 2988705 and 2807019
geohot 0a72954
check safety, update EtherCollateral
K-Ho 1f0a912
console.log for failing opcodes in new safety checker
geohot 9db26aa
needed push check in SafetyChecker invalid for libraries. rebuild wit…
geohot 5b32333
fix tabs
geohot b6a97b8
passes in optimized except EtherCollateral
geohot ca7e267
under 3 million
geohot bbb8bda
updated optimized EtherCollateral
K-Ho 23eb63e
i mean, it's broken, but...
geohot 95c228f
2236313
geohot 51950bc
flip stack order
geohot 7e07a99
low gas 1432474 lol
geohot d6041b2
i forget how big 32 bytes is -- 1349924
geohot 4f4401b
call check actually might be safe
geohot a290cb0
cleanup 1323483
geohot 0e61310
updated SWAP1 JSON files
K-Ho 5660cb1
update safety checker for new string and fix test
geohot 28aa44c
add back all tests and console logs
geohot 604464c
remove auxdata
K-Ho 03d3aa0
go slow for now, until whitelist is fixed -- 2065930
geohot 93c7fa0
remove whitelisted opcodes as input, write generation code for it
geohot e38bf13
flip the if, use less gas
geohot 854a532
add comments for readability
geohot eae86af
this actually uses less gas now
geohot 63c5c33
unused var
geohot 002a879
comments, lint
K-Ho d56ed67
clean up tests, add comments, remove unoptimized Synthetix contracts
K-Ho 0549d4a
minor clean up
K-Ho f5cad3c
clean up approach, add comments, tests
K-Ho File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,6 @@ | ||
| pragma solidity ^0.5.0; | ||
| pragma experimental ABIEncoderV2; | ||
|
|
||
| /* Contract Imports */ | ||
| import { ExecutionManager } from "./ExecutionManager.sol"; | ||
|
|
||
| /* Library Imports */ | ||
| import { ContractResolver } from "../utils/resolvers/ContractResolver.sol"; | ||
|
|
||
|
|
@@ -15,45 +12,20 @@ import { ContractResolver } from "../utils/resolvers/ContractResolver.sol"; | |
| * 2. All CALLs are to the Execution Manager and have no value. | ||
| */ | ||
| contract SafetyChecker is ContractResolver { | ||
| /* | ||
| * Contract Variables | ||
| */ | ||
|
|
||
| uint256 public opcodeWhitelistMask; | ||
|
|
||
|
|
||
| /* | ||
| * Constructor | ||
| */ | ||
|
|
||
| /** | ||
| * @param _addressResolver Address of the AddressResolver contract. | ||
| * @param _opcodeWhitelistMask Whitelist mask of allowed opcodes. | ||
| */ | ||
| constructor( | ||
| address _addressResolver, | ||
| uint256 _opcodeWhitelistMask | ||
| address _addressResolver | ||
| ) | ||
| public | ||
| ContractResolver(_addressResolver) | ||
| { | ||
| opcodeWhitelistMask = _opcodeWhitelistMask; | ||
| } | ||
|
|
||
|
|
||
| /* | ||
| * Public Functions | ||
| */ | ||
|
|
||
| /** | ||
| * Returns whether or not all of the provided bytecode is safe. | ||
| * @dev More info on creation vs. runtime bytecode: | ||
| * https://medium.com/authereum/bytecode-and-init-code-and-runtime-code-oh-my-7bcd89065904. | ||
| * @param _bytecode The bytecode to safety check. This can be either | ||
| * creation bytecode (aka initcode) or runtime bytecode | ||
| * (aka cont | ||
| * More info on creation vs. runtime bytecode: | ||
| * https://medium.com/authereum/bytecode-and-init-code-and-runtime-code-oh-my-7bcd89065904ract code). | ||
| * @param _bytecode The bytecode to safety check. | ||
| * @return `true` if the bytecode is safe, `false` otherwise. | ||
| */ | ||
| function isBytecodeSafe( | ||
|
|
@@ -63,136 +35,107 @@ contract SafetyChecker is ContractResolver { | |
| view | ||
| returns (bool) | ||
| { | ||
| bool seenJUMP = false; | ||
| bool insideUnreachableCode = false; | ||
| uint256 ops = 0; | ||
| for (uint256 pc = 0; pc < _bytecode.length; pc++) { | ||
| // autogenerated by gen_safety_checker_constants.py | ||
| // number of bytes to skip for each opcode | ||
| uint256[8] memory opcodeSkippableBytes = [ | ||
| uint256(0x0001010101010101010101010000000001010101010101010101010101010000), | ||
| uint256(0x0100000000000000000000000000000000000000010101010101000000010100), | ||
| uint256(0x0000000000000000000000000000000001010101000000010101010100000000), | ||
| uint256(0x0203040500000000000000000000000000000000000000000000000000000000), | ||
| uint256(0x0101010101010101010101010101010101010101010101010101010101010101), | ||
| uint256(0x0101010101000000000000000000000000000000000000000000000000000000), | ||
| uint256(0x0000000000000000000000000000000000000000000000000000000000000000), | ||
| uint256(0x0000000000000000000000000000000000000000000000000000000000000000)]; | ||
| // Mask to gate opcode specific cases | ||
| uint256 opcodeGateMask = ~uint256(0xffffffffffffffffffffffe000000000fffffffff070ffff9c0ffffec000f001); | ||
| // Halting opcodes | ||
| uint256 opcodeHaltingMask = ~uint256(0x6008000000000000000000000000000000000000004000000000000000000001); | ||
| // PUSH opcodes | ||
| uint256 opcodePushMask = ~uint256(0xffffffff000000000000000000000000); | ||
|
|
||
| uint256 codeLength; | ||
| uint256 _pc; | ||
| assembly { | ||
| _pc := add(_bytecode, 0x20) | ||
| } | ||
| codeLength = _pc + _bytecode.length; | ||
| do { | ||
| // current opcode: 0x00...0xff | ||
| uint256 op = uint8(_bytecode[pc]); | ||
|
|
||
| // PUSH## | ||
| if (op >= 0x60 && op <= 0x7f) { | ||
| // subsequent bytes are not opcodes. Skip them. | ||
| pc += (op - 0x5f); | ||
| uint256 opNum; | ||
|
|
||
| // inline assembly removes the extra add + bounds check | ||
| assembly { | ||
| let word := mload(_pc) //load the next 32 bytes at pc into word | ||
|
|
||
| // Look up number of bytes to skip from opcodeSkippableBytes and then update indexInWord | ||
| // E.g. the 02030405 in opcodeSkippableBytes is the number of bytes to skip for PUSH1->4 | ||
| // We repeat this 6 times, thus we can only skip bytes for up to PUSH4 ((1+4) * 6 = 30 < 32). | ||
| // If we see an opcode that is listed as 0 skippable bytes e.g. PUSH5, then we will get stuck on that indexInWord and then opNum will be set to the PUSH5 opcode. | ||
| let indexInWord := byte(0, mload(add(opcodeSkippableBytes, byte(0, word)))) | ||
| indexInWord := add(indexInWord, byte(0, mload(add(opcodeSkippableBytes, byte(indexInWord, word))))) | ||
| indexInWord := add(indexInWord, byte(0, mload(add(opcodeSkippableBytes, byte(indexInWord, word))))) | ||
| indexInWord := add(indexInWord, byte(0, mload(add(opcodeSkippableBytes, byte(indexInWord, word))))) | ||
| indexInWord := add(indexInWord, byte(0, mload(add(opcodeSkippableBytes, byte(indexInWord, word))))) | ||
| indexInWord := add(indexInWord, byte(0, mload(add(opcodeSkippableBytes, byte(indexInWord, word))))) | ||
| _pc := add(_pc, indexInWord) | ||
|
|
||
| opNum := byte(indexInWord, word) | ||
| } | ||
| // If we're in between a STOP or REVERT or JUMP and a JUMPDEST | ||
| if (insideUnreachableCode) { | ||
| // JUMPDEST | ||
| if (op == 0x5b) { | ||
| // this bytecode is now reachable via JUMP or JUMPI | ||
| insideUnreachableCode = false; | ||
| } | ||
| } else { | ||
| // check that opcode is whitelisted (using the whitelist bit mask) | ||
| uint256 opBit = 1 << op; | ||
| if (opcodeWhitelistMask & opBit != opBit) { | ||
| // encountered a non-whitelisted opcode! | ||
| return false; | ||
| } | ||
| // append this opcode to a list of ops | ||
| ops <<= 8; | ||
| ops |= op; | ||
| // [0x57, 0x56, 0x00, 0xfd, 0xfe, 0xf3, 0xf1] all have handlers | ||
| if (opBit & 0x600a00000000000000000000000000000000000000c000000000000000000001 != 0) { | ||
| // JUMPI | ||
| if (op == 0x57) { | ||
| // We can now reach all JUMPDESTs | ||
| seenJUMP = true; | ||
| // JUMP | ||
| } else if (op == 0x56) { | ||
| // We can now reach all JUMPDESTs | ||
| seenJUMP = true; | ||
| // we are now inside unreachable code until we hit a JUMPDEST! | ||
| insideUnreachableCode = true; | ||
| // STOP or REVERT or INVALID or RETURN (see safety checker docs in wiki for more info) | ||
| } else if (op == 0x00 || op == 0xfd || op == 0xfe || op == 0xf3) { | ||
| // If we can't jump to JUMPDESTs, then all remaining bytecode is unreachable | ||
| if (!seenJUMP) { | ||
| return true; | ||
| } | ||
| // We are now inside unreachable code until we hit a JUMPDEST! | ||
| insideUnreachableCode = true; | ||
| // CALL | ||
| } else if (op == 0xf1) { | ||
| // Minimum 4 total ops: | ||
| // 1. PUSH1 value | ||
| // 2. PUSH20 execution manager address | ||
| // 3. PUSH or DUP gas | ||
| // 4. CALL | ||
|
|
||
| // if opIndex < 3, there would be 0s here, so we don't need the check | ||
| uint256 gasOp = (ops >> 8) & 0xFF; | ||
| uint256 addressOp = (ops >> 16) & 0xFF; | ||
| uint256 valueOp = (ops >> 24) & 0xFF; | ||
| if ( | ||
| gasOp < 0x60 || // PUSHes are 0x60...0x7f | ||
| gasOp > 0x8f || // DUPs are 0x80...0x8f | ||
| addressOp != 0x73 || // address must be set with a PUSH20 | ||
| valueOp != 0x60 // value must be set with a PUSH1 | ||
| ) { | ||
| return false; | ||
| } else { | ||
| uint256 pushedBytes; | ||
| // gas is set with a PUSH## | ||
| if (gasOp >= 0x60 && gasOp <= 0x7f) { | ||
| pushedBytes = gasOp - 0x5f; | ||
| } | ||
|
|
||
| // 23 is from 1 + PUSH20 + 20 bytes of address + PUSH or DUP gas | ||
| byte callValue = _bytecode[pc - (23 + pushedBytes)]; | ||
|
|
||
| // 21 is from 1 + 19 bytes of address + PUSH or DUP gas | ||
| address callAddress = toAddress(_bytecode, (pc - (21 + pushedBytes))); | ||
|
|
||
| // CALL is made to the execution manager with msg.value of 0 ETH | ||
| if (callAddress != address(resolveExecutionManager()) || callValue != 0 ) { | ||
| return false; | ||
| } | ||
| // + push opcodes | ||
| // + stop opcodes [STOP(0x00),JUMP(0x56),RETURN(0xf3),REVERT(0xfd),INVALID(0xfe)] | ||
| // + caller opcode CALLER(0x33) | ||
| // + blacklisted opcodes | ||
| uint256 opBit = 1 << opNum; | ||
| if (opBit & opcodeGateMask == 0) { | ||
| if (opBit & opcodePushMask == 0) { | ||
| // all pushes are valid opcodes | ||
| // subsequent bytes are not opcodes. Skip them. | ||
| _pc += (opNum - 0x5e); // PUSH1 is 0x60, so opNum-0x5f = PUSHed bytes and we +1 to | ||
| // skip the _pc++; line below in order to save gas ((-0x5f + 1) = -0x5e) | ||
| continue; | ||
| } else if (opBit & opcodeHaltingMask == 0) { | ||
| // STOP or JUMP or RETURN or REVERT or INVALID (see safety checker docs in wiki for more info) | ||
| // We are now inside unreachable code until we hit a JUMPDEST! | ||
|
Comment on lines
+98
to
+99
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can probably remove these if |
||
| do { | ||
| _pc++; | ||
| assembly { | ||
| opNum := byte(0, mload(_pc)) | ||
| } | ||
| // encountered a JUMPDEST | ||
| if (opNum == 0x5b) break; | ||
| // skip PUSHed bytes | ||
| if ((1 << opNum) & opcodePushMask == 0) _pc += (opNum - 0x5f); // opNum-0x5f = PUSHed bytes (PUSH1 is 0x60) | ||
| } while (_pc < codeLength); | ||
| // opNum is 0x5b, so we don't continue here since the pc++ is fine | ||
| } else if (opNum == 0x33) { // Caller opcode | ||
| // Sequence around CALLER must be: | ||
| // 1. CALLER (execution manager address) <-- We are here | ||
| // 2. PUSH1 0x0 (value) | ||
| // 3. SWAP1 (swap value and addres) | ||
| // 4. GAS (gas for call) | ||
| // 5. CALL | ||
|
|
||
| uint256 ops; // next 6 bytes of bytecode | ||
| // 32 - 6 bytes = 26 bytes = 208 bits | ||
| assembly { | ||
| ops := shr(208, mload(_pc)) | ||
| } | ||
|
|
||
| // allowed = CALLER PUSH1 0x00 SWAP1 GAS CALL | ||
| if (ops != 0x336000905af1) { | ||
| return false; | ||
| } | ||
|
|
||
| _pc += 6; | ||
| continue; | ||
| } else { | ||
| // encountered a non-whitelisted opcode! | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
| _pc++; | ||
| } while (_pc < codeLength); | ||
| return true; | ||
| } | ||
|
|
||
|
|
||
| /* | ||
| * Internal Functions | ||
| */ | ||
|
|
||
| /** | ||
| * Converts the 20 bytes at _start of _bytes into an address. | ||
| * @param _bytes The bytes to extract the address from. | ||
| * @param _start The start index from which to extract the address from | ||
| * (e.g. 0 if _bytes starts with the address). | ||
| * @return Bytes converted to an address. | ||
| */ | ||
| function toAddress( | ||
| bytes memory _bytes, | ||
| uint256 _start | ||
| ) | ||
| internal | ||
| pure | ||
| returns (address addr) | ||
| { | ||
| require(_bytes.length >= (_start + 20), "Addresses must be at least 20 bytes"); | ||
|
|
||
| assembly { | ||
| addr := mload(add(add(_bytes, 20), _start)) | ||
| } | ||
| } | ||
|
|
||
|
|
||
| /* | ||
| * Contract Resolution | ||
| */ | ||
|
|
||
| function resolveExecutionManager() | ||
| internal | ||
| view | ||
| returns (ExecutionManager) | ||
| { | ||
| return ExecutionManager(resolveContract("ExecutionManager")); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these be removed?