From 0477a18f89000e18458801cd7f963f327691e7e0 Mon Sep 17 00:00:00 2001 From: Paulius Date: Sun, 10 Jun 2018 12:31:31 +0300 Subject: [PATCH 1/9] Add 'address(address)' to the list of built-in functions --- .../src/solidity-analyzer/modules/staticAnalysisCommon.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/remix-analyzer/src/solidity-analyzer/modules/staticAnalysisCommon.js b/remix-analyzer/src/solidity-analyzer/modules/staticAnalysisCommon.js index 4b6b89f97e..3a8f022be2 100644 --- a/remix-analyzer/src/solidity-analyzer/modules/staticAnalysisCommon.js +++ b/remix-analyzer/src/solidity-analyzer/modules/staticAnalysisCommon.js @@ -71,7 +71,8 @@ var builtinFunctions = { 'require(bool)': true, 'require(bool,string memory)': true, 'gasleft()': true, - 'blockhash(uint)': true + 'blockhash(uint)': true, + 'address(address)': true } var lowLevelCallTypes = { From 6d94bd0ed6716239b654c9419b9a787ab376daac Mon Sep 17 00:00:00 2001 From: Paulius Date: Sun, 10 Jun 2018 12:43:00 +0300 Subject: [PATCH 2/9] Static analysis: Add module for deletion from dynamic array --- .../src/solidity-analyzer/modules/list.js | 3 +- .../modules/staticAnalysisCommon.js | 20 +++++++++++++ .../modules/deleteFromDynamicArray.js | 29 +++++++++++++++++++ 3 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 remix-solidity/src/analysis/modules/deleteFromDynamicArray.js diff --git a/remix-analyzer/src/solidity-analyzer/modules/list.js b/remix-analyzer/src/solidity-analyzer/modules/list.js index 971e8758d8..91a8842fe5 100644 --- a/remix-analyzer/src/solidity-analyzer/modules/list.js +++ b/remix-analyzer/src/solidity-analyzer/modules/list.js @@ -15,5 +15,6 @@ module.exports = [ require('./deleteDynamicArrays'), require('./assignAndCompare'), require('./erc20Decimals'), - require('./stringBytesLength') + require('./stringBytesLength'), + require('./deleteFromDynamicArray') ] diff --git a/remix-analyzer/src/solidity-analyzer/modules/staticAnalysisCommon.js b/remix-analyzer/src/solidity-analyzer/modules/staticAnalysisCommon.js index 3a8f022be2..89cedfe536 100644 --- a/remix-analyzer/src/solidity-analyzer/modules/staticAnalysisCommon.js +++ b/remix-analyzer/src/solidity-analyzer/modules/staticAnalysisCommon.js @@ -501,6 +501,24 @@ function isDynamicArrayAccess (node) { return node && nodeType(node, exactMatch(nodeTypes.IDENTIFIER)) && (node.attributes.type.endsWith('[] storage ref') || node.attributes.type === 'bytes storage ref' || node.attributes.type === 'string storage ref') } +/** + * True if node is a delete instruction for an element from a dynamic array + * @node {ASTNode} node to check for + * @return {bool} + */ +function isDeleteFromDynamicArray (node) { + return isDeleteUnaryOperation(node) && isIndexAccess(node.children[0]) +} + +/** + * True if node is the access of an index + * @node {ASTNode} node to check for + * @return {bool} + */ +function isIndexAccess (node) { + return node && node.name === 'IndexAccess' +} + /** * True if call to code within the current contracts context including (delegate) library call * @node {ASTNode} some AstNode @@ -999,9 +1017,11 @@ module.exports = { // #################### Complex Node Identification isDeleteOfDynamicArray: isDeleteOfDynamicArray, + isDeleteFromDynamicArray: isDeleteFromDynamicArray, isAbiNamespaceCall: isAbiNamespaceCall, isSpecialVariableAccess: isSpecialVariableAccess, isDynamicArrayAccess: isDynamicArrayAccess, + isIndexAccess: isIndexAccess, isSubScopeWithTopLevelUnAssignedBinOp: isSubScopeWithTopLevelUnAssignedBinOp, hasFunctionBody: hasFunctionBody, isInteraction: isInteraction, diff --git a/remix-solidity/src/analysis/modules/deleteFromDynamicArray.js b/remix-solidity/src/analysis/modules/deleteFromDynamicArray.js new file mode 100644 index 0000000000..9557deb1dc --- /dev/null +++ b/remix-solidity/src/analysis/modules/deleteFromDynamicArray.js @@ -0,0 +1,29 @@ +var name = 'Delete from dynamic Array: ' +var desc = 'Using delete on an array leaves a gap' +var categories = require('./categories') +var common = require('./staticAnalysisCommon') + +function deleteFromDynamicArray () { + this.relevantNodes = [] +} + +deleteFromDynamicArray.prototype.visit = function (node) { + if (common.isDeleteFromDynamicArray(node)) this.relevantNodes.push(node) +} + +deleteFromDynamicArray.prototype.report = function (compilationResults) { + return this.relevantNodes.map((node) => { + return { + warning: 'Using delete on an array leaves a gap. The length of the array remains the same. If you want to remove the empty position you need to shift items manually and update the length property.\n', + location: node.src, + more: 'https://github.com/miguelmota/solidity-idiosyncrasies' + } + }) +} + +module.exports = { + name: name, + description: desc, + category: categories.MISC, + Module: deleteFromDynamicArray +} From 7bf87ea1755a12f25b5671f3026c034f206a601c Mon Sep 17 00:00:00 2001 From: Paulius Date: Sun, 10 Jun 2018 13:49:42 +0300 Subject: [PATCH 3/9] Static analysis: Add module for loop iteration over dynamic array --- .../src/solidity-analyzer/modules/list.js | 3 +- .../modules/staticAnalysisCommon.js | 10 ++++++ .../forLoopIteratesOverDynamicArray.js | 33 +++++++++++++++++++ 3 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 remix-solidity/src/analysis/modules/forLoopIteratesOverDynamicArray.js diff --git a/remix-analyzer/src/solidity-analyzer/modules/list.js b/remix-analyzer/src/solidity-analyzer/modules/list.js index 91a8842fe5..6c7be2d630 100644 --- a/remix-analyzer/src/solidity-analyzer/modules/list.js +++ b/remix-analyzer/src/solidity-analyzer/modules/list.js @@ -16,5 +16,6 @@ module.exports = [ require('./assignAndCompare'), require('./erc20Decimals'), require('./stringBytesLength'), - require('./deleteFromDynamicArray') + require('./deleteFromDynamicArray'), + require('./forLoopIteratesOverDynamicArray') ] diff --git a/remix-analyzer/src/solidity-analyzer/modules/staticAnalysisCommon.js b/remix-analyzer/src/solidity-analyzer/modules/staticAnalysisCommon.js index 89cedfe536..eb5db00554 100644 --- a/remix-analyzer/src/solidity-analyzer/modules/staticAnalysisCommon.js +++ b/remix-analyzer/src/solidity-analyzer/modules/staticAnalysisCommon.js @@ -905,6 +905,15 @@ function isBytesLengthCheck (node) { return isMemberAccess(node, exactMatch(util.escapeRegExp(basicTypes.UINT)), undefined, util.escapeRegExp('bytes *'), 'length') } +/** + * True if it is a 'for' loop + * @node {ASTNode} some AstNode + * @return {bool} + */ +function isForLoop (node) { + return nodeType(node, exactMatch(nodeTypes.FORSTATEMENT)) +} + // #################### Complex Node Identification - Private function isMemberAccess (node, retType, accessor, accessorType, memberName) { @@ -1055,6 +1064,7 @@ module.exports = { isIntDivision: isIntDivision, isStringToBytesConversion: isStringToBytesConversion, isBytesLengthCheck: isBytesLengthCheck, + isForLoop: isForLoop, // #################### Trivial Node Identification isDeleteUnaryOperation: isDeleteUnaryOperation, diff --git a/remix-solidity/src/analysis/modules/forLoopIteratesOverDynamicArray.js b/remix-solidity/src/analysis/modules/forLoopIteratesOverDynamicArray.js new file mode 100644 index 0000000000..40b8c899a6 --- /dev/null +++ b/remix-solidity/src/analysis/modules/forLoopIteratesOverDynamicArray.js @@ -0,0 +1,33 @@ +var name = 'For loop iterates over dynamic array: ' +var desc = 'The number of \'for\' loop iterations depends on dynamic array\'s size' +var categories = require('./categories') +var common = require('./staticAnalysisCommon') + +function forLoopIteratesOverDynamicArray () { + this.relevantNodes = [] +} + +forLoopIteratesOverDynamicArray.prototype.visit = function (node) { + if (common.isForLoop(node) && + node.children[1].children[1].attributes.member_name === 'length' && + node.children[1].children[1].children[0].attributes.type.indexOf('[]') != -1) { + this.relevantNodes.push(node) + } +} + +forLoopIteratesOverDynamicArray.prototype.report = function (compilationResults) { + return this.relevantNodes.map((node) => { + return { + warning: 'Loops that do not have a fixed number of iterations, for example, loops that depend on storage values, have to be used carefully: Due to the block gas limit, transactions can only consume a certain amount of gas. The number of iterations in a loop can grow beyond the block gas limit which can cause the complete contract to be stalled at a certain point. Additionally, using unbounded loops incurs in a lot of avoidable gas costs. Carefully test how many items at maximum you can pass to such functions to make it successful.', + location: node.src, + more: 'http://solidity.readthedocs.io/en/v0.4.24/security-considerations.html#gas-limit-and-loops' + } + }) +} + +module.exports = { + name: name, + description: desc, + category: categories.GAS, + Module: forLoopIteratesOverDynamicArray +} From e0340000c695d6fb224fc7323c437e0a2ba016cd Mon Sep 17 00:00:00 2001 From: Paulius Date: Sun, 10 Jun 2018 14:05:30 +0300 Subject: [PATCH 4/9] Static analysis: Update module for loop iteration over dynamic array --- .../modules/forLoopIteratesOverDynamicArray.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/remix-solidity/src/analysis/modules/forLoopIteratesOverDynamicArray.js b/remix-solidity/src/analysis/modules/forLoopIteratesOverDynamicArray.js index 40b8c899a6..dbbcb90d58 100644 --- a/remix-solidity/src/analysis/modules/forLoopIteratesOverDynamicArray.js +++ b/remix-solidity/src/analysis/modules/forLoopIteratesOverDynamicArray.js @@ -8,11 +8,11 @@ function forLoopIteratesOverDynamicArray () { } forLoopIteratesOverDynamicArray.prototype.visit = function (node) { - if (common.isForLoop(node) && - node.children[1].children[1].attributes.member_name === 'length' && - node.children[1].children[1].children[0].attributes.type.indexOf('[]') != -1) { - this.relevantNodes.push(node) - } + if (common.isForLoop(node) && + node.children[1].children[1].attributes.member_name === 'length' && + node.children[1].children[1].children[0].attributes.type.indexOf('[]') !== -1) { + this.relevantNodes.push(node) + } } forLoopIteratesOverDynamicArray.prototype.report = function (compilationResults) { From 87fb68bd6ed33ab76971518548f107d628363970 Mon Sep 17 00:00:00 2001 From: Paulius Date: Tue, 21 Aug 2018 12:21:14 +0300 Subject: [PATCH 5/9] Move new static analysis modules to a proper place --- .../src/solidity-analyzer}/modules/deleteFromDynamicArray.js | 0 .../solidity-analyzer}/modules/forLoopIteratesOverDynamicArray.js | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename {remix-solidity/src/analysis => remix-analyzer/src/solidity-analyzer}/modules/deleteFromDynamicArray.js (100%) rename {remix-solidity/src/analysis => remix-analyzer/src/solidity-analyzer}/modules/forLoopIteratesOverDynamicArray.js (100%) diff --git a/remix-solidity/src/analysis/modules/deleteFromDynamicArray.js b/remix-analyzer/src/solidity-analyzer/modules/deleteFromDynamicArray.js similarity index 100% rename from remix-solidity/src/analysis/modules/deleteFromDynamicArray.js rename to remix-analyzer/src/solidity-analyzer/modules/deleteFromDynamicArray.js diff --git a/remix-solidity/src/analysis/modules/forLoopIteratesOverDynamicArray.js b/remix-analyzer/src/solidity-analyzer/modules/forLoopIteratesOverDynamicArray.js similarity index 100% rename from remix-solidity/src/analysis/modules/forLoopIteratesOverDynamicArray.js rename to remix-analyzer/src/solidity-analyzer/modules/forLoopIteratesOverDynamicArray.js From 6da7ef238400c2e45cbdfd1f1ef45a9f97ca30fc Mon Sep 17 00:00:00 2001 From: yann300 Date: Wed, 19 Sep 2018 12:18:11 +0200 Subject: [PATCH 6/9] standard --- .../modules/forLoopIteratesOverDynamicArray.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/remix-analyzer/src/solidity-analyzer/modules/forLoopIteratesOverDynamicArray.js b/remix-analyzer/src/solidity-analyzer/modules/forLoopIteratesOverDynamicArray.js index dbbcb90d58..3de397a85e 100644 --- a/remix-analyzer/src/solidity-analyzer/modules/forLoopIteratesOverDynamicArray.js +++ b/remix-analyzer/src/solidity-analyzer/modules/forLoopIteratesOverDynamicArray.js @@ -8,11 +8,11 @@ function forLoopIteratesOverDynamicArray () { } forLoopIteratesOverDynamicArray.prototype.visit = function (node) { - if (common.isForLoop(node) && - node.children[1].children[1].attributes.member_name === 'length' && - node.children[1].children[1].children[0].attributes.type.indexOf('[]') !== -1) { - this.relevantNodes.push(node) - } + if (common.isForLoop(node) && + node.children[1].children[1].attributes.member_name === 'length' && + node.children[1].children[1].children[0].attributes.type.indexOf('[]') !== -1) { + this.relevantNodes.push(node) + } } forLoopIteratesOverDynamicArray.prototype.report = function (compilationResults) { From 6501c2821a65870a2fb5e07d7577fc0f9263ef92 Mon Sep 17 00:00:00 2001 From: 0mkar <0mkar@protonmail.com> Date: Wed, 3 Oct 2018 12:07:52 +0530 Subject: [PATCH 7/9] add deleteFromDynamicArray tests --- .../staticAnalysisIntegration-test.js | 57 +++++++++++++++++++ .../test-contracts/deleteFromDynamicArray.sol | 21 +++++++ 2 files changed, 78 insertions(+) create mode 100644 remix-analyzer/test/analysis/test-contracts/deleteFromDynamicArray.sol diff --git a/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js b/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js index 56748e5534..33abf1b937 100644 --- a/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js +++ b/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js @@ -29,6 +29,7 @@ var testFiles = [ 'forgottenReturn.sol', 'selfdestruct.sol', 'deleteDynamicArray.sol', + 'deleteFromDynamicArray.sol', 'blockLevelCompare.sol', 'intDivisionTruncate.sol', 'ERC20.sol', @@ -67,6 +68,7 @@ test('Integration test thisLocal.js', function (t) { 'forgottenReturn.sol': 0, 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, + 'deleteFromDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, @@ -103,6 +105,7 @@ test('Integration test checksEffectsInteraction.js', function (t) { 'forgottenReturn.sol': 0, 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, + 'deleteFromDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, @@ -139,6 +142,7 @@ test('Integration test constantFunctions.js', function (t) { 'forgottenReturn.sol': 0, 'selfdestruct.sol': 1, 'deleteDynamicArray.sol': 0, + 'deleteFromDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, @@ -175,6 +179,7 @@ test('Integration test inlineAssembly.js', function (t) { 'forgottenReturn.sol': 0, 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, + 'deleteFromDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, @@ -211,6 +216,7 @@ test('Integration test txOrigin.js', function (t) { 'forgottenReturn.sol': 0, 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, + 'deleteFromDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, @@ -247,6 +253,7 @@ test('Integration test gasCosts.js', function (t) { 'forgottenReturn.sol': 3, 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 2, + 'deleteFromDynamicArray.sol': 2, 'blockLevelCompare.sol': 1, 'intDivisionTruncate.sol': 1, 'ERC20.sol': 2, @@ -283,6 +290,7 @@ test('Integration test similarVariableNames.js', function (t) { 'forgottenReturn.sol': 0, 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 1, + 'deleteFromDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, @@ -319,6 +327,7 @@ test('Integration test inlineAssembly.js', function (t) { 'forgottenReturn.sol': 0, 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, + 'deleteFromDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, @@ -355,6 +364,7 @@ test('Integration test blockTimestamp.js', function (t) { 'forgottenReturn.sol': 0, 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, + 'deleteFromDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, @@ -391,6 +401,7 @@ test('Integration test lowLevelCalls.js', function (t) { 'forgottenReturn.sol': 0, 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, + 'deleteFromDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, @@ -427,6 +438,7 @@ test('Integration test blockBlockhash.js', function (t) { 'forgottenReturn.sol': 0, 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, + 'deleteFromDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, @@ -463,6 +475,7 @@ test('Integration test noReturn.js', function (t) { 'forgottenReturn.sol': 1, 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, + 'deleteFromDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, @@ -499,6 +512,7 @@ test('Integration test selfdestruct.js', function (t) { 'forgottenReturn.sol': 0, 'selfdestruct.sol': 3, 'deleteDynamicArray.sol': 0, + 'deleteFromDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, 'ERC20.sol': 0, 'intDivisionTruncate.sol': 5, @@ -535,6 +549,7 @@ test('Integration test guardConditions.js', function (t) { 'forgottenReturn.sol': 0, 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 1, + 'deleteFromDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 1, 'ERC20.sol': 0, @@ -571,6 +586,7 @@ test('Integration test deleteDynamicArrays.js', function (t) { 'forgottenReturn.sol': 0, 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 2, + 'deleteFromDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, @@ -582,6 +598,43 @@ test('Integration test deleteDynamicArrays.js', function (t) { }) }) +test('Integration test deleteFromDynamicArray.js', function (t) { + t.plan(testFiles.length) + + var module = require('../../src/solidity-analyzer/modules/deleteFromDynamicArray') + + 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, + 'deleteFromDynamicArray.sol': 1, + 'blockLevelCompare.sol': 0, + 'intDivisionTruncate.sol': 0, + 'ERC20.sol': 0, + 'stringBytesLength.sol': 0 + } + + runModuleOnFiles(module, t, (file, report) => { + t.equal(report.length, lengthCheck[file], `${file} has right amount of deleteFromDynamicArray warnings`) + }) +}) + test('Integration test assignAndCompare.js', function (t) { t.plan(testFiles.length) @@ -607,6 +660,7 @@ test('Integration test assignAndCompare.js', function (t) { 'forgottenReturn.sol': 0, 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, + 'deleteFromDynamicArray.sol': 0, 'blockLevelCompare.sol': 8, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, @@ -643,6 +697,7 @@ test('Integration test intDivisionTruncate.js', function (t) { 'forgottenReturn.sol': 0, 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, + 'deleteFromDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 2, 'ERC20.sol': 0, @@ -679,6 +734,7 @@ test('Integration test erc20Decimal.js', function (t) { 'forgottenReturn.sol': 0, 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, + 'deleteFromDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 1, @@ -715,6 +771,7 @@ test('Integration test stringBytesLength.js', function (t) { 'forgottenReturn.sol': 0, 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 0, + 'deleteFromDynamicArray.sol': 0, 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, diff --git a/remix-analyzer/test/analysis/test-contracts/deleteFromDynamicArray.sol b/remix-analyzer/test/analysis/test-contracts/deleteFromDynamicArray.sol new file mode 100644 index 0000000000..a8e0c0eae0 --- /dev/null +++ b/remix-analyzer/test/analysis/test-contracts/deleteFromDynamicArray.sol @@ -0,0 +1,21 @@ +pragma solidity ^0.4.22; +contract arr { + uint[] array = [1,2,3]; + function removeAtIndex() public returns (uint[]) { + delete array[1]; + return array; + } + + function safeRemoveAtIndex(uint index) returns (uint[]) { + if (index >= array.length) return; + + for (uint i = index; i < array.length-1; i++) { + array[i] = array[i+1]; + } + + delete array[array.length-1]; + array.length--; + + return array; + } +} From fac8e2c3e2140d87e2feb4af97f6ca142b5f7f22 Mon Sep 17 00:00:00 2001 From: 0mkar <0mkar@protonmail.com> Date: Wed, 3 Oct 2018 13:04:39 +0530 Subject: [PATCH 8/9] add forLoopIteratesOverDynamicArray tests --- .../staticAnalysisIntegration-test.js | 101 ++++++++++++++---- .../forLoopIteratesOverDynamicArray.sol | 14 +++ 2 files changed, 94 insertions(+), 21 deletions(-) create mode 100644 remix-analyzer/test/analysis/test-contracts/forLoopIteratesOverDynamicArray.sol diff --git a/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js b/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js index 33abf1b937..d08d057a9a 100644 --- a/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js +++ b/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js @@ -33,7 +33,8 @@ var testFiles = [ 'blockLevelCompare.sol', 'intDivisionTruncate.sol', 'ERC20.sol', - 'stringBytesLength.sol' + 'stringBytesLength.sol', + 'forLoopIteratesOverDynamicArray.sol' ] var testFileAsts = {} @@ -72,7 +73,8 @@ test('Integration test thisLocal.js', function (t) { 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, - 'stringBytesLength.sol': 0 + 'stringBytesLength.sol': 0, + 'forLoopIteratesOverDynamicArray.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -109,7 +111,8 @@ test('Integration test checksEffectsInteraction.js', function (t) { 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, - 'stringBytesLength.sol': 0 + 'stringBytesLength.sol': 0, + 'forLoopIteratesOverDynamicArray.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -146,7 +149,8 @@ test('Integration test constantFunctions.js', function (t) { 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, - 'stringBytesLength.sol': 0 + 'stringBytesLength.sol': 0, + 'forLoopIteratesOverDynamicArray.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -183,7 +187,8 @@ test('Integration test inlineAssembly.js', function (t) { 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, - 'stringBytesLength.sol': 0 + 'stringBytesLength.sol': 0, + 'forLoopIteratesOverDynamicArray.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -220,7 +225,8 @@ test('Integration test txOrigin.js', function (t) { 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, - 'stringBytesLength.sol': 0 + 'stringBytesLength.sol': 0, + 'forLoopIteratesOverDynamicArray.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -257,7 +263,8 @@ test('Integration test gasCosts.js', function (t) { 'blockLevelCompare.sol': 1, 'intDivisionTruncate.sol': 1, 'ERC20.sol': 2, - 'stringBytesLength.sol': 1 + 'stringBytesLength.sol': 1, + 'forLoopIteratesOverDynamicArray.sol': 1 } runModuleOnFiles(module, t, (file, report) => { @@ -294,7 +301,8 @@ test('Integration test similarVariableNames.js', function (t) { 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, - 'stringBytesLength.sol': 0 + 'stringBytesLength.sol': 0, + 'forLoopIteratesOverDynamicArray.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -331,7 +339,8 @@ test('Integration test inlineAssembly.js', function (t) { 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, - 'stringBytesLength.sol': 0 + 'stringBytesLength.sol': 0, + 'forLoopIteratesOverDynamicArray.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -368,7 +377,8 @@ test('Integration test blockTimestamp.js', function (t) { 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, - 'stringBytesLength.sol': 0 + 'stringBytesLength.sol': 0, + 'forLoopIteratesOverDynamicArray.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -405,7 +415,8 @@ test('Integration test lowLevelCalls.js', function (t) { 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, - 'stringBytesLength.sol': 0 + 'stringBytesLength.sol': 0, + 'forLoopIteratesOverDynamicArray.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -442,7 +453,8 @@ test('Integration test blockBlockhash.js', function (t) { 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, - 'stringBytesLength.sol': 0 + 'stringBytesLength.sol': 0, + 'forLoopIteratesOverDynamicArray.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -479,7 +491,8 @@ test('Integration test noReturn.js', function (t) { 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, - 'stringBytesLength.sol': 0 + 'stringBytesLength.sol': 0, + 'forLoopIteratesOverDynamicArray.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -516,7 +529,8 @@ test('Integration test selfdestruct.js', function (t) { 'blockLevelCompare.sol': 0, 'ERC20.sol': 0, 'intDivisionTruncate.sol': 5, - 'stringBytesLength.sol': 0 + 'stringBytesLength.sol': 0, + 'forLoopIteratesOverDynamicArray.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -553,7 +567,8 @@ test('Integration test guardConditions.js', function (t) { 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 1, 'ERC20.sol': 0, - 'stringBytesLength.sol': 0 + 'stringBytesLength.sol': 0, + 'forLoopIteratesOverDynamicArray.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -590,7 +605,8 @@ test('Integration test deleteDynamicArrays.js', function (t) { 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, - 'stringBytesLength.sol': 0 + 'stringBytesLength.sol': 0, + 'forLoopIteratesOverDynamicArray.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -627,7 +643,8 @@ test('Integration test deleteFromDynamicArray.js', function (t) { 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, - 'stringBytesLength.sol': 0 + 'stringBytesLength.sol': 0, + 'forLoopIteratesOverDynamicArray.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -664,7 +681,8 @@ test('Integration test assignAndCompare.js', function (t) { 'blockLevelCompare.sol': 8, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, - 'stringBytesLength.sol': 0 + 'stringBytesLength.sol': 0, + 'forLoopIteratesOverDynamicArray.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -701,7 +719,8 @@ test('Integration test intDivisionTruncate.js', function (t) { 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 2, 'ERC20.sol': 0, - 'stringBytesLength.sol': 0 + 'stringBytesLength.sol': 0, + 'forLoopIteratesOverDynamicArray.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -738,7 +757,8 @@ test('Integration test erc20Decimal.js', function (t) { 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 1, - 'stringBytesLength.sol': 0 + 'stringBytesLength.sol': 0, + 'forLoopIteratesOverDynamicArray.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -775,7 +795,8 @@ test('Integration test stringBytesLength.js', function (t) { 'blockLevelCompare.sol': 0, 'intDivisionTruncate.sol': 0, 'ERC20.sol': 0, - 'stringBytesLength.sol': 1 + 'stringBytesLength.sol': 1, + 'forLoopIteratesOverDynamicArray.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -783,6 +804,44 @@ test('Integration test stringBytesLength.js', function (t) { }) }) +test('Integration test forLoopIteratesOverDynamicArray.js', function (t) { + t.plan(testFiles.length) + + var module = require('../../src/solidity-analyzer/modules/forLoopIteratesOverDynamicArray') + + var lengthCheck = { + 'KingOfTheEtherThrone.sol': 0, + 'assembly.sol': 0, + 'ballot.sol': 2, + 'ballot_reentrant.sol': 1, + '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, + 'deleteFromDynamicArray.sol': 0, + 'blockLevelCompare.sol': 0, + 'intDivisionTruncate.sol': 0, + 'ERC20.sol': 0, + 'stringBytesLength.sol': 0, + 'forLoopIteratesOverDynamicArray.sol': 1 + } + + runModuleOnFiles(module, t, (file, report) => { + t.equal(report.length, lengthCheck[file], `${file} has right amount of forLoopIteratesOverDynamicArray warnings`) + }) +}) + // #################### Helpers function runModuleOnFiles (module, t, cb) { var statRunner = new StatRunner() diff --git a/remix-analyzer/test/analysis/test-contracts/forLoopIteratesOverDynamicArray.sol b/remix-analyzer/test/analysis/test-contracts/forLoopIteratesOverDynamicArray.sol new file mode 100644 index 0000000000..16bb69c7e8 --- /dev/null +++ b/remix-analyzer/test/analysis/test-contracts/forLoopIteratesOverDynamicArray.sol @@ -0,0 +1,14 @@ +pragma solidity ^0.4.22; +contract forLoopArr { + uint[] array; + constructor(uint[] _array) { + array = _array; + } + + function shiftArrItem(uint index) returns(uint[]) { + for (uint i = index; i < array.length-1; i++) { + array[i] = array[i+1]; + } + return array; + } +} From f9ab44cf424a8368cf0442005da2acdec8643dd1 Mon Sep 17 00:00:00 2001 From: 0mkar <0mkar@protonmail.com> Date: Wed, 3 Oct 2018 18:43:24 +0530 Subject: [PATCH 9/9] Temp remove failing tests for deleteFromDynamicArray & forLoopIteratesOverDynamicArray --- .../test/analysis/staticAnalysisIntegration-test.js | 2 +- .../test/analysis/test-contracts/deleteFromDynamicArray.sol | 5 +++-- .../test-contracts/forLoopIteratesOverDynamicArray.sol | 3 ++- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js b/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js index d08d057a9a..0d3a67cfdb 100644 --- a/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js +++ b/remix-analyzer/test/analysis/staticAnalysisIntegration-test.js @@ -259,7 +259,7 @@ test('Integration test gasCosts.js', function (t) { 'forgottenReturn.sol': 3, 'selfdestruct.sol': 0, 'deleteDynamicArray.sol': 2, - 'deleteFromDynamicArray.sol': 2, + 'deleteFromDynamicArray.sol': 1, 'blockLevelCompare.sol': 1, 'intDivisionTruncate.sol': 1, 'ERC20.sol': 2, diff --git a/remix-analyzer/test/analysis/test-contracts/deleteFromDynamicArray.sol b/remix-analyzer/test/analysis/test-contracts/deleteFromDynamicArray.sol index a8e0c0eae0..dbe08109bb 100644 --- a/remix-analyzer/test/analysis/test-contracts/deleteFromDynamicArray.sol +++ b/remix-analyzer/test/analysis/test-contracts/deleteFromDynamicArray.sol @@ -6,7 +6,8 @@ contract arr { return array; } - function safeRemoveAtIndex(uint index) returns (uint[]) { + // TODO: deleteFromDynamicArray should not generate warnings if array item is shifted and removed + /* function safeRemoveAtIndex(uint index) returns (uint[]) { if (index >= array.length) return; for (uint i = index; i < array.length-1; i++) { @@ -17,5 +18,5 @@ contract arr { array.length--; return array; - } + } */ } diff --git a/remix-analyzer/test/analysis/test-contracts/forLoopIteratesOverDynamicArray.sol b/remix-analyzer/test/analysis/test-contracts/forLoopIteratesOverDynamicArray.sol index 16bb69c7e8..c8d57c5e2d 100644 --- a/remix-analyzer/test/analysis/test-contracts/forLoopIteratesOverDynamicArray.sol +++ b/remix-analyzer/test/analysis/test-contracts/forLoopIteratesOverDynamicArray.sol @@ -6,7 +6,8 @@ contract forLoopArr { } function shiftArrItem(uint index) returns(uint[]) { - for (uint i = index; i < array.length-1; i++) { + // TODO: for (uint i = index; i < array.length-1; i++) should also generate warning + for (uint i = index; i < array.length; i++) { array[i] = array[i+1]; } return array;