From 3d09878bc598d323e94380106cad6a290a7a1260 Mon Sep 17 00:00:00 2001 From: TilakMaddy Date: Wed, 31 Jul 2024 14:13:38 +0530 Subject: [PATCH] cli/reportgen --- .../adhoc-sol-files-highs-only-report.json | 3 +- reports/report.json | 98 ++++++++++- reports/report.md | 107 +++++++++++- reports/report.sarif | 152 ++++++++++++++++++ 4 files changed, 347 insertions(+), 13 deletions(-) diff --git a/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index 801747b5a..4975fc846 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -193,6 +193,7 @@ "signed-storage-array", "weak-randomness", "pre-declared-local-variable-usage", - "delete-nested-mapping" + "delete-nested-mapping", + "msg-value-in-loop" ] } \ No newline at end of file diff --git a/reports/report.json b/reports/report.json index 3380ece53..e67ce022b 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 73, - "total_sloc": 1996 + "total_source_units": 74, + "total_sloc": 2042 }, "files_details": { "files_details": [ @@ -121,6 +121,10 @@ "file_path": "src/MisusedBoolean.sol", "n_sloc": 67 }, + { + "file_path": "src/MsgValueInLoop.sol", + "n_sloc": 46 + }, { "file_path": "src/MultipleConstructorSchemes.sol", "n_sloc": 10 @@ -300,7 +304,7 @@ ] }, "issue_count": { - "high": 32, + "high": 33, "low": 25 }, "high_issues": { @@ -1782,6 +1786,37 @@ "src_char": "426:25" } ] + }, + { + "title": "Loop contains `msg.value`.", + "description": "Do not use `msg.value` in loops since it may be misleading when calculating the total amount.It's recommended to prepare additional variables outside the loop to do the required calculation.", + "detector_name": "msg-value-in-loop", + "instances": [ + { + "contract_path": "src/MsgValueInLoop.sol", + "line_no": 10, + "src": "249:107", + "src_char": "249:107" + }, + { + "contract_path": "src/MsgValueInLoop.sol", + "line_no": 21, + "src": "530:94", + "src_char": "530:94" + }, + { + "contract_path": "src/MsgValueInLoop.sol", + "line_no": 37, + "src": "955:93", + "src_char": "955:93" + }, + { + "contract_path": "src/MsgValueInLoop.sol", + "line_no": 54, + "src": "1382:97", + "src_char": "1382:97" + } + ] } ] }, @@ -2049,6 +2084,12 @@ "src": "0:24", "src_char": "0:24" }, + { + "contract_path": "src/MsgValueInLoop.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/PreDeclaredVarUsage.sol", "line_no": 2, @@ -2183,6 +2224,30 @@ "src": "129:9", "src_char": "129:9" }, + { + "contract_path": "src/MsgValueInLoop.sol", + "line_no": 8, + "src": "168:3", + "src_char": "168:3" + }, + { + "contract_path": "src/MsgValueInLoop.sol", + "line_no": 19, + "src": "449:3", + "src_char": "449:3" + }, + { + "contract_path": "src/MsgValueInLoop.sol", + "line_no": 34, + "src": "849:3", + "src_char": "849:3" + }, + { + "contract_path": "src/MsgValueInLoop.sol", + "line_no": 51, + "src": "1273:3", + "src_char": "1273:3" + }, { "contract_path": "src/StateVariables.sol", "line_no": 47, @@ -2801,6 +2866,12 @@ "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/MsgValueInLoop.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/StateVariables.sol", "line_no": 2, @@ -3327,6 +3398,24 @@ "src": "693:12", "src_char": "693:12" }, + { + "contract_path": "src/MsgValueInLoop.sol", + "line_no": 26, + "src": "645:8", + "src_char": "645:8" + }, + { + "contract_path": "src/MsgValueInLoop.sol", + "line_no": 43, + "src": "1069:8", + "src_char": "1069:8" + }, + { + "contract_path": "src/MsgValueInLoop.sol", + "line_no": 60, + "src": "1500:8", + "src_char": "1500:8" + }, { "contract_path": "src/SendEtherNoChecks.sol", "line_no": 9, @@ -3725,6 +3814,7 @@ "public-variable-read-in-external-context", "weak-randomness", "pre-declared-local-variable-usage", - "delete-nested-mapping" + "delete-nested-mapping", + "msg-value-in-loop" ] } \ No newline at end of file diff --git a/reports/report.md b/reports/report.md index d6af919d3..664cfe12a 100644 --- a/reports/report.md +++ b/reports/report.md @@ -40,6 +40,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [H-30: Weak Randomness](#h-30-weak-randomness) - [H-31: Usage of variable before declaration.](#h-31-usage-of-variable-before-declaration) - [H-32: Deletion from a nested mappping.](#h-32-deletion-from-a-nested-mappping) + - [H-33: Loop contains `msg.value`.](#h-33-loop-contains-msgvalue) - [Low Issues](#low-issues) - [L-1: Centralization Risk for trusted owners](#l-1-centralization-risk-for-trusted-owners) - [L-2: Solmate's SafeTransferLib does not check for token contract's existence](#l-2-solmates-safetransferlib-does-not-check-for-token-contracts-existence) @@ -74,8 +75,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 73 | -| Total nSLOC | 1996 | +| .sol Files | 74 | +| Total nSLOC | 2042 | ## Files Details @@ -111,6 +112,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/InternalFunctions.sol | 22 | | src/KeccakContract.sol | 21 | | src/MisusedBoolean.sol | 67 | +| src/MsgValueInLoop.sol | 46 | | src/MultipleConstructorSchemes.sol | 10 | | src/OnceModifierExample.sol | 8 | | src/PreDeclaredVarUsage.sol | 9 | @@ -155,14 +157,14 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/reused_contract_name/ContractB.sol | 7 | | src/uniswap/UniswapV2Swapper.sol | 50 | | src/uniswap/UniswapV3Swapper.sol | 150 | -| **Total** | **1996** | +| **Total** | **2042** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 32 | +| High | 33 | | Low | 25 | @@ -1776,6 +1778,41 @@ A deletion in a structure containing a mapping will not delete the mapping. The +## H-33: Loop contains `msg.value`. + +Do not use `msg.value` in loops since it may be misleading when calculating the total amount.It's recommended to prepare additional variables outside the loop to do the required calculation. + +
4 Found Instances + + +- Found in src/MsgValueInLoop.sol [Line: 10](../tests/contract-playground/src/MsgValueInLoop.sol#L10) + + ```solidity + for (uint256 i = 0; i < receivers.length; i++) { + ``` + +- Found in src/MsgValueInLoop.sol [Line: 21](../tests/contract-playground/src/MsgValueInLoop.sol#L21) + + ```solidity + for (uint256 i = 0; i < receivers.length; i++) { + ``` + +- Found in src/MsgValueInLoop.sol [Line: 37](../tests/contract-playground/src/MsgValueInLoop.sol#L37) + + ```solidity + while (i < receivers.length) { + ``` + +- Found in src/MsgValueInLoop.sol [Line: 54](../tests/contract-playground/src/MsgValueInLoop.sol#L54) + + ```solidity + do { + ``` + +
+ + + # Low Issues ## L-1: Centralization Risk for trusted owners @@ -2008,7 +2045,7 @@ ERC20 functions may not behave as expected. For example: return values are not a Consider using a specific version of Solidity in your contracts instead of a wide version. For example, instead of `pragma solidity ^0.8.0;`, use `pragma solidity 0.8.0;` -
19 Found Instances +
20 Found Instances - Found in src/CompilerBugStorageSignedIntegerArray.sol [Line: 2](../tests/contract-playground/src/CompilerBugStorageSignedIntegerArray.sol#L2) @@ -2065,6 +2102,12 @@ Consider using a specific version of Solidity in your contracts instead of a wid pragma solidity ^0.8.24; ``` +- Found in src/MsgValueInLoop.sol [Line: 2](../tests/contract-playground/src/MsgValueInLoop.sol#L2) + + ```solidity + pragma solidity ^0.8.0; + ``` + - Found in src/PreDeclaredVarUsage.sol [Line: 2](../tests/contract-playground/src/PreDeclaredVarUsage.sol#L2) ```solidity @@ -2180,7 +2223,7 @@ Check for `address(0)` when assigning values to address state variables. Instead of marking a function as `public`, consider marking it as `external` if it is not used internally. -
23 Found Instances +
27 Found Instances - Found in src/ArbitraryTransferFrom.sol [Line: 28](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L28) @@ -2207,6 +2250,30 @@ Instead of marking a function as `public`, consider marking it as `external` if function setNumber(uint256 newNumber) public { ``` +- Found in src/MsgValueInLoop.sol [Line: 8](../tests/contract-playground/src/MsgValueInLoop.sol#L8) + + ```solidity + function bad(address[] memory receivers) public payable { + ``` + +- Found in src/MsgValueInLoop.sol [Line: 19](../tests/contract-playground/src/MsgValueInLoop.sol#L19) + + ```solidity + function bad(address[] memory receivers) public payable { + ``` + +- Found in src/MsgValueInLoop.sol [Line: 34](../tests/contract-playground/src/MsgValueInLoop.sol#L34) + + ```solidity + function bad(address[] memory receivers) public payable { + ``` + +- Found in src/MsgValueInLoop.sol [Line: 51](../tests/contract-playground/src/MsgValueInLoop.sol#L51) + + ```solidity + function bad(address[] memory receivers) public payable { + ``` + - Found in src/StateVariables.sol [Line: 47](../tests/contract-playground/src/StateVariables.sol#L47) ```solidity @@ -2774,7 +2841,7 @@ Using `ERC721::_mint()` can mint ERC721 tokens to addresses which don't support Solc compiler version 0.8.20 switches the default target EVM version to Shanghai, which means that the generated bytecode will include PUSH0 opcodes. Be sure to select the appropriate EVM version in case you intend to deploy on a chain other than mainnet like L2 chains that may not support PUSH0, otherwise deployment of your contracts will fail. -
28 Found Instances +
29 Found Instances - Found in src/AdminContract.sol [Line: 2](../tests/contract-playground/src/AdminContract.sol#L2) @@ -2837,6 +2904,12 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai pragma solidity 0.8.20; ``` +- Found in src/MsgValueInLoop.sol [Line: 2](../tests/contract-playground/src/MsgValueInLoop.sol#L2) + + ```solidity + pragma solidity ^0.8.0; + ``` + - Found in src/StateVariables.sol [Line: 2](../tests/contract-playground/src/StateVariables.sol#L2) ```solidity @@ -3346,7 +3419,7 @@ Use `e` notation, for example: `1e18`, instead of its full numeric value. Instead of separating the logic into a separate function, consider inlining the logic into the calling function. This can reduce the number of function calls and improve readability. -
12 Found Instances +
15 Found Instances - Found in src/CallGraphTests.sol [Line: 6](../tests/contract-playground/src/CallGraphTests.sol#L6) @@ -3379,6 +3452,24 @@ Instead of separating the logic into a separate function, consider inlining the function internalSet2(uint256 _newValue) internal { ``` +- Found in src/MsgValueInLoop.sol [Line: 26](../tests/contract-playground/src/MsgValueInLoop.sol#L26) + + ```solidity + function addToBal(address[] memory receivers, uint256 index) internal { + ``` + +- Found in src/MsgValueInLoop.sol [Line: 43](../tests/contract-playground/src/MsgValueInLoop.sol#L43) + + ```solidity + function addToBal(address[] memory receivers, uint256 index) internal { + ``` + +- Found in src/MsgValueInLoop.sol [Line: 60](../tests/contract-playground/src/MsgValueInLoop.sol#L60) + + ```solidity + function addToBal(address[] memory receivers, uint256 index) internal { + ``` + - Found in src/SendEtherNoChecks.sol [Line: 9](../tests/contract-playground/src/SendEtherNoChecks.sol#L9) ```solidity diff --git a/reports/report.sarif b/reports/report.sarif index c60ae1436..da1152d5f 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -2591,6 +2591,59 @@ }, "ruleId": "delete-nested-mapping" }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MsgValueInLoop.sol" + }, + "region": { + "byteLength": 107, + "byteOffset": 249 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MsgValueInLoop.sol" + }, + "region": { + "byteLength": 94, + "byteOffset": 530 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MsgValueInLoop.sol" + }, + "region": { + "byteLength": 93, + "byteOffset": 955 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MsgValueInLoop.sol" + }, + "region": { + "byteLength": 97, + "byteOffset": 1382 + } + } + } + ], + "message": { + "text": "Do not use `msg.value` in loops since it may be misleading when calculating the total amount.It's recommended to prepare additional variables outside the loop to do the required calculation." + }, + "ruleId": "msg-value-in-loop" + }, { "level": "note", "locations": [ @@ -3046,6 +3099,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MsgValueInLoop.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3284,6 +3348,50 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MsgValueInLoop.sol" + }, + "region": { + "byteLength": 3, + "byteOffset": 168 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MsgValueInLoop.sol" + }, + "region": { + "byteLength": 3, + "byteOffset": 449 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MsgValueInLoop.sol" + }, + "region": { + "byteLength": 3, + "byteOffset": 849 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MsgValueInLoop.sol" + }, + "region": { + "byteLength": 3, + "byteOffset": 1273 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -4394,6 +4502,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MsgValueInLoop.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -5343,6 +5462,39 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MsgValueInLoop.sol" + }, + "region": { + "byteLength": 8, + "byteOffset": 645 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MsgValueInLoop.sol" + }, + "region": { + "byteLength": 8, + "byteOffset": 1069 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MsgValueInLoop.sol" + }, + "region": { + "byteLength": 8, + "byteOffset": 1500 + } + } + }, { "physicalLocation": { "artifactLocation": {