Unverified Commit b93b3577 authored by Aniket's avatar Aniket Committed by GitHub

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

[Static analysis] Interaction with different addresses inside the loop handled
parents 7ea3f1df 376baccd
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) {
......@@ -948,6 +958,17 @@ function isBytesLengthCheck (node) {
}
/**
* 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
* @return {bool}
......@@ -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
......
......@@ -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
}
......
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);
}
}
\ No newline at end of file
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);
}
}
\ No newline at end of file
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);
}
}
\ No newline at end of file
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);
}
}
\ No newline at end of file
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment