analyzer warnings reviewed

pull/5370/head
aniket-engg 5 years ago committed by Aniket
parent e793a28f5b
commit 08c0f2a9bd
  1. 2
      remix-analyzer/src/solidity-analyzer/modules/assignAndCompare.ts
  2. 4
      remix-analyzer/src/solidity-analyzer/modules/blockBlockhash.ts
  3. 6
      remix-analyzer/src/solidity-analyzer/modules/blockTimestamp.ts
  4. 2
      remix-analyzer/src/solidity-analyzer/modules/checksEffectsInteraction.ts
  5. 2
      remix-analyzer/src/solidity-analyzer/modules/deleteDynamicArrays.ts
  6. 2
      remix-analyzer/src/solidity-analyzer/modules/deleteFromDynamicArray.ts
  7. 4
      remix-analyzer/src/solidity-analyzer/modules/erc20Decimals.ts
  8. 2
      remix-analyzer/src/solidity-analyzer/modules/etherTransferInLoop.ts
  9. 2
      remix-analyzer/src/solidity-analyzer/modules/forLoopIteratesOverDynamicArray.ts
  10. 4
      remix-analyzer/src/solidity-analyzer/modules/gasCosts.ts
  11. 2
      remix-analyzer/src/solidity-analyzer/modules/guardConditions.ts
  12. 2
      remix-analyzer/src/solidity-analyzer/modules/inlineAssembly.ts
  13. 16
      remix-analyzer/src/solidity-analyzer/modules/lowLevelCalls.ts
  14. 4
      remix-analyzer/src/solidity-analyzer/modules/selfdestruct.ts
  15. 2
      remix-analyzer/src/solidity-analyzer/modules/stringBytesLength.ts
  16. 2
      remix-analyzer/src/solidity-analyzer/modules/thisLocal.ts

