Merge pull request #1242 from Aniket-Engg/fix/#854

[Static analysis] Interaction with different addresses inside the loop handled
pull/5370/head
Aniket 5 years ago committed by GitHub
commit 1360616b2e
  1. 41
      remix-analyzer/src/solidity-analyzer/modules/etherTransferInLoop.js
  2. 24
      remix-analyzer/src/solidity-analyzer/modules/staticAnalysisCommon.js
  3. 1360
      remix-analyzer/test/analysis/staticAnalysisCommon-test.js
  4. 61
      remix-analyzer/test/analysis/staticAnalysisIntegration-test-0.4.24.js
  5. 60
      remix-analyzer/test/analysis/staticAnalysisIntegration-test-0.5.0.js
  6. 31
      remix-analyzer/test/analysis/test-contracts/solidity-v0.4.24/etherTransferInLoop.sol
  7. 30
      remix-analyzer/test/analysis/test-contracts/solidity-v0.4.24/loops.sol
  8. 31
      remix-analyzer/test/analysis/test-contracts/solidity-v0.5/etherTransferInLoop.sol
  9. 30
      remix-analyzer/test/analysis/test-contracts/solidity-v0.5/loops.sol

@ -0,0 +1,41 @@
var name = 'Ether transfer in a loop: '
var desc = 'Avoid transferring Ether to multiple addresses in a loop'
var categories = require('./categories')
var common = require('./staticAnalysisCommon')
function etherTransferInLoop () {
this.relevantNodes = []
}
etherTransferInLoop.prototype.visit = function (node) {
if (common.isLoop(node)) {
var transferNodes = []
var loopBlockStartIndex = common.getLoopBlockStartIndex(node)
if (common.isBlock(node.children[loopBlockStartIndex])) {
transferNodes = node.children[loopBlockStartIndex].children
.filter(child => (common.isExpressionStatement(child) &&
child.children[0].name === 'FunctionCall' &&
common.isTransfer(child.children[0].children[0])))
if (transferNodes.length > 0) {
this.relevantNodes.push(...transferNodes)
}
}
}
}
etherTransferInLoop.prototype.report = function (compilationResults) {
return this.relevantNodes.map((node) => {
return {
warning: 'Ether payout should not be done in a loop: 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. If required then make sure that number of iterations are low and you trust each address involved.',
location: node.src,
more: 'https://solidity.readthedocs.io/en/latest/security-considerations.html#gas-limit-and-loops'
}
})
}
module.exports = {
name: name,
description: desc,
category: categories.GAS,
Module: etherTransferInLoop
}

