From ccb0e71f8250a8664ff953aeb3706e6c3c1f96dc Mon Sep 17 00:00:00 2001 From: TilakMaddy Date: Fri, 2 Aug 2024 19:37:52 +0530 Subject: [PATCH] cli/reportgen --- .../adhoc-sol-files-highs-only-report.json | 5 +- reports/report.json | 214 ++++++------ reports/report.md | 284 +++++++--------- reports/report.sarif | 320 +++++++++--------- reports/templegold-report.md | 56 +-- 5 files changed, 422 insertions(+), 457 deletions(-) diff --git a/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index 01414ca1f..ad6e9c156 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -182,6 +182,7 @@ "yul-return", "state-variable-shadowing", "unchecked-send", + "misused-boolean", "send-ether-no-checks", "delegate-call-unchecked-address", "tautological-compare", @@ -194,12 +195,8 @@ "weak-randomness", "pre-declared-local-variable-usage", "delete-nested-mapping", -<<<<<<< HEAD - "boolean-equality", -======= "tx-origin-used-for-auth", "msg-value-in-loop", ->>>>>>> dev "contract-locks-ether" ] } \ No newline at end of file diff --git a/reports/report.json b/reports/report.json index e54b0fbcb..b9f9d63f8 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,12 +1,7 @@ { "files_summary": { -<<<<<<< HEAD - "total_source_units": 76, - "total_sloc": 2155 -======= - "total_source_units": 77, - "total_sloc": 2225 ->>>>>>> dev + "total_source_units": 78, + "total_sloc": 2252 }, "files_details": { "files_details": [ @@ -325,13 +320,8 @@ ] }, "issue_count": { -<<<<<<< HEAD - "high": 34, - "low": 26 -======= "high": 36, - "low": 25 ->>>>>>> dev + "low": 26 }, "high_issues": { "issues": [ @@ -1465,6 +1455,73 @@ } ] }, + { + "title": "Misused boolean with logical operators", + "description": "The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively.", + "detector_name": "misused-boolean", + "instances": [ + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 12, + "src": "257:19", + "src_char": "257:19" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 19, + "src": "419:20", + "src_char": "419:20" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 26, + "src": "582:20", + "src_char": "582:20" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 33, + "src": "745:19", + "src_char": "745:19" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 40, + "src": "908:51", + "src_char": "908:51" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 47, + "src": "1060:52", + "src_char": "1060:52" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 54, + "src": "1213:53", + "src_char": "1213:53" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 61, + "src": "1366:21", + "src_char": "1366:21" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 68, + "src": "1530:17", + "src_char": "1530:17" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 75, + "src": "1691:18", + "src_char": "1691:18" + } + ] + }, { "title": "Sending native Eth is not protected from these functions.", "description": "Introduce checks for `msg.sender` in the function", @@ -1802,35 +1859,6 @@ ] }, { -<<<<<<< HEAD - "title": "Boolean equality is not required.", - "description": "If `x` is a boolean, there is no need to do `if(x == true)` or `if(x == false)`. Just use `if(x)` and `if(!x)` respectively.", - "detector_name": "boolean-equality", - "instances": [ - { - "contract_path": "src/BooleanEquality.sol", - "line_no": 5, - "src": "133:14", - "src_char": "133:14" - }, - { - "contract_path": "src/BooleanEquality.sol", - "line_no": 12, - "src": "292:15", - "src_char": "292:15" - }, - { - "contract_path": "src/BooleanEquality.sol", - "line_no": 19, - "src": "454:15", - "src_char": "454:15" - }, - { - "contract_path": "src/BooleanEquality.sol", - "line_no": 26, - "src": "614:16", - "src_char": "614:16" -======= "title": "Potential use of `tx.origin` for authentication.", "description": "Using `tx.origin` may lead to problems when users are interacting via smart contract with your protocol. It is recommended to use `msg.sender` for authentication.", "detector_name": "tx-origin-used-for-auth", @@ -1883,7 +1911,6 @@ "line_no": 71, "src": "1844:97", "src_char": "1844:97" ->>>>>>> dev } ] }, @@ -3979,73 +4006,6 @@ } ] }, - { - "title": "Misused boolean with logical operators", - "description": "The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively.", - "detector_name": "misused-boolean", - "instances": [ - { - "contract_path": "src/MisusedBoolean.sol", - "line_no": 12, - "src": "257:19", - "src_char": "257:19" - }, - { - "contract_path": "src/MisusedBoolean.sol", - "line_no": 19, - "src": "419:20", - "src_char": "419:20" - }, - { - "contract_path": "src/MisusedBoolean.sol", - "line_no": 26, - "src": "582:20", - "src_char": "582:20" - }, - { - "contract_path": "src/MisusedBoolean.sol", - "line_no": 33, - "src": "745:19", - "src_char": "745:19" - }, - { - "contract_path": "src/MisusedBoolean.sol", - "line_no": 40, - "src": "908:51", - "src_char": "908:51" - }, - { - "contract_path": "src/MisusedBoolean.sol", - "line_no": 47, - "src": "1060:52", - "src_char": "1060:52" - }, - { - "contract_path": "src/MisusedBoolean.sol", - "line_no": 54, - "src": "1213:53", - "src_char": "1213:53" - }, - { - "contract_path": "src/MisusedBoolean.sol", - "line_no": 61, - "src": "1366:21", - "src_char": "1366:21" - }, - { - "contract_path": "src/MisusedBoolean.sol", - "line_no": 68, - "src": "1530:17", - "src_char": "1530:17" - }, - { - "contract_path": "src/MisusedBoolean.sol", - "line_no": 75, - "src": "1691:18", - "src_char": "1691:18" - } - ] - }, { "title": "Redundant statements have no effect.", "description": "Remove the redundant statements because no code will be generated and it just congests the codebase.", @@ -4119,6 +4079,37 @@ "src_char": "1175:14" } ] + }, + { + "title": "Boolean equality is not required.", + "description": "If `x` is a boolean, there is no need to do `if(x == true)` or `if(x == false)`. Just use `if(x)` and `if(!x)` respectively.", + "detector_name": "boolean-equality", + "instances": [ + { + "contract_path": "src/BooleanEquality.sol", + "line_no": 5, + "src": "133:14", + "src_char": "133:14" + }, + { + "contract_path": "src/BooleanEquality.sol", + "line_no": 12, + "src": "292:15", + "src_char": "292:15" + }, + { + "contract_path": "src/BooleanEquality.sol", + "line_no": 19, + "src": "454:15", + "src_char": "454:15" + }, + { + "contract_path": "src/BooleanEquality.sol", + "line_no": 26, + "src": "614:16", + "src_char": "614:16" + } + ] } ] }, @@ -4181,12 +4172,9 @@ "weak-randomness", "pre-declared-local-variable-usage", "delete-nested-mapping", -<<<<<<< HEAD "boolean-equality", -======= "tx-origin-used-for-auth", "msg-value-in-loop", ->>>>>>> dev "contract-locks-ether" ] } \ No newline at end of file diff --git a/reports/report.md b/reports/report.md index baf08ca47..b856fd2a7 100644 --- a/reports/report.md +++ b/reports/report.md @@ -28,22 +28,6 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [H-18: Yul block contains `return` function call.](#h-18-yul-block-contains-return-function-call) - [H-19: Shadowed State Variables in Inheritance Hierarchy](#h-19-shadowed-state-variables-in-inheritance-hierarchy) - [H-20: Unchecked `bool success` value for send call.](#h-20-unchecked-bool-success-value-for-send-call) -<<<<<<< HEAD - - [H-21: Sending native Eth is not protected from these functions.](#h-21-sending-native-eth-is-not-protected-from-these-functions) - - [H-22: Delegatecall made by the function without checks on any adress.](#h-22-delegatecall-made-by-the-function-without-checks-on-any-adress) - - [H-23: Tautological comparison.](#h-23-tautological-comparison) - - [H-24: RTLO character detected in file. \u{202e}](#h-24-rtlo-character-detected-in-file-u202e) - - [H-25: Return value of the function call is not checked.](#h-25-return-value-of-the-function-call-is-not-checked) - - [H-26: Dangerous unary operator found in assignment.](#h-26-dangerous-unary-operator-found-in-assignment) - - [H-27: Tautology or Contradiction in comparison.](#h-27-tautology-or-contradiction-in-comparison) - - [H-28: Dangerous strict equality checks on contract balances.](#h-28-dangerous-strict-equality-checks-on-contract-balances) - - [H-29: Compiler Bug: Signed array in storage detected for compiler version `<0.5.10`](#h-29-compiler-bug-signed-array-in-storage-detected-for-compiler-version-0510) - - [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: Boolean equality is not required.](#h-33-boolean-equality-is-not-required) - - [H-34: Contract locks Ether without a withdraw function.](#h-34-contract-locks-ether-without-a-withdraw-function) -======= - [H-21: Misused boolean with logical operators](#h-21-misused-boolean-with-logical-operators) - [H-22: Sending native Eth is not protected from these functions.](#h-22-sending-native-eth-is-not-protected-from-these-functions) - [H-23: Delegatecall made by the function without checks on any adress.](#h-23-delegatecall-made-by-the-function-without-checks-on-any-adress) @@ -60,7 +44,6 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [H-34: Potential use of `tx.origin` for authentication.](#h-34-potential-use-of-txorigin-for-authentication) - [H-35: Loop contains `msg.value`.](#h-35-loop-contains-msgvalue) - [H-36: Contract locks Ether without a withdraw function.](#h-36-contract-locks-ether-without-a-withdraw-function) ->>>>>>> dev - [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) @@ -85,9 +68,9 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [L-21: Unused Custom Error](#l-21-unused-custom-error) - [L-22: Loop contains `require`/`revert` statements](#l-22-loop-contains-requirerevert-statements) - [L-23: Incorrect Order of Division and Multiplication](#l-23-incorrect-order-of-division-and-multiplication) - - [L-24: Misused boolean with logical operators](#l-24-misused-boolean-with-logical-operators) - - [L-25: Redundant statements have no effect.](#l-25-redundant-statements-have-no-effect) - - [L-26: Public variables of a contract read in an external context (using `this`).](#l-26-public-variables-of-a-contract-read-in-an-external-context-using-this) + - [L-24: Redundant statements have no effect.](#l-24-redundant-statements-have-no-effect) + - [L-25: Public variables of a contract read in an external context (using `this`).](#l-25-public-variables-of-a-contract-read-in-an-external-context-using-this) + - [L-26: Boolean equality is not required.](#l-26-boolean-equality-is-not-required) # Summary @@ -96,13 +79,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -<<<<<<< HEAD -| .sol Files | 76 | -| Total nSLOC | 2155 | -======= -| .sol Files | 77 | -| Total nSLOC | 2225 | ->>>>>>> dev +| .sol Files | 78 | +| Total nSLOC | 2252 | ## Files Details @@ -187,24 +165,15 @@ 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 | -<<<<<<< HEAD -| **Total** | **2155** | -======= -| **Total** | **2225** | ->>>>>>> dev +| **Total** | **2252** | ## Issue Summary | Category | No. of Issues | | --- | --- | -<<<<<<< HEAD -| High | 34 | -| Low | 26 | -======= | High | 36 | -| Low | 25 | ->>>>>>> dev +| Low | 26 | # High Issues @@ -1421,7 +1390,78 @@ The transaction `address(payable?).send(address)` may fail because of reasons li -## H-21: Sending native Eth is not protected from these functions. +## H-21: Misused boolean with logical operators + +The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively. + +
10 Found Instances + + +- Found in src/MisusedBoolean.sol [Line: 12](../tests/contract-playground/src/MisusedBoolean.sol#L12) + + ```solidity + if (isEven(num) || true) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 19](../tests/contract-playground/src/MisusedBoolean.sol#L19) + + ```solidity + if (isEven(num) && false) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 26](../tests/contract-playground/src/MisusedBoolean.sol#L26) + + ```solidity + if (false && isEven(num)) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 33](../tests/contract-playground/src/MisusedBoolean.sol#L33) + + ```solidity + if (true || isEven(num)) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 40](../tests/contract-playground/src/MisusedBoolean.sol#L40) + + ```solidity + if (true) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 47](../tests/contract-playground/src/MisusedBoolean.sol#L47) + + ```solidity + if (false) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 54](../tests/contract-playground/src/MisusedBoolean.sol#L54) + + ```solidity + if (!false) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 61](../tests/contract-playground/src/MisusedBoolean.sol#L61) + + ```solidity + if (isEven(num) && !false) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 68](../tests/contract-playground/src/MisusedBoolean.sol#L68) + + ```solidity + if (isEven(num) && NO) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 75](../tests/contract-playground/src/MisusedBoolean.sol#L75) + + ```solidity + if (isEven(num) && !NO) { + ``` + +
+ + + +## H-22: Sending native Eth is not protected from these functions. Introduce checks for `msg.sender` in the function @@ -1510,7 +1550,7 @@ Introduce checks for `msg.sender` in the function -## H-22: Delegatecall made by the function without checks on any adress. +## H-23: Delegatecall made by the function without checks on any adress. Introduce checks on the address @@ -1539,7 +1579,7 @@ Introduce checks on the address -## H-23: Tautological comparison. +## H-24: Tautological comparison. The left hand side and the right hand side of the binary operation has the same value. This makes the condition always true or always false. @@ -1574,7 +1614,7 @@ The left hand side and the right hand side of the binary operation has the same -## H-24: RTLO character detected in file. \u{202e} +## H-25: RTLO character detected in file. \u{202e} Right to left override character may be misledaing and cause potential attacks by visually misordering method arguments! @@ -1591,7 +1631,7 @@ Right to left override character may be misledaing and cause potential attacks b -## H-25: Return value of the function call is not checked. +## H-26: Return value of the function call is not checked. Function returns a value but it is ignored. @@ -1614,7 +1654,7 @@ Function returns a value but it is ignored. -## H-26: Dangerous unary operator found in assignment. +## H-27: Dangerous unary operator found in assignment. Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in between. @@ -1637,7 +1677,7 @@ Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in -## H-27: Tautology or Contradiction in comparison. +## H-28: Tautology or Contradiction in comparison. The condition has been determined to be either always true or always false due to the integer range in which we're operating. @@ -1660,7 +1700,7 @@ The condition has been determined to be either always true or always false due t -## H-28: Dangerous strict equality checks on contract balances. +## H-29: Dangerous strict equality checks on contract balances. A contract's balance can be forcibly manipulated by another selfdestructing contract. Therefore, it's recommended to use >, <, >= or <= instead of strict equality. @@ -1689,7 +1729,7 @@ A contract's balance can be forcibly manipulated by another selfdestructing cont -## H-29: Compiler Bug: Signed array in storage detected for compiler version `<0.5.10` +## H-30: Compiler Bug: Signed array in storage detected for compiler version `<0.5.10` If you want to leverage signed arrays in storage by assigning a literal array with at least one negative number, then you mus use solidity version 0.5.10 or above. This is because of a bug in older compilers. @@ -1706,7 +1746,7 @@ If you want to leverage signed arrays in storage by assigning a literal array wi -## H-30: Weak Randomness +## H-31: Weak Randomness The use of keccak256 hash functions on predictable values like block.timestamp, block.number, or similar data, including modulo operations on these values, should be avoided for generating randomness, as they are easily predictable and manipulable. The `PREVRANDAO` opcode also should not be used as a source of randomness. Instead, utilize Chainlink VRF for cryptographically secure and provably random values to ensure protocol integrity. @@ -1771,7 +1811,7 @@ The use of keccak256 hash functions on predictable values like block.timestamp, -## H-31: Usage of variable before declaration. +## H-32: Usage of variable before declaration. This is a bad practice that may lead to unintended consequences. Please declare the variable before using it. @@ -1788,7 +1828,7 @@ This is a bad practice that may lead to unintended consequences. Please declare -## H-32: Deletion from a nested mappping. +## H-33: Deletion from a nested mappping. A deletion in a structure containing a mapping will not delete the mapping. The remaining data may be used to compromise the contract. @@ -1805,37 +1845,6 @@ A deletion in a structure containing a mapping will not delete the mapping. The -<<<<<<< HEAD -## H-33: Boolean equality is not required. - -If `x` is a boolean, there is no need to do `if(x == true)` or `if(x == false)`. Just use `if(x)` and `if(!x)` respectively. - -
4 Found Instances - - -- Found in src/BooleanEquality.sol [Line: 5](../tests/contract-playground/src/BooleanEquality.sol#L5) - - ```solidity - if (isEven == true) { - ``` - -- Found in src/BooleanEquality.sol [Line: 12](../tests/contract-playground/src/BooleanEquality.sol#L12) - - ```solidity - if (isEven == !true) { - ``` - -- Found in src/BooleanEquality.sol [Line: 19](../tests/contract-playground/src/BooleanEquality.sol#L19) - - ```solidity - if (isEven == false) { - ``` - -- Found in src/BooleanEquality.sol [Line: 26](../tests/contract-playground/src/BooleanEquality.sol#L26) - - ```solidity - if (isEven == !false) { -======= ## H-34: Potential use of `tx.origin` for authentication. Using `tx.origin` may lead to problems when users are interacting via smart contract with your protocol. It is recommended to use `msg.sender` for authentication. @@ -1859,16 +1868,12 @@ Using `tx.origin` may lead to problems when users are interacting via smart cont ```solidity require(tx.origin == owner, "Not authorized to perform this action"); ->>>>>>> dev ```
-<<<<<<< HEAD -## H-34: Contract locks Ether without a withdraw function. -======= ## H-35: Loop contains `msg.value`. Provide an explicit array of amounts alongside the receivers array, and check that the sum of all amounts matches `msg.value`. @@ -1905,7 +1910,6 @@ Provide an explicit array of amounts alongside the receivers array, and check th ## H-36: Contract locks Ether without a withdraw function. ->>>>>>> dev It appears that the contract includes a payable function to accept Ether but lacks a corresponding function to withdraw it, which leads to the Ether being locked in the contract. To resolve this issue, please implement a public or external function that allows for the withdrawal of Ether from the contract. @@ -4082,153 +4086,117 @@ Division operations followed directly by multiplication operations can lead to p -## L-24: Misused boolean with logical operators - -The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively. - -
10 Found Instances - - -- Found in src/MisusedBoolean.sol [Line: 12](../tests/contract-playground/src/MisusedBoolean.sol#L12) - - ```solidity - if (isEven(num) || true) { - ``` - -- Found in src/MisusedBoolean.sol [Line: 19](../tests/contract-playground/src/MisusedBoolean.sol#L19) - - ```solidity - if (isEven(num) && false) { - ``` +## L-24: Redundant statements have no effect. -- Found in src/MisusedBoolean.sol [Line: 26](../tests/contract-playground/src/MisusedBoolean.sol#L26) +Remove the redundant statements because no code will be generated and it just congests the codebase. - ```solidity - if (false && isEven(num)) { - ``` +
6 Found Instances -- Found in src/MisusedBoolean.sol [Line: 33](../tests/contract-playground/src/MisusedBoolean.sol#L33) - ```solidity - if (true || isEven(num)) { - ``` - -- Found in src/MisusedBoolean.sol [Line: 40](../tests/contract-playground/src/MisusedBoolean.sol#L40) +- Found in src/RedundantStatements.sol [Line: 6](../tests/contract-playground/src/RedundantStatements.sol#L6) ```solidity - if (true) { + uint; // Elementary Type Name ``` -- Found in src/MisusedBoolean.sol [Line: 47](../tests/contract-playground/src/MisusedBoolean.sol#L47) +- Found in src/RedundantStatements.sol [Line: 7](../tests/contract-playground/src/RedundantStatements.sol#L7) ```solidity - if (false) { + bool; // Elementary Type Name ``` -- Found in src/MisusedBoolean.sol [Line: 54](../tests/contract-playground/src/MisusedBoolean.sol#L54) +- Found in src/RedundantStatements.sol [Line: 8](../tests/contract-playground/src/RedundantStatements.sol#L8) ```solidity - if (!false) { + RedundantStatementsContract; // Identifier ``` -- Found in src/MisusedBoolean.sol [Line: 61](../tests/contract-playground/src/MisusedBoolean.sol#L61) +- Found in src/RedundantStatements.sol [Line: 12](../tests/contract-playground/src/RedundantStatements.sol#L12) ```solidity - if (isEven(num) && !false) { + uint; // Elementary Type Name ``` -- Found in src/MisusedBoolean.sol [Line: 68](../tests/contract-playground/src/MisusedBoolean.sol#L68) +- Found in src/RedundantStatements.sol [Line: 13](../tests/contract-playground/src/RedundantStatements.sol#L13) ```solidity - if (isEven(num) && NO) { + assert; // Identifier ``` -- Found in src/MisusedBoolean.sol [Line: 75](../tests/contract-playground/src/MisusedBoolean.sol#L75) +- Found in src/RedundantStatements.sol [Line: 14](../tests/contract-playground/src/RedundantStatements.sol#L14) ```solidity - if (isEven(num) && !NO) { + test; // Identifier ```
-## L-25: Redundant statements have no effect. +## L-25: Public variables of a contract read in an external context (using `this`). -Remove the redundant statements because no code will be generated and it just congests the codebase. - -
6 Found Instances - - -- Found in src/RedundantStatements.sol [Line: 6](../tests/contract-playground/src/RedundantStatements.sol#L6) +The contract reads it's own variable using `this` which adds an unnecessary STATICCALL. Remove `this` and access the variable like storage. - ```solidity - uint; // Elementary Type Name - ``` +
4 Found Instances -- Found in src/RedundantStatements.sol [Line: 7](../tests/contract-playground/src/RedundantStatements.sol#L7) - ```solidity - bool; // Elementary Type Name - ``` - -- Found in src/RedundantStatements.sol [Line: 8](../tests/contract-playground/src/RedundantStatements.sol#L8) +- Found in src/PublicVariableReadInExternalContext.sol [Line: 12](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L12) ```solidity - RedundantStatementsContract; // Identifier + return this.testArray(0); ``` -- Found in src/RedundantStatements.sol [Line: 12](../tests/contract-playground/src/RedundantStatements.sol#L12) +- Found in src/PublicVariableReadInExternalContext.sol [Line: 16](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L16) ```solidity - uint; // Elementary Type Name + return this.testUint256(); ``` -- Found in src/RedundantStatements.sol [Line: 13](../tests/contract-playground/src/RedundantStatements.sol#L13) +- Found in src/PublicVariableReadInExternalContext.sol [Line: 20](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L20) ```solidity - assert; // Identifier + return this.testMap(0); ``` -- Found in src/RedundantStatements.sol [Line: 14](../tests/contract-playground/src/RedundantStatements.sol#L14) +- Found in src/PublicVariableReadInExternalContext.sol [Line: 42](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L42) ```solidity - test; // Identifier + return this.testArray(0); ```
-## L-26: Public variables of a contract read in an external context (using `this`). +## L-26: Boolean equality is not required. -The contract reads it's own variable using `this` which adds an unnecessary STATICCALL. Remove `this` and access the variable like storage. +If `x` is a boolean, there is no need to do `if(x == true)` or `if(x == false)`. Just use `if(x)` and `if(!x)` respectively.
4 Found Instances -- Found in src/PublicVariableReadInExternalContext.sol [Line: 12](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L12) +- Found in src/BooleanEquality.sol [Line: 5](../tests/contract-playground/src/BooleanEquality.sol#L5) ```solidity - return this.testArray(0); + if (isEven == true) { ``` -- Found in src/PublicVariableReadInExternalContext.sol [Line: 16](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L16) +- Found in src/BooleanEquality.sol [Line: 12](../tests/contract-playground/src/BooleanEquality.sol#L12) ```solidity - return this.testUint256(); + if (isEven == !true) { ``` -- Found in src/PublicVariableReadInExternalContext.sol [Line: 20](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L20) +- Found in src/BooleanEquality.sol [Line: 19](../tests/contract-playground/src/BooleanEquality.sol#L19) ```solidity - return this.testMap(0); + if (isEven == false) { ``` -- Found in src/PublicVariableReadInExternalContext.sol [Line: 42](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L42) +- Found in src/BooleanEquality.sol [Line: 26](../tests/contract-playground/src/BooleanEquality.sol#L26) ```solidity - return this.testArray(0); + if (isEven == !false) { ```
diff --git a/reports/report.sarif b/reports/report.sarif index 81d1772e5..a9df86e6b 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -1999,6 +1999,125 @@ }, "ruleId": "unchecked-send" }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 19, + "byteOffset": 257 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 419 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 582 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 19, + "byteOffset": 745 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 51, + "byteOffset": 908 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 52, + "byteOffset": 1060 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 53, + "byteOffset": 1213 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 21, + "byteOffset": 1366 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 17, + "byteOffset": 1530 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 18, + "byteOffset": 1691 + } + } + } + ], + "message": { + "text": "The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively." + }, + "ruleId": "misused-boolean" + }, { "level": "warning", "locations": [ @@ -2575,78 +2694,38 @@ { "physicalLocation": { "artifactLocation": { -<<<<<<< HEAD - "uri": "src/BooleanEquality.sol" - }, - "region": { - "byteLength": 14, - "byteOffset": 133 -======= "uri": "src/TxOriginUsedForAuth.sol" }, "region": { "byteLength": 183, "byteOffset": 1117 ->>>>>>> dev } } }, { "physicalLocation": { "artifactLocation": { -<<<<<<< HEAD - "uri": "src/BooleanEquality.sol" - }, - "region": { - "byteLength": 15, - "byteOffset": 292 -======= "uri": "src/TxOriginUsedForAuth.sol" }, "region": { "byteLength": 90, "byteOffset": 1431 ->>>>>>> dev - } - } - }, - { - "physicalLocation": { - "artifactLocation": { -<<<<<<< HEAD - "uri": "src/BooleanEquality.sol" - }, - "region": { - "byteLength": 15, - "byteOffset": 454 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/BooleanEquality.sol" - }, - "region": { - "byteLength": 16, - "byteOffset": 614 -======= "uri": "src/TxOriginUsedForAuth.sol" }, "region": { "byteLength": 68, "byteOffset": 1610 ->>>>>>> dev } } } ], "message": { -<<<<<<< HEAD - "text": "If `x` is a boolean, there is no need to do `if(x == true)` or `if(x == false)`. Just use `if(x)` and `if(!x)` respectively." - }, - "ruleId": "boolean-equality" -======= "text": "Using `tx.origin` may lead to problems when users are interacting via smart contract with your protocol. It is recommended to use `msg.sender` for authentication." }, "ruleId": "tx-origin-used-for-auth" @@ -2703,7 +2782,6 @@ "text": "Provide an explicit array of amounts alongside the receivers array, and check that the sum of all amounts matches `msg.value`." }, "ruleId": "msg-value-in-loop" ->>>>>>> dev }, { "level": "warning", @@ -6447,118 +6525,74 @@ { "physicalLocation": { "artifactLocation": { - "uri": "src/MisusedBoolean.sol" - }, - "region": { - "byteLength": 19, - "byteOffset": 257 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/MisusedBoolean.sol" - }, - "region": { - "byteLength": 20, - "byteOffset": 419 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/MisusedBoolean.sol" - }, - "region": { - "byteLength": 20, - "byteOffset": 582 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/MisusedBoolean.sol" - }, - "region": { - "byteLength": 19, - "byteOffset": 745 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/MisusedBoolean.sol" + "uri": "src/RedundantStatements.sol" }, "region": { - "byteLength": 51, - "byteOffset": 908 + "byteLength": 4, + "byteOffset": 131 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/MisusedBoolean.sol" + "uri": "src/RedundantStatements.sol" }, "region": { - "byteLength": 52, - "byteOffset": 1060 + "byteLength": 4, + "byteOffset": 169 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/MisusedBoolean.sol" + "uri": "src/RedundantStatements.sol" }, "region": { - "byteLength": 53, - "byteOffset": 1213 + "byteLength": 27, + "byteOffset": 207 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/MisusedBoolean.sol" + "uri": "src/RedundantStatements.sol" }, "region": { - "byteLength": 21, - "byteOffset": 1366 + "byteLength": 4, + "byteOffset": 309 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/MisusedBoolean.sol" + "uri": "src/RedundantStatements.sol" }, "region": { - "byteLength": 17, - "byteOffset": 1530 + "byteLength": 6, + "byteOffset": 347 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/MisusedBoolean.sol" + "uri": "src/RedundantStatements.sol" }, "region": { - "byteLength": 18, - "byteOffset": 1691 + "byteLength": 4, + "byteOffset": 377 } } } ], "message": { - "text": "The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively." + "text": "Remove the redundant statements because no code will be generated and it just congests the codebase." }, - "ruleId": "misused-boolean" + "ruleId": "redundant-statements" }, { "level": "note", @@ -6566,74 +6600,52 @@ { "physicalLocation": { "artifactLocation": { - "uri": "src/RedundantStatements.sol" - }, - "region": { - "byteLength": 4, - "byteOffset": 131 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/RedundantStatements.sol" - }, - "region": { - "byteLength": 4, - "byteOffset": 169 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/RedundantStatements.sol" + "uri": "src/PublicVariableReadInExternalContext.sol" }, "region": { - "byteLength": 27, - "byteOffset": 207 + "byteLength": 14, + "byteOffset": 355 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/RedundantStatements.sol" + "uri": "src/PublicVariableReadInExternalContext.sol" }, "region": { - "byteLength": 4, - "byteOffset": 309 + "byteLength": 16, + "byteOffset": 457 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/RedundantStatements.sol" + "uri": "src/PublicVariableReadInExternalContext.sol" }, "region": { - "byteLength": 6, - "byteOffset": 347 + "byteLength": 12, + "byteOffset": 553 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/RedundantStatements.sol" + "uri": "src/PublicVariableReadInExternalContext.sol" }, "region": { - "byteLength": 4, - "byteOffset": 377 + "byteLength": 14, + "byteOffset": 1175 } } } ], "message": { - "text": "Remove the redundant statements because no code will be generated and it just congests the codebase." + "text": "The contract reads it's own variable using `this` which adds an unnecessary STATICCALL. Remove `this` and access the variable like storage." }, - "ruleId": "redundant-statements" + "ruleId": "public-variable-read-in-external-context" }, { "level": "note", @@ -6641,52 +6653,52 @@ { "physicalLocation": { "artifactLocation": { - "uri": "src/PublicVariableReadInExternalContext.sol" + "uri": "src/BooleanEquality.sol" }, "region": { "byteLength": 14, - "byteOffset": 355 + "byteOffset": 133 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/PublicVariableReadInExternalContext.sol" + "uri": "src/BooleanEquality.sol" }, "region": { - "byteLength": 16, - "byteOffset": 457 + "byteLength": 15, + "byteOffset": 292 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/PublicVariableReadInExternalContext.sol" + "uri": "src/BooleanEquality.sol" }, "region": { - "byteLength": 12, - "byteOffset": 553 + "byteLength": 15, + "byteOffset": 454 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/PublicVariableReadInExternalContext.sol" + "uri": "src/BooleanEquality.sol" }, "region": { - "byteLength": 14, - "byteOffset": 1175 + "byteLength": 16, + "byteOffset": 614 } } } ], "message": { - "text": "The contract reads it's own variable using `this` which adds an unnecessary STATICCALL. Remove `this` and access the variable like storage." + "text": "If `x` is a boolean, there is no need to do `if(x == true)` or `if(x == false)`. Just use `if(x)` and `if(!x)` respectively." }, - "ruleId": "public-variable-read-in-external-context" + "ruleId": "boolean-equality" } ], "tool": { diff --git a/reports/templegold-report.md b/reports/templegold-report.md index 9b24fa7f6..bb3f1c2ac 100644 --- a/reports/templegold-report.md +++ b/reports/templegold-report.md @@ -17,8 +17,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [H-7: Return value of the function call is not checked.](#h-7-return-value-of-the-function-call-is-not-checked) - [H-8: Weak Randomness](#h-8-weak-randomness) - [H-9: Deletion from a nested mappping.](#h-9-deletion-from-a-nested-mappping) - - [H-10: Boolean equality is not required.](#h-10-boolean-equality-is-not-required) - - [H-11: Contract locks Ether without a withdraw function.](#h-11-contract-locks-ether-without-a-withdraw-function) + - [H-10: Contract locks Ether without a withdraw function.](#h-10-contract-locks-ether-without-a-withdraw-function) - [Low Issues](#low-issues) - [L-1: Centralization Risk for trusted owners](#l-1-centralization-risk-for-trusted-owners) - [L-2: `ecrecover` is susceptible to signature malleability](#l-2-ecrecover-is-susceptible-to-signature-malleability) @@ -39,6 +38,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [L-17: Loop contains `require`/`revert` statements](#l-17-loop-contains-requirerevert-statements) - [L-18: Incorrect Order of Division and Multiplication](#l-18-incorrect-order-of-division-and-multiplication) - [L-19: Redundant statements have no effect.](#l-19-redundant-statements-have-no-effect) + - [L-20: Boolean equality is not required.](#l-20-boolean-equality-is-not-required) # Summary @@ -191,8 +191,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 11 | -| Low | 19 | +| High | 10 | +| Low | 20 | # High Issues @@ -554,30 +554,7 @@ A deletion in a structure containing a mapping will not delete the mapping. The -## H-10: Boolean equality is not required. - -If `x` is a boolean, there is no need to do `if(x == true)` or `if(x == false)`. Just use `if(x)` and `if(!x)` respectively. - -
2 Found Instances - - -- Found in contracts/deprecated/Faith.sol [Line: 32](../tests/2024-07-templegold/protocol/contracts/deprecated/Faith.sol#L32) - - ```solidity - require(canManageFaith[msg.sender] == true, "Faith: caller cannot manage faith"); - ``` - -- Found in contracts/deprecated/Faith.sol [Line: 40](../tests/2024-07-templegold/protocol/contracts/deprecated/Faith.sol#L40) - - ```solidity - require(canManageFaith[msg.sender] == true, "Faith: caller cannot manage faith"); - ``` - -
- - - -## H-11: Contract locks Ether without a withdraw function. +## H-10: Contract locks Ether without a withdraw function. It appears that the contract includes a payable function to accept Ether but lacks a corresponding function to withdraw it, which leads to the Ether being locked in the contract. To resolve this issue, please implement a public or external function that allows for the withdrawal of Ether from the contract. @@ -8677,3 +8654,26 @@ Remove the redundant statements because no code will be generated and it just co +## L-20: Boolean equality is not required. + +If `x` is a boolean, there is no need to do `if(x == true)` or `if(x == false)`. Just use `if(x)` and `if(!x)` respectively. + +
2 Found Instances + + +- Found in contracts/deprecated/Faith.sol [Line: 32](../tests/2024-07-templegold/protocol/contracts/deprecated/Faith.sol#L32) + + ```solidity + require(canManageFaith[msg.sender] == true, "Faith: caller cannot manage faith"); + ``` + +- Found in contracts/deprecated/Faith.sol [Line: 40](../tests/2024-07-templegold/protocol/contracts/deprecated/Faith.sol#L40) + + ```solidity + require(canManageFaith[msg.sender] == true, "Faith: caller cannot manage faith"); + ``` + +
+ + +