@ -18,7 +18,7 @@ export default class assignAndCompare implements AnalyzerModule {
report (compilationResults: CompilationResult): ReportObj[] { report (compilationResults: CompilationResult): ReportObj[] {
return this.warningNodes.map((item, i) => { return this.warningNodes.map((item, i) => {
return { return {
warning: 'A binary operation yields a value that is not used in the following. This is often caused by confusing assignment (=) and comparison (==).', warning: 'A binary operation yields a value that is not used further. This is often caused by confusing assignment (=) and comparison (==).',
location: item.src location: item.src
} }
}) })

@ -17,9 +17,9 @@ export default class blockBlockhash implements AnalyzerModule {
report (compilationResults: CompilationResult): ReportObj[] { report (compilationResults: CompilationResult): ReportObj[] {
return this.warningNodes.map((item, i) => { return this.warningNodes.map((item, i) => {
return { return {
warning: `use of "blockhash": "blockhash" is used to access the last 256 block hashes. warning: `Use of "blockhash": "blockhash(uint blockNumber)" is used to access the last 256 block hashes.
A miner computes the block hash by "summing up" the information in the current block mined. A miner computes the block hash by "summing up" the information in the current block mined.
By "summing up" the information in a clever way a miner can try to influence the outcome of a transaction in the current block. By "summing up" the information cleverly, a miner can try to influence the outcome of a transaction in the current block.
This is especially easy if there are only a small number of equally likely outcomes.`, This is especially easy if there are only a small number of equally likely outcomes.`,
location: item.src location: item.src
} }

@ -20,14 +20,14 @@ export default class blockTimestamp implements AnalyzerModule {
report (compilationResults: CompilationResult): ReportObj[] { report (compilationResults: CompilationResult): ReportObj[] {
return this.warningNowNodes.map((item, i) => { return this.warningNowNodes.map((item, i) => {
return { return {
warning: `use of "now": "now" does not mean current time. Now is an alias for block.timestamp. warning: `Use of "now": "now" does not mean current time. "now" is an alias for "block.timestamp".
Block.timestamp can be influenced by miners to a certain degree, be careful.`, "block.timestamp" can be influenced by miners to a certain degree, be careful.`,
location: item.src, location: item.src,
more: 'http://solidity.readthedocs.io/en/develop/frequently-asked-questions.html#are-timestamps-now-block-timestamp-reliable' more: 'http://solidity.readthedocs.io/en/develop/frequently-asked-questions.html#are-timestamps-now-block-timestamp-reliable'
} }
}).concat(this.warningblockTimestampNodes.map((item, i) => { }).concat(this.warningblockTimestampNodes.map((item, i) => {
return { return {
warning: `use of "block.timestamp": "block.timestamp" can be influenced by miners to a certain degree. warning: `Use of "block.timestamp": "block.timestamp" can be influenced by miners to a certain degree.
That means that a miner can "choose" the block.timestamp, to a certain degree, to change the outcome of a transaction in the mined block.`, That means that a miner can "choose" the block.timestamp, to a certain degree, to change the outcome of a transaction in the mined block.`,
location: item.src, location: item.src,
more: 'http://solidity.readthedocs.io/en/develop/frequently-asked-questions.html#are-timestamps-now-block-timestamp-reliable' more: 'http://solidity.readthedocs.io/en/develop/frequently-asked-questions.html#are-timestamps-now-block-timestamp-reliable'

@ -46,7 +46,7 @@ export default class checksEffectsInteraction implements AnalyzerModule {
let comments: string = (hasModifiers) ? 'Note: Modifiers are currently not considered by this static analysis.' : '' let comments: string = (hasModifiers) ? 'Note: Modifiers are currently not considered by this static analysis.' : ''
comments += (multipleContractsWithSameName) ? 'Note: Import aliases are currently not supported by this static analysis.' : '' comments += (multipleContractsWithSameName) ? 'Note: Import aliases are currently not supported by this static analysis.' : ''
warnings.push({ warnings.push({
warning: `Potential Violation of Checks-Effects-Interaction pattern in ${funcName}: Could potentially lead to re-entrancy vulnerability. ${comments}`, warning: `Potential violation of Checks-Effects-Interaction pattern in ${funcName}: Could potentially lead to re-entrancy vulnerability. ${comments}`,
location: func.node['src'], location: func.node['src'],
more: 'http://solidity.readthedocs.io/en/develop/security-considerations.html#re-entrancy' more: 'http://solidity.readthedocs.io/en/develop/security-considerations.html#re-entrancy'
}) })

@ -17,7 +17,7 @@ export default class deleteDynamicArrays implements AnalyzerModule {
report (compilationResults: CompilationResult): ReportObj[] { report (compilationResults: CompilationResult): ReportObj[] {
return this.rel.map((node) => { return this.rel.map((node) => {
return { return {
warning: 'The “delete” operation when applied to a dynamically sized array in Solidity generates code to delete each of the elements contained. If the array is large, this operation can surpass the block gas limit and raise an OOG exception. Also nested dynamically sized objects can produce the same results.', warning: `The "delete" operation when applied to a dynamically sized array in Solidity generates code to delete each of the elements contained. If the array is large, this operation can surpass the block gas limit and raise an OOG exception. Also nested dynamically sized objects can produce the same results.`,
location: node.src, location: node.src,
more: 'http://solidity.readthedocs.io/en/latest/types.html?highlight=array#delete' more: 'http://solidity.readthedocs.io/en/latest/types.html?highlight=array#delete'
} }

@ -17,7 +17,7 @@ export default class deleteFromDynamicArray implements AnalyzerModule {
report (compilationResults: CompilationResult): ReportObj[] { report (compilationResults: CompilationResult): ReportObj[] {
return this.relevantNodes.map((node) => { return this.relevantNodes.map((node) => {
return { 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', 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.`,
location: node.src, location: node.src,
more: 'https://github.com/miguelmota/solidity-idiosyncrasies' more: 'https://github.com/miguelmota/solidity-idiosyncrasies'
} }

@ -32,7 +32,7 @@ export default class erc20Decimals implements AnalyzerModule {
if (decimalsVar.length > 0) { if (decimalsVar.length > 0) {
for (const node of decimalsVar) { for (const node of decimalsVar) {
warnings.push({ warnings.push({
warning: `ERC20 contract's 'decimals' variable should be 'uint8' type`, warning: `ERC20 contract's "decimals" variable should be "uint8" type`,
location: node.src, location: node.src,
more: ' https://eips.ethereum.org/EIPS/eip-20' more: ' https://eips.ethereum.org/EIPS/eip-20'
}) })
@ -40,7 +40,7 @@ export default class erc20Decimals implements AnalyzerModule {
} else if (decimalsFun.length > 0) { } else if (decimalsFun.length > 0) {
for (const fn of decimalsFun) { for (const fn of decimalsFun) {
warnings.push({ warnings.push({
warning: `ERC20 contract's 'decimals' function should have 'uint8' as return type`, warning: `ERC20 contract's "decimals" function should have "uint8" as return type`,
location: fn.node.src, location: fn.node.src,
more: ' https://eips.ethereum.org/EIPS/eip-20' more: ' https://eips.ethereum.org/EIPS/eip-20'
}) })

@ -29,7 +29,7 @@ export default class etherTransferInLoop implements AnalyzerModule {
report (compilationResults: CompilationResult): ReportObj[] { report (compilationResults: CompilationResult): ReportObj[] {
return this.relevantNodes.map((node) => { return this.relevantNodes.map((node) => {
return { 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.', 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, location: node.src,
more: 'https://solidity.readthedocs.io/en/latest/security-considerations.html#gas-limit-and-loops' more: 'https://solidity.readthedocs.io/en/latest/security-considerations.html#gas-limit-and-loops'
} }

@ -23,7 +23,7 @@ export default class forLoopIteratesOverDynamicArray implements AnalyzerModule {
report (compilationResults: CompilationResult): ReportObj[] { report (compilationResults: CompilationResult): ReportObj[] {
return this.relevantNodes.map((node) => { return this.relevantNodes.map((node) => {
return { 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.', 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. \n 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, location: node.src,
more: 'http://solidity.readthedocs.io/en/v0.4.24/security-considerations.html#gas-limit-and-loops' more: 'http://solidity.readthedocs.io/en/v0.4.24/security-considerations.html#gas-limit-and-loops'
} }

@ -48,7 +48,7 @@ export default class gasCosts implements AnalyzerModule {
}) })
} else { } else {
report.push({ report.push({
warning: `Gas requirement of function ${contractName}.${method.name} ${methodGas.msg}. warning: `Gas requirement of function ${contractName}.${method.name} ${methodGas.msg}:
If the gas requirement of a function is higher than the block gas limit, it cannot be executed. If the gas requirement of a function is higher than the block gas limit, it cannot be executed.
Please avoid loops in your functions or actions that modify large areas of storage Please avoid loops in your functions or actions that modify large areas of storage
(this includes clearing or copying arrays in storage)`, (this includes clearing or copying arrays in storage)`,
@ -75,7 +75,7 @@ export default class gasCosts implements AnalyzerModule {
} }
} else { } else {
const gas: string = contract.evm.gasEstimates.external[methodSignature] const gas: string = contract.evm.gasEstimates.external[methodSignature]
const gasString: string = gas === null ? 'unknown or not constant' : 'high: ' + gas const gasString: string = gas === null ? 'unknown or not constant' : 'is ' + gas
if (gas === null || parseInt(gas) >= 3000000 || gas === 'infinite') { if (gas === null || parseInt(gas) >= 3000000 || gas === 'infinite') {
return { return {
isInfinite: true, isInfinite: true,

@ -17,7 +17,7 @@ export default class guardConditions implements AnalyzerModule {
report (compilationResults: CompilationResult): ReportObj[] { report (compilationResults: CompilationResult): ReportObj[] {
return this.guards.map((node) => { return this.guards.map((node) => {
return { 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.`, 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.`,
location: node.src, location: node.src,
more: 'http://solidity.readthedocs.io/en/develop/control-structures.html#error-handling-assert-require-revert-and-exceptions' more: 'http://solidity.readthedocs.io/en/develop/control-structures.html#error-handling-assert-require-revert-and-exceptions'
} }

@ -16,7 +16,7 @@ export default class inlineAssembly implements AnalyzerModule {
report (compilationResults: CompilationResult): ReportObj[] { report (compilationResults: CompilationResult): ReportObj[] {
return this.inlineAssNodes.map((node) => { return this.inlineAssNodes.map((node) => {
return { return {
warning: `CAUTION: The Contract uses inline assembly, this is only advised in rare cases. warning: `The Contract uses inline assembly, this is only advised in rare cases.
Additionally static analysis modules do not parse inline Assembly, this can lead to wrong analysis results.`, Additionally static analysis modules do not parse inline Assembly, this can lead to wrong analysis results.`,
location: node.src, location: node.src,
more: 'http://solidity.readthedocs.io/en/develop/assembly.html#solidity-assembly' more: 'http://solidity.readthedocs.io/en/develop/assembly.html#solidity-assembly'

@ -39,26 +39,26 @@ export default class lowLevelCalls implements AnalyzerModule {
let morehref: string = '' let morehref: string = ''
switch (item.type) { switch (item.type) {
case lowLevelCallTypes.CALL: case lowLevelCallTypes.CALL:
text = `use of "call": the use of low level "call" should be avoided whenever possible. text = `Use of "call": should be avoided whenever possible.
It can lead to unexpected behavior if return value is not handled properly. It can lead to unexpected behavior if return value is not handled properly.
Please use Direct Calls via specifying the called contract's interface.` Please use Direct Calls via specifying the called contract's interface.`
morehref = 'http://solidity.readthedocs.io/en/develop/control-structures.html?#external-function-calls' morehref = 'http://solidity.readthedocs.io/en/develop/control-structures.html?#external-function-calls'
// http://solidity.readthedocs.io/en/develop/frequently-asked-questions.html?#why-is-the-low-level-function-call-less-favorable-than-instantiating-a-contract-with-a-variable-contractb-b-and-executing-its-functions-b-dosomething // http://solidity.readthedocs.io/en/develop/frequently-asked-questions.html?#why-is-the-low-level-function-call-less-favorable-than-instantiating-a-contract-with-a-variable-contractb-b-and-executing-its-functions-b-dosomething
break break
case lowLevelCallTypes.CALLCODE: case lowLevelCallTypes.CALLCODE:
text = `use of "callcode": the use of low level "callcode" should be avoided whenever possible. text = `Use of "callcode": should be avoided whenever possible.
External code that is called can change the state of the calling contract and send ether from the caller's balance. External code, that is called can change the state of the calling contract and send ether from the caller's balance.
If this is wanted behaviour use the Solidity library feature if possible.` If this is wanted behaviour, use the Solidity library feature if possible.`
morehref = 'http://solidity.readthedocs.io/en/develop/contracts.html#libraries' morehref = 'http://solidity.readthedocs.io/en/develop/contracts.html#libraries'
break break
case lowLevelCallTypes.DELEGATECALL: case lowLevelCallTypes.DELEGATECALL:
text = `use of "delegatecall": the use of low level "delegatecall" should be avoided whenever possible. text = `Use of "delegatecall": should be avoided whenever possible.
External code that is called can change the state of the calling contract and send ether from the caller's balance. External code, that is called can change the state of the calling contract and send ether from the caller's balance.
If this is wanted behaviour use the Solidity library feature if possible.` If this is wanted behaviour, use the Solidity library feature if possible.`
morehref = 'http://solidity.readthedocs.io/en/develop/contracts.html#libraries' morehref = 'http://solidity.readthedocs.io/en/develop/contracts.html#libraries'
break break
case lowLevelCallTypes.SEND: case lowLevelCallTypes.SEND:
text = `use of "send": "send" does not throw an exception when not successful, make sure you deal with the failure case accordingly. text = `Use of "send": "send" does not throw an exception when not successful, make sure you deal with the failure case accordingly.
Use "transfer" whenever failure of the ether transfer should rollback the whole transaction. Use "transfer" whenever failure of the ether transfer should rollback the whole transaction.
Note: if you "send/transfer" ether to a contract the fallback function is called, the callees fallback function is very limited due to the limited amount of gas provided by "send/transfer". Note: if you "send/transfer" ether to a contract the fallback function is called, the callees fallback function is very limited due to the limited amount of gas provided by "send/transfer".
No state changes are possible but the callee can log the event or revert the transfer. "send/transfer" is syntactic sugar for a "call" to the fallback function with 2300 gas and a specified ether value.` No state changes are possible but the callee can log the event or revert the transfer. "send/transfer" is syntactic sugar for a "call" to the fallback function with 2300 gas and a specified ether value.`

@ -26,7 +26,7 @@ export default class selfdestruct implements AnalyzerModule {
func.relevantNodes.forEach((node) => { func.relevantNodes.forEach((node) => {
if (isSelfdestructCall(node)) { if (isSelfdestructCall(node)) {
warnings.push({ warnings.push({
warning: 'Use of selfdestruct: can block calling contracts unexpectedly. Be especially careful if this contract is planned to be used by other contracts (i.e. library contracts, interactions). Selfdestruction of the callee contract can leave callers in an inoperable state.', warning: `Use of selfdestruct: Can block calling contracts unexpectedly. Be especially careful if this contract is planned to be used by other contracts (i.e. library contracts, interactions). Selfdestruction of the callee contract can leave callers in an inoperable state.`,
location: node.src, location: node.src,
more: 'https://paritytech.io/blog/security-alert.html' more: 'https://paritytech.io/blog/security-alert.html'
}) })
@ -34,7 +34,7 @@ export default class selfdestruct implements AnalyzerModule {
} }
if (isStatement(node) && hasSelf) { if (isStatement(node) && hasSelf) {
warnings.push({ warnings.push({
warning: 'Use of selfdestruct: No code after selfdestruct is executed. Selfdestruct is a terminal.', warning: `Use of selfdestruct: No code after selfdestruct is executed. Selfdestruct is a terminal.`,
location: node.src, location: node.src,
more: 'http://solidity.readthedocs.io/en/develop/introduction-to-smart-contracts.html#self-destruct' more: 'http://solidity.readthedocs.io/en/develop/introduction-to-smart-contracts.html#self-destruct'
}) })

@ -20,7 +20,7 @@ export default class stringBytesLength implements AnalyzerModule {
report (compilationResults: CompilationResult): ReportObj[] { report (compilationResults: CompilationResult): ReportObj[] {
if (this.stringToBytesConversions.length > 0 && this.bytesLengthChecks.length > 0) { if (this.stringToBytesConversions.length > 0 && this.bytesLengthChecks.length > 0) {
return [{ return [{
warning: 'Bytes and string length are not the same since strings are assumed to be UTF-8 encoded (according to the ABI defintion) therefore one character is not nessesarily encoded in one byte of data.', warning: `"bytes" and "string" lengths are not the same since strings are assumed to be UTF-8 encoded (according to the ABI defintion) therefore one character is not nessesarily encoded in one byte of data.`,
location: this.bytesLengthChecks[0].src, location: this.bytesLengthChecks[0].src,
more: 'https://github.com/ethereum/wiki/wiki/Ethereum-Contract-ABI#argument-encoding' more: 'https://github.com/ethereum/wiki/wiki/Ethereum-Contract-ABI#argument-encoding'
}] }]

@ -17,7 +17,7 @@ export default class thisLocal implements AnalyzerModule {
report (compilationResults: CompilationResult): ReportObj[] { report (compilationResults: CompilationResult): ReportObj[] {
return this.warningNodes.map(function (item, i) { return this.warningNodes.map(function (item, i) {
return { return {
warning: 'Use of "this" for local functions: Never use this to call functions in the same contract, it only consumes more gas than normal local calls.', warning: `Use of "this" for local functions: Never use "this" to call functions in the same contract, it only consumes more gas than normal local calls.`,
location: item.src, location: item.src,
more: 'http://solidity.readthedocs.io/en/develop/control-structures.html#external-function-calls' more: 'http://solidity.readthedocs.io/en/develop/control-structures.html#external-function-calls'
} }

Loading…
Cancel
Save