From ee2f4640be987b34baf1de14311121bf4cd625bd Mon Sep 17 00:00:00 2001 From: Tilak Madichetti Date: Tue, 30 Jul 2024 18:36:42 +0530 Subject: [PATCH] Detector: Strict Equality Check on Contracts' balances (#625) Co-authored-by: Alex Roan --- aderyn_core/src/detect/detector.rs | 5 + .../high/dangerous_strict_equality_balance.rs | 127 ++++++++++++++++++ aderyn_core/src/detect/high/mod.rs | 2 + .../adhoc-sol-files-highs-only-report.json | 1 + reports/report.json | 64 ++++++++- reports/report.md | 86 +++++++++--- reports/report.sarif | 86 ++++++++++++ .../src/DangerousStrictEquality1.sol | 8 ++ .../src/DangerousStrictEquality2.sol | 12 ++ 9 files changed, 373 insertions(+), 18 deletions(-) create mode 100644 aderyn_core/src/detect/high/dangerous_strict_equality_balance.rs create mode 100644 tests/contract-playground/src/DangerousStrictEquality1.sol create mode 100644 tests/contract-playground/src/DangerousStrictEquality2.sol diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index 048fdef7e..a8c366c9d 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -66,6 +66,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), Box::::default(), Box::::default(), Box::::default(), @@ -134,6 +135,7 @@ pub(crate) enum IssueDetectorNamePool { RTLO, UncheckedReturn, DangerousUnaryOperator, + DangerousStrictEquailtyOnContractBalance, SignedStorageArray, RedundantStatements, PublicVariableReadInExternalContext, @@ -276,6 +278,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { Some(Box::::default()) } + IssueDetectorNamePool::DangerousStrictEquailtyOnContractBalance => { + Some(Box::::default()) + } IssueDetectorNamePool::SignedStorageArray => { Some(Box::::default()) } diff --git a/aderyn_core/src/detect/high/dangerous_strict_equality_balance.rs b/aderyn_core/src/detect/high/dangerous_strict_equality_balance.rs new file mode 100644 index 000000000..8e5098bf6 --- /dev/null +++ b/aderyn_core/src/detect/high/dangerous_strict_equality_balance.rs @@ -0,0 +1,127 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::{Expression, NodeID}; + +use crate::capture; +use crate::detect::detector::IssueDetectorNamePool; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct DangerousStrictEqualityOnBalanceDetector { + // Keys are: [0] source file name, [1] line number, [2] character location of node. + // Do not add items manually, use `capture!` to add nodes to this BTreeMap. + found_instances: BTreeMap<(String, usize, String), NodeID>, +} + +impl IssueDetector for DangerousStrictEqualityOnBalanceDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + // When you have found an instance of the issue, + // use the following macro to add it to `found_instances`: + // + // capture!(self, context, item); + + for binary_operation in context + .binary_operations() + .into_iter() + .filter(|&op| op.operator == "==" || op.operator == "!=") + { + for expr in [ + binary_operation.left_expression.as_ref(), + binary_operation.right_expression.as_ref(), + ] { + if let Expression::MemberAccess(member_access) = expr { + if member_access.member_name == "balance" + && member_access + .expression + .as_ref() + .type_descriptions() + .is_some_and(|type_desc| { + type_desc.type_string.as_ref().is_some_and(|type_string| { + // For older solc versions when you say this.balance, "this" is of type contract XXX + type_string.starts_with("contract ") + // In newers solidity versions, you say adddress(this).balance or payable(address(this)).balance + || type_string == "address" + || type_string == "address payable" + }) + }) + { + capture!(self, context, binary_operation); + } + } + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("Dangerous strict equality checks on contract balances.") + } + + fn description(&self) -> String { + String::from("A contract's balance can be forcibly manipulated by another selfdestructing contract. Therefore, it's recommended to use >, <, >= or <= instead of strict equality.") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + IssueDetectorNamePool::DangerousStrictEquailtyOnContractBalance.to_string() + } +} + +#[cfg(test)] +mod dangerous_strict_equality_balance_tests { + use crate::detect::{ + detector::IssueDetector, + high::dangerous_strict_equality_balance::DangerousStrictEqualityOnBalanceDetector, + }; + + #[test] + fn test_dangerous_strict_equality_balance1() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/DangerousStrictEquality1.sol", + ); + + let mut detector = DangerousStrictEqualityOnBalanceDetector::default(); + let found = detector.detect(&context).unwrap(); + // assert that the detector found an issue + assert!(found); + // assert that the detector found the correct number of instances + assert_eq!(detector.instances().len(), 1); + // assert the severity is high + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::High + ); + } + + #[test] + fn test_dangerous_strict_equality_balance2() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/DangerousStrictEquality2.sol", + ); + + let mut detector = DangerousStrictEqualityOnBalanceDetector::default(); + let found = detector.detect(&context).unwrap(); + // assert that the detector found an issue + assert!(found); + // assert that the detector found the correct number of instances + assert_eq!(detector.instances().len(), 2); + // assert the severity is high + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::High + ); + } +} diff --git a/aderyn_core/src/detect/high/mod.rs b/aderyn_core/src/detect/high/mod.rs index 2021881f2..abd5620de 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -1,6 +1,7 @@ pub(crate) mod arbitrary_transfer_from; pub(crate) mod avoid_abi_encode_packed; pub(crate) mod block_timestamp_deadline; +pub(crate) mod dangerous_strict_equality_balance; pub(crate) mod dangerous_unary_operator; pub(crate) mod delegate_call_in_loop; pub(crate) mod delegate_call_no_address_check; @@ -33,6 +34,7 @@ pub(crate) mod yul_return; pub use arbitrary_transfer_from::ArbitraryTransferFromDetector; pub use avoid_abi_encode_packed::AvoidAbiEncodePackedDetector; pub use block_timestamp_deadline::BlockTimestampDeadlineDetector; +pub use dangerous_strict_equality_balance::DangerousStrictEqualityOnBalanceDetector; pub use dangerous_unary_operator::DangerousUnaryOperatorDetector; pub use delegate_call_in_loop::DelegateCallInLoopDetector; pub use delegate_call_no_address_check::DelegateCallOnUncheckedAddressDetector; diff --git a/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index 2363e9be8..801747b5a 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -189,6 +189,7 @@ "rtlo", "unchecked-return", "dangerous-unary-operator", + "dangerous-strict-equailty-on-contract-balance", "signed-storage-array", "weak-randomness", "pre-declared-local-variable-usage", diff --git a/reports/report.json b/reports/report.json index 37c0f9e16..3380ece53 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 71, - "total_sloc": 1981 + "total_source_units": 73, + "total_sloc": 1996 }, "files_details": { "files_details": [ @@ -49,6 +49,14 @@ "file_path": "src/CrazyPragma.sol", "n_sloc": 4 }, + { + "file_path": "src/DangerousStrictEquality1.sol", + "n_sloc": 6 + }, + { + "file_path": "src/DangerousStrictEquality2.sol", + "n_sloc": 9 + }, { "file_path": "src/DangerousUnaryOperator.sol", "n_sloc": 13 @@ -292,7 +300,7 @@ ] }, "issue_count": { - "high": 31, + "high": 32, "low": 25 }, "high_issues": { @@ -1650,6 +1658,31 @@ } ] }, + { + "title": "Dangerous strict equality checks on contract balances.", + "description": "A contract's balance can be forcibly manipulated by another selfdestructing contract. Therefore, it's recommended to use >, <, >= or <= instead of strict equality.", + "detector_name": "dangerous-strict-equailty-on-contract-balance", + "instances": [ + { + "contract_path": "src/DangerousStrictEquality1.sol", + "line_no": 6, + "src": "177:25", + "src_char": "177:25" + }, + { + "contract_path": "src/DangerousStrictEquality2.sol", + "line_no": 6, + "src": "177:34", + "src_char": "177:34" + }, + { + "contract_path": "src/DangerousStrictEquality2.sol", + "line_no": 10, + "src": "305:43", + "src_char": "305:43" + } + ] + }, { "title": "Compiler Bug: Signed array in storage detected for compiler version `<0.5.10`", "description": "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.", @@ -1986,6 +2019,12 @@ "src": "32:32", "src_char": "32:32" }, + { + "contract_path": "src/DangerousStrictEquality1.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/DangerousUnaryOperator.sol", "line_no": 2, @@ -2343,6 +2382,18 @@ "src": "1275:66", "src_char": "1275:66" }, + { + "contract_path": "src/DangerousStrictEquality2.sol", + "line_no": 6, + "src": "202:9", + "src_char": "202:9" + }, + { + "contract_path": "src/DangerousStrictEquality2.sol", + "line_no": 10, + "src": "339:9", + "src_char": "339:9" + }, { "contract_path": "src/DelegateCallWithoutAddressCheck.sol", "line_no": 24, @@ -2714,6 +2765,12 @@ "src": "32:32", "src_char": "32:32" }, + { + "contract_path": "src/DangerousStrictEquality2.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/DelegateCallWithoutAddressCheck.sol", "line_no": 2, @@ -3662,6 +3719,7 @@ "rtlo", "unchecked-return", "dangerous-unary-operator", + "dangerous-strict-equailty-on-contract-balance", "signed-storage-array", "redundant-statements", "public-variable-read-in-external-context", diff --git a/reports/report.md b/reports/report.md index 345aa4778..d6af919d3 100644 --- a/reports/report.md +++ b/reports/report.md @@ -35,10 +35,11 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [H-25: RTLO character detected in file. \u{202e}](#h-25-rtlo-character-detected-in-file-u202e) - [H-26: Return value of the function call is not checked.](#h-26-return-value-of-the-function-call-is-not-checked) - [H-27: Dangerous unary operator found in assignment.](#h-27-dangerous-unary-operator-found-in-assignment) - - [H-28: Compiler Bug: Signed array in storage detected for compiler version `<0.5.10`](#h-28-compiler-bug-signed-array-in-storage-detected-for-compiler-version-0510) - - [H-29: Weak Randomness](#h-29-weak-randomness) - - [H-30: Usage of variable before declaration.](#h-30-usage-of-variable-before-declaration) - - [H-31: Deletion from a nested mappping.](#h-31-deletion-from-a-nested-mappping) + - [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) - [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) @@ -73,8 +74,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 71 | -| Total nSLOC | 1981 | +| .sol Files | 73 | +| Total nSLOC | 1996 | ## Files Details @@ -92,6 +93,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/ContractWithTodo.sol | 7 | | src/Counter.sol | 20 | | src/CrazyPragma.sol | 4 | +| src/DangerousStrictEquality1.sol | 6 | +| src/DangerousStrictEquality2.sol | 9 | | src/DangerousUnaryOperator.sol | 13 | | src/DelegateCallWithoutAddressCheck.sol | 31 | | src/DeletionNestedMappingStructureContract.sol | 11 | @@ -152,14 +155,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** | **1981** | +| **Total** | **1996** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 31 | +| High | 32 | | Low | 25 | @@ -1628,7 +1631,36 @@ Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in -## H-28: Compiler Bug: Signed array in storage detected for compiler version `<0.5.10` +## H-28: 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. + +
3 Found Instances + + +- Found in src/DangerousStrictEquality1.sol [Line: 6](../tests/contract-playground/src/DangerousStrictEquality1.sol#L6) + + ```solidity + return this.balance == 100 ether; + ``` + +- Found in src/DangerousStrictEquality2.sol [Line: 6](../tests/contract-playground/src/DangerousStrictEquality2.sol#L6) + + ```solidity + return address(this).balance == 100 ether; + ``` + +- Found in src/DangerousStrictEquality2.sol [Line: 10](../tests/contract-playground/src/DangerousStrictEquality2.sol#L10) + + ```solidity + return payable(address(this)).balance == 100 ether; + ``` + +
+ + + +## H-29: 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. @@ -1645,7 +1677,7 @@ If you want to leverage signed arrays in storage by assigning a literal array wi -## H-29: Weak Randomness +## H-30: 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. @@ -1710,7 +1742,7 @@ The use of keccak256 hash functions on predictable values like block.timestamp, -## H-30: Usage of variable before declaration. +## H-31: Usage of variable before declaration. This is a bad practice that may lead to unintended consequences. Please declare the variable before using it. @@ -1727,7 +1759,7 @@ This is a bad practice that may lead to unintended consequences. Please declare -## H-31: Deletion from a nested mappping. +## H-32: 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. @@ -1976,7 +2008,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;` -
18 Found Instances +
19 Found Instances - Found in src/CompilerBugStorageSignedIntegerArray.sol [Line: 2](../tests/contract-playground/src/CompilerBugStorageSignedIntegerArray.sol#L2) @@ -2003,6 +2035,12 @@ Consider using a specific version of Solidity in your contracts instead of a wid pragma solidity >=0.8.19 <0.9.1; ``` +- Found in src/DangerousStrictEquality1.sol [Line: 2](../tests/contract-playground/src/DangerousStrictEquality1.sol#L2) + + ```solidity + pragma solidity ^0.4.0; + ``` + - Found in src/DangerousUnaryOperator.sol [Line: 2](../tests/contract-playground/src/DangerousUnaryOperator.sol#L2) ```solidity @@ -2291,7 +2329,7 @@ Instead of marking a function as `public`, consider marking it as `external` if If the same constant literal value is used multiple times, create a constant state variable and reference it throughout the contract. -
36 Found Instances +
38 Found Instances - Found in src/Casting.sol [Line: 16](../tests/contract-playground/src/Casting.sol#L16) @@ -2372,6 +2410,18 @@ If the same constant literal value is used multiple times, create a constant sta bytes32 multipleUseOfBytes32 = 0x8a1b3dbe6301650442bfa765d4de23775fc9a4ec4329ebb5995ec7f1e3777dc4; ``` +- Found in src/DangerousStrictEquality2.sol [Line: 6](../tests/contract-playground/src/DangerousStrictEquality2.sol#L6) + + ```solidity + return address(this).balance == 100 ether; + ``` + +- Found in src/DangerousStrictEquality2.sol [Line: 10](../tests/contract-playground/src/DangerousStrictEquality2.sol#L10) + + ```solidity + return payable(address(this)).balance == 100 ether; + ``` + - Found in src/DelegateCallWithoutAddressCheck.sol [Line: 24](../tests/contract-playground/src/DelegateCallWithoutAddressCheck.sol#L24) ```solidity @@ -2724,7 +2774,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. -
27 Found Instances +
28 Found Instances - Found in src/AdminContract.sol [Line: 2](../tests/contract-playground/src/AdminContract.sol#L2) @@ -2751,6 +2801,12 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai pragma solidity >=0.8.19 <0.9.1; ``` +- Found in src/DangerousStrictEquality2.sol [Line: 2](../tests/contract-playground/src/DangerousStrictEquality2.sol#L2) + + ```solidity + pragma solidity 0.8.20; + ``` + - Found in src/DelegateCallWithoutAddressCheck.sol [Line: 2](../tests/contract-playground/src/DelegateCallWithoutAddressCheck.sol#L2) ```solidity diff --git a/reports/report.sarif b/reports/report.sarif index 12a857671..c60ae1436 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -2381,6 +2381,48 @@ }, "ruleId": "dangerous-unary-operator" }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DangerousStrictEquality1.sol" + }, + "region": { + "byteLength": 25, + "byteOffset": 177 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DangerousStrictEquality2.sol" + }, + "region": { + "byteLength": 34, + "byteOffset": 177 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DangerousStrictEquality2.sol" + }, + "region": { + "byteLength": 43, + "byteOffset": 305 + } + } + } + ], + "message": { + "text": "A contract's balance can be forcibly manipulated by another selfdestructing contract. Therefore, it's recommended to use >, <, >= or <= instead of strict equality." + }, + "ruleId": "dangerous-strict-equailty-on-contract-balance" + }, { "level": "warning", "locations": [ @@ -2949,6 +2991,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DangerousStrictEquality1.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3592,6 +3645,28 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DangerousStrictEquality2.sol" + }, + "region": { + "byteLength": 9, + "byteOffset": 202 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DangerousStrictEquality2.sol" + }, + "region": { + "byteLength": 9, + "byteOffset": 339 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -4253,6 +4328,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DangerousStrictEquality2.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { diff --git a/tests/contract-playground/src/DangerousStrictEquality1.sol b/tests/contract-playground/src/DangerousStrictEquality1.sol new file mode 100644 index 000000000..e41b87c3e --- /dev/null +++ b/tests/contract-playground/src/DangerousStrictEquality1.sol @@ -0,0 +1,8 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.4.0; + +contract DangerousStrictEquality1 { + function makeStrictBalanceCheck() external view returns (bool) { + return this.balance == 100 ether; + } +} diff --git a/tests/contract-playground/src/DangerousStrictEquality2.sol b/tests/contract-playground/src/DangerousStrictEquality2.sol new file mode 100644 index 000000000..908e09def --- /dev/null +++ b/tests/contract-playground/src/DangerousStrictEquality2.sol @@ -0,0 +1,12 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.20; + +contract DangerousStrictEquality2 { + function makeStrictBalanceCheck() external view returns (bool) { + return address(this).balance == 100 ether; + } + + function makeStrictBalanceCheck2() external view returns (bool) { + return payable(address(this)).balance == 100 ether; + } +}