From 7bc57243cab52c3148877354e824aa8045554cd5 Mon Sep 17 00:00:00 2001 From: soad003 Date: Mon, 9 Jul 2018 17:18:15 +0200 Subject: [PATCH 1/4] Static Analysis: ERC20 decimals type check --- .../src/analysis/modules/erc20Decimals.js | 61 +++++++++++++ .../modules/abstractAstView.js | 1 + .../solidity-analyzer/modules/categories.js | 3 +- .../modules/staticAnalysisCommon.js | 22 ++++- .../staticAnalysisIntegration-test.js | 89 +++++++++++++++---- .../test/analysis/test-contracts/ERC20.sol | 46 ++++++++++ 6 files changed, 201 insertions(+), 21 deletions(-) create mode 100644 remix-analyzer/src/analysis/modules/erc20Decimals.js create mode 100644 remix-analyzer/test/analysis/test-contracts/ERC20.sol diff --git a/remix-analyzer/src/analysis/modules/erc20Decimals.js b/remix-analyzer/src/analysis/modules/erc20Decimals.js new file mode 100644 index 0000000000..604507b2b8 --- /dev/null +++ b/remix-analyzer/src/analysis/modules/erc20Decimals.js @@ -0,0 +1,61 @@ +var name = 'ERC20: ' +var desc = 'Decimal should be uint8' +var categories = require('./categories') +var common = require('./staticAnalysisCommon') +var AbstractAst = require('./abstractAstView') + +function erc20Decimals () { + this.abstractAst = new AbstractAst() + this.visit = this.abstractAst.build_visit( + (node) => false + ) + this.report = this.abstractAst.build_report(report) +} + +erc20Decimals.prototype.visit = function () { throw new Error('erc20Decimals.js no visit function set upon construction') } + +erc20Decimals.prototype.report = function () { throw new Error('erc20Decimals.js no report function set upon construction') } + +function report (contracts, multipleContractsWithSameName) { + var warnings = [] + + contracts.forEach((contract) => { + let contractAbiSignatures = contract.functions.map((f) => common.helpers.buildAbiSignature(common.getFunctionDefinitionName(f.node), f.parameters)) + + if (isERC20(contractAbiSignatures)) { + let decimalsVar = contract.stateVariables.filter((stateVar) => common.getDeclaredVariableName(stateVar) === 'decimals' && (common.getDeclaredVariableType(stateVar) !== 'uint8' || stateVar.attributes.visibility !== 'public')) + let decimalsFun = contract.functions.filter((f) => common.getFunctionDefinitionName(f.node) === 'decimals' && + ( + (f.returns.length === 0 || f.returns.length > 1) || + (f.returns.length === 1 && (f.returns[0].type !== 'uint8' || f.node.attributes.visibility !== 'public')) + ) + ) + + if (decimalsVar.length > 0 || decimalsFun.length > 0) { + warnings.push({ + warning: 'ERC20 Contracts decimals function should have uint8 as return type', + location: null, + more: 'https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20.md' + }) + } + } + }) + + return warnings +} + +function isERC20 (funSignatures) { + return funSignatures.includes('totalSupply()') && + funSignatures.includes('balanceOf(address)') && + funSignatures.includes('transfer(address,uint256)') && + funSignatures.includes('transferFrom(address,address,uint256)') && + funSignatures.includes('approve(address,uint256)') && + funSignatures.includes('allowance(address,address)') +} + +module.exports = { + name: name, + description: desc, + category: categories.ERC, + Module: erc20Decimals +} diff --git a/remix-analyzer/src/solidity-analyzer/modules/abstractAstView.js b/remix-analyzer/src/solidity-analyzer/modules/abstractAstView.js index ea28f77b32..399929650a 100644 --- a/remix-analyzer/src/solidity-analyzer/modules/abstractAstView.js +++ b/remix-analyzer/src/solidity-analyzer/modules/abstractAstView.js @@ -31,6 +31,7 @@ function abstractAstView () { * "modifierInvocations": [], // Modifier invocation AST nodes that are applied on this function * "localVariables": [], // Local variable declaration nodes * "parameters": [] // Parameter types of the function in order of definition + * "returns": [] // list of return vars as { type: ... , name: ... } * } * ], * "modifiers": [], // Modifiers definded by the contract, format similar to functions diff --git a/remix-analyzer/src/solidity-analyzer/modules/categories.js b/remix-analyzer/src/solidity-analyzer/modules/categories.js index 2b0e8d3414..4b3c89a53f 100644 --- a/remix-analyzer/src/solidity-analyzer/modules/categories.js +++ b/remix-analyzer/src/solidity-analyzer/modules/categories.js @@ -1,5 +1,6 @@ module.exports = { SECURITY: {displayName: 'Security', id: 'SEC'}, GAS: {displayName: 'Gas & Economy', id: 'GAS'}, - MISC: {displayName: 'Miscellaneous', id: 'MISC'} + MISC: {displayName: 'Miscellaneous', id: 'MISC'}, + ERC: {displayName: 'ERC', id: 'ERC'} } diff --git a/remix-analyzer/src/solidity-analyzer/modules/staticAnalysisCommon.js b/remix-analyzer/src/solidity-analyzer/modules/staticAnalysisCommon.js index 3f8c9953a9..cdfd65a72b 100644 --- a/remix-analyzer/src/solidity-analyzer/modules/staticAnalysisCommon.js +++ b/remix-analyzer/src/solidity-analyzer/modules/staticAnalysisCommon.js @@ -265,6 +265,18 @@ function getDeclaredVariableName (varDeclNode) { return varDeclNode.attributes.name } +/** + * Returns the type of a variable definition, Throws on wrong node. + * Example: + * var x = 10; => x + * @varDeclNode {ASTNode} Variable declaration node + * @return {string} variable type + */ +function getDeclaredVariableType (varDeclNode) { + if (!isVariableDeclaration(varDeclNode)) throw new Error('staticAnalysisCommon.js: not a variable declaration') + return varDeclNode.attributes.type +} + /** * Returns state variable declaration nodes for a contract, Throws on wrong node. * Example: @@ -757,7 +769,7 @@ function isBlockTimestampAccess (node) { * @return {bool} */ function isBlockBlockHashAccess (node) { - return isSpecialVariableAccess(node, specialVariables.BLOCKHASH) + return isSpecialVariableAccess(node, specialVariables.BLOCKHASH) || isBuiltinFunctionCall(node) && getLocalCallName(node) === 'blockhash' } /** @@ -919,6 +931,10 @@ function buildFunctionSignature (paramTypes, returnTypes, isPayable, additionalM return 'function (' + util.concatWithSeperator(paramTypes, ',') + ')' + ((isPayable) ? ' payable' : '') + ((additionalMods) ? ' ' + additionalMods : '') + ((returnTypes.length) ? ' returns (' + util.concatWithSeperator(returnTypes, ',') + ')' : '') } +function buildAbiSignature (funName, paramTypes) { + return funName + '(' + util.concatWithSeperator(paramTypes, ',') + ')' +} + /** * Finds first node of a certain type under a specific node. * @node {AstNode} node to start form @@ -948,6 +964,7 @@ module.exports = { getContractName: getContractName, getEffectedVariableName: getEffectedVariableName, getDeclaredVariableName: getDeclaredVariableName, + getDeclaredVariableType: getDeclaredVariableType, getLocalCallName: getLocalCallName, getInheritsFromName: getInheritsFromName, getExternalDirectCallContractName: getExternalDirectCallContractName, @@ -1033,6 +1050,7 @@ module.exports = { nodeType: nodeType, name: name, operator: operator, - buildFunctionSignature: buildFunctionSignature + buildFunctionSignature: buildFunctionSignature, + buildAbiSignature: buildAbiSignature } } diff --git a/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js b/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js index 1ffaefecb7..c5157d13c4 100644 --- a/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js +++ b/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js @@ -30,7 +30,8 @@ var testFiles = [ 'selfdestruct.sol', 'deleteDynamicArray.sol', 'blockLevelCompare.sol', - 'intDivisionTruncate.sol' + 'intDivisionTruncate.sol', + 'ERC20.sol' ] var testFileAsts = {} @@ -66,7 +67,8 @@ test('Integration test thisLocal.js', function (t) { 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, - 'intDivisionTruncate.sol': 0 + 'intDivisionTruncate.sol': 0, + 'ERC20.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -100,7 +102,8 @@ test('Integration test checksEffectsInteraction.js', function (t) { 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, - 'intDivisionTruncate.sol': 0 + 'intDivisionTruncate.sol': 0, + 'ERC20.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -134,7 +137,8 @@ test('Integration test constantFunctions.js', function (t) { 'selfdestruct.sol': 1, 'deleteDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, - 'intDivisionTruncate.sol': 0 + 'intDivisionTruncate.sol': 0, + 'ERC20.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -168,7 +172,8 @@ test('Integration test inlineAssembly.js', function (t) { 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, - 'intDivisionTruncate.sol': 0 + 'intDivisionTruncate.sol': 0, + 'ERC20.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -202,7 +207,8 @@ test('Integration test txOrigin.js', function (t) { 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, - 'intDivisionTruncate.sol': 0 + 'intDivisionTruncate.sol': 0, + 'ERC20.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -236,7 +242,8 @@ test('Integration test gasCosts.js', function (t) { 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 2, 'blockLevelCompare.sol': 1, - 'intDivisionTruncate.sol': 1 + 'intDivisionTruncate.sol': 1, + 'ERC20.sol': 2 } runModuleOnFiles(module, t, (file, report) => { @@ -270,7 +277,8 @@ test('Integration test similarVariableNames.js', function (t) { 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 1, 'blockLevelCompare.sol': 0, - 'intDivisionTruncate.sol': 0 + 'intDivisionTruncate.sol': 0, + 'ERC20.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -304,7 +312,8 @@ test('Integration test inlineAssembly.js', function (t) { 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, - 'intDivisionTruncate.sol': 0 + 'intDivisionTruncate.sol': 0, + 'ERC20.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -338,7 +347,8 @@ test('Integration test blockTimestamp.js', function (t) { 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, - 'intDivisionTruncate.sol': 0 + 'intDivisionTruncate.sol': 0, + 'ERC20.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -372,7 +382,8 @@ test('Integration test lowLevelCalls.js', function (t) { 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, - 'intDivisionTruncate.sol': 0 + 'intDivisionTruncate.sol': 0, + 'ERC20.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -406,7 +417,8 @@ test('Integration test blockBlockhash.js', function (t) { 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, - 'intDivisionTruncate.sol': 0 + 'intDivisionTruncate.sol': 0, + 'ERC20.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -440,7 +452,8 @@ test('Integration test noReturn.js', function (t) { 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, - 'intDivisionTruncate.sol': 0 + 'intDivisionTruncate.sol': 0, + 'ERC20.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -474,7 +487,8 @@ test('Integration test selfdestruct.js', function (t) { 'selfdestruct.sol': 3, 'deleteDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, - 'intDivisionTruncate.sol': 5 + 'intDivisionTruncate.sol': 5, + 'ERC20.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -508,7 +522,8 @@ test('Integration test guardConditions.js', function (t) { 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 1, 'blockLevelCompare.sol': 0, - 'intDivisionTruncate.sol': 1 + 'intDivisionTruncate.sol': 1, + 'ERC20.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -542,7 +557,8 @@ test('Integration test deleteDynamicArrays.js', function (t) { 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 2, 'blockLevelCompare.sol': 0, - 'intDivisionTruncate.sol': 0 + 'intDivisionTruncate.sol': 0, + 'ERC20.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -576,7 +592,8 @@ test('Integration test assignAndCompare.js', function (t) { 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, 'blockLevelCompare.sol': 8, - 'intDivisionTruncate.sol': 0 + 'intDivisionTruncate.sol': 0, + 'ERC20.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -610,7 +627,8 @@ test('Integration test intDivisionTruncate.js', function (t) { 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, - 'intDivisionTruncate.sol': 2 + 'intDivisionTruncate.sol': 2, + 'ERC20.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -618,6 +636,41 @@ test('Integration test intDivisionTruncate.js', function (t) { }) }) +test('Integration test erc20Decimal.js', function (t) { + t.plan(testFiles.length) + + var module = require('../../src/analysis/modules/erc20Decimals') + + var lengthCheck = { + 'KingOfTheEtherThrone.sol': 0, + 'assembly.sol': 0, + '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': 0, + 'library.sol': 0, + 'transfer.sol': 0, + 'ctor.sol': 0, + 'forgottenReturn.sol': 0, + 'selfdestruct.sol': 0, + 'deleteDynamicArray.sol': 0, + 'blockLevelCompare.sol': 0, + 'intDivisionTruncate.sol': 0, + 'ERC20.sol': 1 + } + + runModuleOnFiles(module, t, (file, report) => { + t.equal(report.length, lengthCheck[file], `${file} has right amount of erc20Decimals warnings`) + }) +}) + // #################### Helpers function runModuleOnFiles (module, t, cb) { var statRunner = new StatRunner() diff --git a/remix-analyzer/test/analysis/test-contracts/ERC20.sol b/remix-analyzer/test/analysis/test-contracts/ERC20.sol new file mode 100644 index 0000000000..38a6f77113 --- /dev/null +++ b/remix-analyzer/test/analysis/test-contracts/ERC20.sol @@ -0,0 +1,46 @@ + +pragma solidity ^0.4.17; +contract EIP20 { + + uint public decimals = 12; + + // optional + function name() public pure returns (string) { + return "MYTOKEN"; + } + + // optional + function symbol() public pure returns (string) { + return "MT"; + } + + // optional + //function decimals() internal pure returns (uint8) { + // return 12; + //} + + function totalSupply() public pure returns (uint256) { + return 12000; + } + + function balanceOf(address _owner) public pure returns (uint256) { + return 0; + } + + function transfer(address _to, uint256 _value) public pure returns (bool success) { + return true; + } + + function transferFrom(address _from, address _to, uint256 _value) public pure returns (bool) { + return true; + } + + function approve(address _spender, uint256 _value) public pure returns (bool) { + return true; + } + + function allowance(address _owner, address _spender) public pure returns (uint256) { + return 0; + } + +} \ No newline at end of file From c43c917ee3ec9feb3b1d8ada3ec10250307303a3 Mon Sep 17 00:00:00 2001 From: soad003 Date: Fri, 13 Jul 2018 14:43:30 +0200 Subject: [PATCH 2/4] Static Analysis: ERC20 decimals, include module --- remix-analyzer/src/solidity-analyzer/modules/list.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/remix-analyzer/src/solidity-analyzer/modules/list.js b/remix-analyzer/src/solidity-analyzer/modules/list.js index f693508ac4..1cbdd0473a 100644 --- a/remix-analyzer/src/solidity-analyzer/modules/list.js +++ b/remix-analyzer/src/solidity-analyzer/modules/list.js @@ -13,5 +13,6 @@ module.exports = [ require('./selfdestruct'), require('./guardConditions'), require('./deleteDynamicArrays'), - require('./assignAndCompare') + require('./assignAndCompare'), + require('./erc20Decimals') ] From 7fc9c2eb5ed94b971bc7fc16e74e017a480ce481 Mon Sep 17 00:00:00 2001 From: soad003 Date: Tue, 17 Jul 2018 14:56:33 +0200 Subject: [PATCH 3/4] Static Analysis: ERC20 decimals, fixes suggested by axic --- remix-analyzer/src/analysis/modules/erc20Decimals.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/remix-analyzer/src/analysis/modules/erc20Decimals.js b/remix-analyzer/src/analysis/modules/erc20Decimals.js index 604507b2b8..265dfef0d7 100644 --- a/remix-analyzer/src/analysis/modules/erc20Decimals.js +++ b/remix-analyzer/src/analysis/modules/erc20Decimals.js @@ -35,7 +35,7 @@ function report (contracts, multipleContractsWithSameName) { warnings.push({ warning: 'ERC20 Contracts decimals function should have uint8 as return type', location: null, - more: 'https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20.md' + more: ' https://eips.ethereum.org/EIPS/eip-20' }) } } From d1374ba6f204266ee170ef4f191f73423e689c63 Mon Sep 17 00:00:00 2001 From: soad003 Date: Wed, 26 Sep 2018 10:26:02 +0200 Subject: [PATCH 4/4] static analysis: new folder structure --- .../{analysis => solidity-analyzer}/modules/erc20Decimals.js | 2 ++ remix-analyzer/test/analysis/staticAnalysisIntegration-test.js | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) rename remix-analyzer/src/{analysis => solidity-analyzer}/modules/erc20Decimals.js (97%) diff --git a/remix-analyzer/src/analysis/modules/erc20Decimals.js b/remix-analyzer/src/solidity-analyzer/modules/erc20Decimals.js similarity index 97% rename from remix-analyzer/src/analysis/modules/erc20Decimals.js rename to remix-analyzer/src/solidity-analyzer/modules/erc20Decimals.js index 265dfef0d7..11747f4461 100644 --- a/remix-analyzer/src/analysis/modules/erc20Decimals.js +++ b/remix-analyzer/src/solidity-analyzer/modules/erc20Decimals.js @@ -3,6 +3,7 @@ var desc = 'Decimal should be uint8' var categories = require('./categories') var common = require('./staticAnalysisCommon') var AbstractAst = require('./abstractAstView') +var algo = require('./algorithmCategories') function erc20Decimals () { this.abstractAst = new AbstractAst() @@ -57,5 +58,6 @@ module.exports = { name: name, description: desc, category: categories.ERC, + algorithm: algo.EXACT, Module: erc20Decimals } diff --git a/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js b/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js index c5157d13c4..0ad1335640 100644 --- a/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js +++ b/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js @@ -639,7 +639,7 @@ test('Integration test intDivisionTruncate.js', function (t) { test('Integration test erc20Decimal.js', function (t) { t.plan(testFiles.length) - var module = require('../../src/analysis/modules/erc20Decimals') + var module = require('../../src/solidity-analyzer/modules/erc20Decimals') var lengthCheck = { 'KingOfTheEtherThrone.sol': 0,