@ -410,6 +410,16 @@ function getUnAssignedTopLevelBinOps (subScope) {
return subScope.children.filter(isBinaryOpInExpression)
}
function getLoopBlockStartIndex (node) {
if (isLoop(node)) {
if (nodeType(node, exactMatch(nodeTypes.FORSTATEMENT))) {
return 3 // For 'for' loop
} else {
return 1 // For 'while' and 'do-while' loop
}
}
}
// #################### Trivial Node Identification
function isFunctionDefinition (node) {
@ -947,6 +957,17 @@ function isBytesLengthCheck (node) {
return isMemberAccess(node, exactMatch(util.escapeRegExp(basicTypes.UINT)), undefined, util.escapeRegExp('bytes *'), 'length')
}
/**
* True if it is a loop
* @node {ASTNode} some AstNode
* @return {bool}
*/
function isLoop (node) {
return nodeType(node, exactMatch(nodeTypes.FORSTATEMENT)) ||
nodeType(node, exactMatch(nodeTypes.WHILESTATEMENT)) ||
nodeType(node, exactMatch(nodeTypes.DOWHILESTATEMENT))
}
/**
* True if it is a 'for' loop
* @node {ASTNode} some AstNode
@ -1073,6 +1094,7 @@ module.exports = {
getFunctionOrModifierDefinitionParameterPart: getFunctionOrModifierDefinitionParameterPart,
getFunctionOrModifierDefinitionReturnParameterPart: getFunctionOrModifierDefinitionReturnParameterPart,
getUnAssignedTopLevelBinOps: getUnAssignedTopLevelBinOps,
getLoopBlockStartIndex: getLoopBlockStartIndex,
// #################### Complex Node Identification
isDeleteOfDynamicArray: isDeleteOfDynamicArray,
@ -1117,6 +1139,7 @@ module.exports = {
isIntDivision: isIntDivision,
isStringToBytesConversion: isStringToBytesConversion,
isBytesLengthCheck: isBytesLengthCheck,
isLoop: isLoop,
isForLoop: isForLoop,
// #################### Trivial Node Identification
@ -1136,6 +1159,7 @@ module.exports = {
isNewExpression: isNewExpression,
isReturn: isReturn,
isStatement: isStatement,
isExpressionStatement: isExpressionStatement,
isBlock: isBlock,
// #################### Constants

File diff suppressed because it is too large Load Diff

@ -37,6 +37,7 @@ var testFiles = [
'intDivisionTruncate.sol',
'ERC20.sol',
'stringBytesLength.sol',
'etherTransferInLoop.sol',
'forLoopIteratesOverDynamicArray.sol'
]
@ -77,6 +78,7 @@ test('Integration test thisLocal.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -115,6 +117,7 @@ test('Integration test checksEffectsInteraction.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -153,6 +156,7 @@ test('Integration test constantFunctions.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -191,6 +195,7 @@ test('Integration test inlineAssembly.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -229,6 +234,7 @@ test('Integration test txOrigin.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -267,6 +273,7 @@ test('Integration test gasCosts.js', function (t) {
'intDivisionTruncate.sol': 1,
'ERC20.sol': 2,
'stringBytesLength.sol': 1,
'etherTransferInLoop.sol': 3,
'forLoopIteratesOverDynamicArray.sol': 1
}
@ -305,6 +312,7 @@ test('Integration test similarVariableNames.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -343,6 +351,7 @@ test('Integration test inlineAssembly.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -381,6 +390,7 @@ test('Integration test blockTimestamp.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -419,6 +429,7 @@ test('Integration test lowLevelCalls.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -457,6 +468,7 @@ test('Integration test blockBlockhash.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -495,6 +507,7 @@ test('Integration test noReturn.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -533,6 +546,7 @@ test('Integration test selfdestruct.js', function (t) {
'ERC20.sol': 0,
'intDivisionTruncate.sol': 5,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -571,6 +585,7 @@ test('Integration test guardConditions.js', function (t) {
'intDivisionTruncate.sol': 1,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -609,6 +624,7 @@ test('Integration test deleteDynamicArrays.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -647,6 +663,7 @@ test('Integration test deleteFromDynamicArray.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -685,6 +702,7 @@ test('Integration test assignAndCompare.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -723,6 +741,7 @@ test('Integration test intDivisionTruncate.js', function (t) {
'intDivisionTruncate.sol': 2,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -761,6 +780,7 @@ test('Integration test erc20Decimal.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 1,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -799,6 +819,7 @@ test('Integration test stringBytesLength.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 1,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -807,6 +828,45 @@ test('Integration test stringBytesLength.js', function (t) {
})
})
test('Integration test etherTransferInLoop.js', function (t) {
t.plan(testFiles.length)
var module = require('../../src/solidity-analyzer/modules/etherTransferInLoop')
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': 0,
'blockLevelCompare.sol': 0,
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 3,
'forLoopIteratesOverDynamicArray.sol': 0
}
runModuleOnFiles(module, t, (file, report) => {
t.equal(report.length, lengthCheck[file], `${file} has right amount of etherTransferInLoop warnings`)
})
})
test('Integration test forLoopIteratesOverDynamicArray.js', function (t) {
t.plan(testFiles.length)
@ -837,6 +897,7 @@ test('Integration test forLoopIteratesOverDynamicArray.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 1
}

@ -37,6 +37,7 @@ var testFiles = [
'intDivisionTruncate.sol',
'ERC20.sol',
'stringBytesLength.sol',
'etherTransferInLoop.sol',
'forLoopIteratesOverDynamicArray.sol'
]
@ -77,6 +78,7 @@ test('Integration test thisLocal.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -115,6 +117,7 @@ test('Integration test checksEffectsInteraction.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -153,6 +156,7 @@ test('Integration test constantFunctions.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -191,6 +195,7 @@ test('Integration test inlineAssembly.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -229,6 +234,7 @@ test('Integration test txOrigin.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -267,6 +273,7 @@ test('Integration test gasCosts.js', function (t) {
'intDivisionTruncate.sol': 1,
'ERC20.sol': 2,
'stringBytesLength.sol': 1,
'etherTransferInLoop.sol': 3,
'forLoopIteratesOverDynamicArray.sol': 1
}
@ -305,6 +312,7 @@ test('Integration test similarVariableNames.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -343,6 +351,7 @@ test('Integration test inlineAssembly.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -381,6 +390,7 @@ test('Integration test blockTimestamp.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -419,6 +429,7 @@ test('Integration test lowLevelCalls.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -457,6 +468,7 @@ test('Integration test blockBlockhash.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -538,6 +550,7 @@ test('Integration test selfdestruct.js', function (t) {
'ERC20.sol': 0,
'intDivisionTruncate.sol': 5,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -576,6 +589,7 @@ test('Integration test guardConditions.js', function (t) {
'intDivisionTruncate.sol': 1,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -614,6 +628,7 @@ test('Integration test deleteDynamicArrays.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -652,6 +667,7 @@ test('Integration test deleteFromDynamicArray.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -690,6 +706,7 @@ test('Integration test assignAndCompare.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -728,6 +745,7 @@ test('Integration test intDivisionTruncate.js', function (t) {
'intDivisionTruncate.sol': 2,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -766,6 +784,7 @@ test('Integration test erc20Decimal.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 1,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -804,6 +823,7 @@ test('Integration test stringBytesLength.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 1,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 0
}
@ -812,6 +832,45 @@ test('Integration test stringBytesLength.js', function (t) {
})
})
test('Integration test etherTransferInLoop.js', function (t) {
t.plan(testFiles.length)
var module = require('../../src/solidity-analyzer/modules/etherTransferInLoop')
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': 0,
'blockLevelCompare.sol': 0,
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 3,
'forLoopIteratesOverDynamicArray.sol': 0
}
runModuleOnFiles(module, t, (file, report) => {
t.equal(report.length, lengthCheck[file], `${file} has right amount of etherTransferInLoop warnings`)
})
})
test('Integration test forLoopIteratesOverDynamicArray.js', function (t) {
t.plan(testFiles.length)
@ -842,6 +901,7 @@ test('Integration test forLoopIteratesOverDynamicArray.js', function (t) {
'intDivisionTruncate.sol': 0,
'ERC20.sol': 0,
'stringBytesLength.sol': 0,
'etherTransferInLoop.sol': 0,
'forLoopIteratesOverDynamicArray.sol': 1
}

@ -0,0 +1,31 @@
pragma solidity ^0.4.24;
contract etherTransferInLoop {
address owner;
constructor() public {
owner = msg.sender;
}
function transferInForLoop(uint index) public {
for (uint i = index; i < 10; i++) {
owner.transfer(i);
}
}
function transferInWhileLoop(uint index) public {
uint i = index;
while (i < 10) {
owner.transfer(i);
i++;
}
}
function transferInDoWhileLoop(uint index) public {
uint i = index;
do {
owner.transfer(i);
i++;
} while (i < 10);
}
}

@ -0,0 +1,30 @@
pragma solidity ^0.4.9;
contract loops {
uint[] array;
constructor(uint[] memory _array) public {
array = _array;
}
function fnWithForLoop(uint index) public {
for (uint i = index; i < 10; i++) {
array.push(i);
}
}
function fnWithWhileLoop(uint index) public {
uint i = index;
while (i < 10) {
array.push(i);
i++;
}
}
function fnWithDoWhileLoop(uint index) public {
uint i = index;
do{
array.push(i);
i++;
}while (i < 10);
}
}

@ -0,0 +1,31 @@
pragma solidity >=0.4.9 <0.6.0;
contract etherTransferInLoop {
address payable owner;
constructor() public {
owner = msg.sender;
}
function transferInForLoop(uint index) public {
for (uint i = index; i < 10; i++) {
owner.transfer(i);
}
}
function transferInWhileLoop(uint index) public {
uint i = index;
while (i < 10) {
owner.transfer(i);
i++;
}
}
function transferInDoWhileLoop(uint index) public {
uint i = index;
do {
owner.transfer(i);
i++;
} while (i < 10);
}
}

@ -0,0 +1,30 @@
pragma solidity >=0.4.9 <0.6.0;
contract loops {
uint[] array;
constructor(uint[] memory _array) public {
array = _array;
}
function fnWithForLoop(uint index) public {
for (uint i = index; i < 10; i++) {
array.push(i);
}
}
function fnWithWhileLoop(uint index) public {
uint i = index;
while (i < 10) {
array.push(i);
i++;
}
}
function fnWithDoWhileLoop(uint index) public {
uint i = index;
do{
array.push(i);
i++;
}while (i < 10);
}
}
Loading…
Cancel
Save