From 32a682762ab96d61f4d7831f2b26379a81a57efa Mon Sep 17 00:00:00 2001 From: soad003 Date: Wed, 13 Dec 2017 13:39:34 +0100 Subject: [PATCH] Static Analysis: require / assert --- remix-solidity/package.json | 2 +- .../src/analysis/modules/guardConditions.js | 29 +++++++++++++++++ remix-solidity/src/analysis/modules/list.js | 3 +- .../analysis/modules/staticAnalysisCommon.js | 24 +++++++++++++- .../staticAnalysisIntegration-test.js | 31 +++++++++++++++++++ .../test/analysis/test-contracts/assembly.sol | 5 +-- .../test/analysis/test-contracts/globals.sol | 5 +-- 7 files changed, 92 insertions(+), 7 deletions(-) create mode 100644 remix-solidity/src/analysis/modules/guardConditions.js diff --git a/remix-solidity/package.json b/remix-solidity/package.json index 299104e0e8..db21b7ed7a 100644 --- a/remix-solidity/package.json +++ b/remix-solidity/package.json @@ -31,7 +31,7 @@ "scripts": { "postinstall": "npm-link-local ../remix-lib && npm-link-local ../remix-core", "test": "standard && npm run downloadsolc && tape ./test/tests.js", - "downloadsolc": "wget https://ethereum.github.io/solc-bin/soljson.js" + "downloadsolc": "test -e soljson.js || wget https://ethereum.github.io/solc-bin/soljson.js" }, "standard": { "ignore": [ diff --git a/remix-solidity/src/analysis/modules/guardConditions.js b/remix-solidity/src/analysis/modules/guardConditions.js new file mode 100644 index 0000000000..7a4dbf548d --- /dev/null +++ b/remix-solidity/src/analysis/modules/guardConditions.js @@ -0,0 +1,29 @@ +var name = 'Guard Conditions: ' +var desc = 'Use require and appropriately' +var categories = require('./categories') +var common = require('./staticAnalysisCommon') + +function guardConditions () { + this.guards = [] +} + +guardConditions.prototype.visit = function (node) { + if (common.isRequireCall(node) || common.isAssertCall(node)) this.guards.push(node) +} + +guardConditions.prototype.report = function (compilationResults) { + if (this.guards.length > 0) { + return [{ + warning: 'Use assert(x) if you never ever want x to be false, not in any circumstance (apart from a bug in your code). Use require(x) if x can be false, due to e.g. invalid input or a failing external component.', + more: 'http://solidity.readthedocs.io/en/develop/control-structures.html#error-handling-assert-require-revert-and-exceptions' + }] + } + return [] +} + +module.exports = { + name: name, + description: desc, + category: categories.MISC, + Module: guardConditions +} diff --git a/remix-solidity/src/analysis/modules/list.js b/remix-solidity/src/analysis/modules/list.js index e510f323f6..a4e0ea6b0e 100644 --- a/remix-solidity/src/analysis/modules/list.js +++ b/remix-solidity/src/analysis/modules/list.js @@ -10,5 +10,6 @@ module.exports = [ require('./lowLevelCalls'), require('./blockBlockhash'), require('./noReturn'), - require('./selfdestruct') + require('./selfdestruct'), + require('./guardConditions') ] diff --git a/remix-solidity/src/analysis/modules/staticAnalysisCommon.js b/remix-solidity/src/analysis/modules/staticAnalysisCommon.js index 0c3fdc00be..28fd50281a 100644 --- a/remix-solidity/src/analysis/modules/staticAnalysisCommon.js +++ b/remix-solidity/src/analysis/modules/staticAnalysisCommon.js @@ -20,7 +20,9 @@ var nodeTypes = { INLINEASSEMBLY: 'InlineAssembly', BLOCK: 'Block', NEWEXPRESSION: 'NewExpression', - RETURN: 'Return' + RETURN: 'Return', + ASSERT: 'assert', + REQUIRE: 'require' } var basicTypes = { @@ -416,6 +418,24 @@ function isSelfdestructCall (node) { return isBuiltinFunctionCall(node) && getLocalCallName(node) === 'selfdestruct' } +/** + * True if node is a call to assert + * @node {ASTNode} some AstNode + * @return {bool} + */ +function isAssertCall (node) { + return isBuiltinFunctionCall(node) && getLocalCallName(node) === 'assert' +} + +/** + * True if node is a call to assert + * @node {ASTNode} some AstNode + * @return {bool} + */ +function isRequireCall (node) { + return isBuiltinFunctionCall(node) && getLocalCallName(node) === 'require' +} + /** * True if is storage variable declaration * @node {ASTNode} some AstNode @@ -810,6 +830,8 @@ module.exports = { isMinusMinusUnaryOperation: isMinusMinusUnaryOperation, isBuiltinFunctionCall: isBuiltinFunctionCall, isSelfdestructCall: isSelfdestructCall, + isAssertCall: isAssertCall, + isRequireCall: isRequireCall, // #################### Trivial Node Identification isFunctionDefinition: isFunctionDefinition, diff --git a/remix-solidity/test/analysis/staticAnalysisIntegration-test.js b/remix-solidity/test/analysis/staticAnalysisIntegration-test.js index 9acf740910..3104a520e4 100644 --- a/remix-solidity/test/analysis/staticAnalysisIntegration-test.js +++ b/remix-solidity/test/analysis/staticAnalysisIntegration-test.js @@ -441,6 +441,37 @@ test('Integration test selfdestruct.js', function (t) { }) }) +test('Integration test guardConditions.js', function (t) { + t.plan(testFiles.length) + + var module = require('../../src/analysis/modules/guardConditions') + + var lengthCheck = { + 'KingOfTheEtherThrone.sol': 0, + 'assembly.sol': 1, + 'ballot.sol': 0, + 'ballot_reentrant.sol': 0, + 'ballot_withoutWarnings.sol': 0, + 'cross_contract.sol': 0, + 'inheritance.sol': 0, + 'modifier1.sol': 0, + 'modifier2.sol': 0, + 'notReentrant.sol': 0, + 'structReentrant.sol': 0, + 'thisLocal.sol': 0, + 'globals.sol': 1, + 'library.sol': 0, + 'transfer.sol': 0, + 'ctor.sol': 0, + 'forgottenReturn.sol': 0, + 'selfdestruct.sol': 0 + } + + runModuleOnFiles(module, t, (file, report) => { + t.equal(report.length, lengthCheck[file], `${file} has right amount of guardCondition warnings`) + }) +}) + // #################### Helpers function runModuleOnFiles (module, t, cb) { var statRunner = new StatRunner() diff --git a/remix-solidity/test/analysis/test-contracts/assembly.sol b/remix-solidity/test/analysis/test-contracts/assembly.sol index a71c7e4a26..fb647502eb 100644 --- a/remix-solidity/test/analysis/test-contracts/assembly.sol +++ b/remix-solidity/test/analysis/test-contracts/assembly.sol @@ -4,6 +4,7 @@ address owner; function at(address _addr) returns (bytes o_code) { + assert(_addr != 0x0); assembly { // retrieve the size of the code, this needs assembly let size := extcodesize(_addr) @@ -20,8 +21,8 @@ } function bla() { - if(tx.origin == owner) - msg.sender.send(19); + require(tx.origin == owner); + msg.sender.send(19); assembly { } diff --git a/remix-solidity/test/analysis/test-contracts/globals.sol b/remix-solidity/test/analysis/test-contracts/globals.sol index c945f84bba..7df37c757f 100644 --- a/remix-solidity/test/analysis/test-contracts/globals.sol +++ b/remix-solidity/test/analysis/test-contracts/globals.sol @@ -32,8 +32,8 @@ contract a is bla { now; tx.gasprice; tx.origin; - //assert(now == block.timestamp); - //require(now == block.timestamp); + // assert(1 == 2); + // require(1 == 1); keccak256(a); sha3(a); sha256(a); @@ -52,6 +52,7 @@ contract a is bla { //a.transfer(a.balance); selfdestruct(a); //revert(); + assert(a.balance == 0); } } \ No newline at end of file