From 4454c90ff8041354ed4c259e0b2b7689f5c35678 Mon Sep 17 00:00:00 2001 From: Tilak Madichetti Date: Fri, 2 Aug 2024 18:54:30 +0530 Subject: [PATCH] Detector: Bad use of `tx.origin` (#642) Co-authored-by: Alex Roan --- aderyn_core/src/detect/detector.rs | 5 + aderyn_core/src/detect/high/mod.rs | 2 + .../detect/high/tx_origin_used_for_auth.rs | 171 ++++++++++++++++++ .../adhoc-sol-files-highs-only-report.json | 1 + reports/report.json | 48 ++++- reports/report.md | 63 ++++++- reports/report.sarif | 64 +++++++ .../src/TxOriginUsedForAuth.sol | 62 +++++++ 8 files changed, 403 insertions(+), 13 deletions(-) create mode 100644 aderyn_core/src/detect/high/tx_origin_used_for_auth.rs create mode 100644 tests/contract-playground/src/TxOriginUsedForAuth.sol diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index 8c27cfaca..2f764df71 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -74,6 +74,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), Box::::default(), Box::::default(), ] @@ -146,6 +147,7 @@ pub(crate) enum IssueDetectorNamePool { WeakRandomness, PreDeclaredLocalVariableUsage, DeleteNestedMapping, + TxOriginUsedForAuth, MsgValueInLoop, ContractLocksEther, // NOTE: `Undecided` will be the default name (for new bots). @@ -306,6 +308,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { Some(Box::::default()) } + IssueDetectorNamePool::TxOriginUsedForAuth => { + Some(Box::::default()) + } IssueDetectorNamePool::MsgValueInLoop => Some(Box::::default()), IssueDetectorNamePool::ContractLocksEther => { Some(Box::::default()) diff --git a/aderyn_core/src/detect/high/mod.rs b/aderyn_core/src/detect/high/mod.rs index b33b9f716..782707503 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -26,6 +26,7 @@ pub(crate) mod storage_array_edit_with_memory; pub(crate) mod storage_signed_integer_array; pub(crate) mod tautological_compare; pub(crate) mod tautology_or_contradiction; +pub(crate) mod tx_origin_used_for_auth; pub(crate) mod unchecked_return; pub(crate) mod unchecked_send; pub(crate) mod uninitialized_state_variable; @@ -62,6 +63,7 @@ pub use storage_array_edit_with_memory::StorageArrayEditWithMemoryDetector; pub use storage_signed_integer_array::StorageSignedIntegerArrayDetector; pub use tautological_compare::TautologicalCompareDetector; pub use tautology_or_contradiction::TautologyOrContraditionDetector; +pub use tx_origin_used_for_auth::TxOriginUsedForAuthDetector; pub use unchecked_return::UncheckedReturnDetector; pub use unchecked_send::UncheckedSendDetector; pub use uninitialized_state_variable::UninitializedStateVariableDetector; diff --git a/aderyn_core/src/detect/high/tx_origin_used_for_auth.rs b/aderyn_core/src/detect/high/tx_origin_used_for_auth.rs new file mode 100644 index 000000000..f733de663 --- /dev/null +++ b/aderyn_core/src/detect/high/tx_origin_used_for_auth.rs @@ -0,0 +1,171 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::{ASTNode, Expression, Identifier, NodeID}; + +use crate::capture; +use crate::context::browser::ExtractMemberAccesses; +use crate::context::investigator::{ + StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor, +}; +use crate::detect::detector::IssueDetectorNamePool; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct TxOriginUsedForAuthDetector { + // 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 TxOriginUsedForAuthDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + for if_statement in context.if_statements() { + // Check within the condition block only + let ast_node: ASTNode = if_statement.condition.clone().into(); + self.check_eligibility_and_capture(context, &[&ast_node], &(if_statement.into()))?; + } + + for function_call in context.function_calls() { + if let Expression::Identifier(Identifier { name, .. }) = + function_call.expression.as_ref() + { + if name != "require" { + continue; + } + + // Now, check for arguments of the `require(..., "message")` function call + let arguments = function_call + .arguments + .clone() + .into_iter() + .map(|n| n.into()) + .collect::>(); + + let ast_nodes: &[&ASTNode] = &(arguments.iter().collect::>()); + self.check_eligibility_and_capture(context, ast_nodes, &(function_call.into()))?; + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("Potential use of `tx.origin` for authentication.") + } + + fn description(&self) -> String { + String::from("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.") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + format!("{}", IssueDetectorNamePool::TxOriginUsedForAuth) + } +} + +impl TxOriginUsedForAuthDetector { + fn check_eligibility_and_capture( + &mut self, + context: &WorkspaceContext, + check_nodes: &[&ASTNode], + capture_node: &ASTNode, + ) -> Result<(), Box> { + // Boilerplate + let mut tracker = MsgSenderAndTxOriginTracker::default(); + let investigator = StandardInvestigator::new( + context, + check_nodes, + StandardInvestigationStyle::Downstream, + )?; + investigator.investigate(context, &mut tracker)?; + + if tracker.satisifed() { + capture!(self, context, capture_node); + } + Ok(()) + } +} + +#[derive(Default)] +struct MsgSenderAndTxOriginTracker { + reads_msg_sender: bool, + reads_tx_origin: bool, +} + +impl MsgSenderAndTxOriginTracker { + /// To avoid FP (msg.sender == tx.origin) we require that tx.origin is present and msg.sender is absent + /// for it to be considered satisfied + fn satisifed(&self) -> bool { + self.reads_tx_origin && !self.reads_msg_sender + } +} + +impl StandardInvestigatorVisitor for MsgSenderAndTxOriginTracker { + fn visit_any(&mut self, node: &crate::ast::ASTNode) -> eyre::Result<()> { + let member_accesses = ExtractMemberAccesses::from(node).extracted; + + let has_msg_sender = member_accesses.iter().any(|member_access| { + member_access.member_name == "sender" + && if let Expression::Identifier(identifier) = member_access.expression.as_ref() { + identifier.name == "msg" + } else { + false + } + }); + self.reads_msg_sender = self.reads_msg_sender || has_msg_sender; + + let has_tx_origin = member_accesses.iter().any(|member_access| { + member_access.member_name == "origin" + && if let Expression::Identifier(identifier) = member_access.expression.as_ref() { + identifier.name == "tx" + } else { + false + } + }); + self.reads_tx_origin = self.reads_tx_origin || has_tx_origin; + + Ok(()) + } +} + +#[cfg(test)] +mod tx_origin_used_for_auth_detector { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, high::tx_origin_used_for_auth::TxOriginUsedForAuthDetector, + }; + + #[test] + #[serial] + fn test_tx_origin_used_for_auth() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/TxOriginUsedForAuth.sol", + ); + + let mut detector = TxOriginUsedForAuthDetector::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(), 3); + // assert the severity is high + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::High + ); + } +} diff --git a/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index 6cf4758ea..ad6e9c156 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -195,6 +195,7 @@ "weak-randomness", "pre-declared-local-variable-usage", "delete-nested-mapping", + "tx-origin-used-for-auth", "msg-value-in-loop", "contract-locks-ether" ] diff --git a/reports/report.json b/reports/report.json index 60aa6c2e3..0eb69bb87 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 76, - "total_sloc": 2183 + "total_source_units": 77, + "total_sloc": 2225 }, "files_details": { "files_details": [ @@ -193,6 +193,10 @@ "file_path": "src/TestERC20.sol", "n_sloc": 62 }, + { + "file_path": "src/TxOriginUsedForAuth.sol", + "n_sloc": 42 + }, { "file_path": "src/UncheckedReturn.sol", "n_sloc": 33 @@ -312,7 +316,7 @@ ] }, "issue_count": { - "high": 35, + "high": 36, "low": 25 }, "high_issues": { @@ -1850,6 +1854,31 @@ } ] }, + { + "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", + "instances": [ + { + "contract_path": "src/TxOriginUsedForAuth.sol", + "line_no": 40, + "src": "1117:183", + "src_char": "1117:183" + }, + { + "contract_path": "src/TxOriginUsedForAuth.sol", + "line_no": 51, + "src": "1431:90", + "src_char": "1431:90" + }, + { + "contract_path": "src/TxOriginUsedForAuth.sol", + "line_no": 59, + "src": "1610:68", + "src_char": "1610:68" + } + ] + }, { "title": "Loop contains `msg.value`.", "description": "Provide an explicit array of amounts alongside the receivers array, and check that the sum of all amounts matches `msg.value`.", @@ -2250,6 +2279,12 @@ "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/TxOriginUsedForAuth.sol", + "line_no": 2, + "src": "32:24", + "src_char": "32:24" + }, { "contract_path": "src/UncheckedSend.sol", "line_no": 2, @@ -3128,6 +3163,12 @@ "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/TxOriginUsedForAuth.sol", + "line_no": 2, + "src": "32:24", + "src_char": "32:24" + }, { "contract_path": "src/UnsafeERC721Mint.sol", "line_no": 2, @@ -4060,6 +4101,7 @@ "weak-randomness", "pre-declared-local-variable-usage", "delete-nested-mapping", + "tx-origin-used-for-auth", "msg-value-in-loop", "contract-locks-ether" ] diff --git a/reports/report.md b/reports/report.md index ad62f5932..9ef473456 100644 --- a/reports/report.md +++ b/reports/report.md @@ -41,8 +41,9 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [H-31: Weak Randomness](#h-31-weak-randomness) - [H-32: Usage of variable before declaration.](#h-32-usage-of-variable-before-declaration) - [H-33: Deletion from a nested mappping.](#h-33-deletion-from-a-nested-mappping) - - [H-34: Loop contains `msg.value`.](#h-34-loop-contains-msgvalue) - - [H-35: Contract locks Ether without a withdraw function.](#h-35-contract-locks-ether-without-a-withdraw-function) + - [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) - [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) @@ -77,8 +78,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 76 | -| Total nSLOC | 2183 | +| .sol Files | 77 | +| Total nSLOC | 2225 | ## Files Details @@ -132,6 +133,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/TautologicalCompare.sol | 17 | | src/TautologyOrContradiction.sol | 11 | | src/TestERC20.sol | 62 | +| src/TxOriginUsedForAuth.sol | 42 | | src/UncheckedReturn.sol | 33 | | src/UncheckedSend.sol | 18 | | src/UninitializedStateVariable.sol | 29 | @@ -161,14 +163,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** | **2183** | +| **Total** | **2225** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 35 | +| High | 36 | | Low | 25 | @@ -1841,7 +1843,36 @@ A deletion in a structure containing a mapping will not delete the mapping. The -## H-34: Loop contains `msg.value`. +## 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. + +
3 Found Instances + + +- Found in src/TxOriginUsedForAuth.sol [Line: 40](../tests/contract-playground/src/TxOriginUsedForAuth.sol#L40) + + ```solidity + if (tx.origin == owner) { + ``` + +- Found in src/TxOriginUsedForAuth.sol [Line: 51](../tests/contract-playground/src/TxOriginUsedForAuth.sol#L51) + + ```solidity + if (tx.origin == owner || authorizedUsers[tx.origin]) { + ``` + +- Found in src/TxOriginUsedForAuth.sol [Line: 59](../tests/contract-playground/src/TxOriginUsedForAuth.sol#L59) + + ```solidity + require(tx.origin == owner, "Not authorized to perform this action"); + ``` + +
+ + + +## 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`. @@ -1876,7 +1907,7 @@ Provide an explicit array of amounts alongside the receivers array, and check th -## H-35: Contract locks Ether without a withdraw function. +## H-36: 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. @@ -2185,7 +2216,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;` -
22 Found Instances +
23 Found Instances - Found in src/CompilerBugStorageSignedIntegerArray.sol [Line: 2](../tests/contract-playground/src/CompilerBugStorageSignedIntegerArray.sol#L2) @@ -2272,6 +2303,12 @@ Consider using a specific version of Solidity in your contracts instead of a wid pragma solidity ^0.5.0; ``` +- Found in src/TxOriginUsedForAuth.sol [Line: 2](../tests/contract-playground/src/TxOriginUsedForAuth.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + - Found in src/UncheckedSend.sol [Line: 2](../tests/contract-playground/src/UncheckedSend.sol#L2) ```solidity @@ -3071,7 +3108,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. -
30 Found Instances +
31 Found Instances - Found in src/AdminContract.sol [Line: 2](../tests/contract-playground/src/AdminContract.sol#L2) @@ -3170,6 +3207,12 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai pragma solidity 0.8.20; ``` +- Found in src/TxOriginUsedForAuth.sol [Line: 2](../tests/contract-playground/src/TxOriginUsedForAuth.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + - Found in src/UnsafeERC721Mint.sol [Line: 2](../tests/contract-playground/src/UnsafeERC721Mint.sol#L2) ```solidity diff --git a/reports/report.sarif b/reports/report.sarif index 9f48e2311..240ed9ec5 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -2688,6 +2688,48 @@ }, "ruleId": "delete-nested-mapping" }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/TxOriginUsedForAuth.sol" + }, + "region": { + "byteLength": 183, + "byteOffset": 1117 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/TxOriginUsedForAuth.sol" + }, + "region": { + "byteLength": 90, + "byteOffset": 1431 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/TxOriginUsedForAuth.sol" + }, + "region": { + "byteLength": 68, + "byteOffset": 1610 + } + } + } + ], + "message": { + "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" + }, { "level": "warning", "locations": [ @@ -3381,6 +3423,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/TxOriginUsedForAuth.sol" + }, + "region": { + "byteLength": 24, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -4960,6 +5013,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/TxOriginUsedForAuth.sol" + }, + "region": { + "byteLength": 24, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { diff --git a/tests/contract-playground/src/TxOriginUsedForAuth.sol b/tests/contract-playground/src/TxOriginUsedForAuth.sol new file mode 100644 index 000000000..0be0f8e02 --- /dev/null +++ b/tests/contract-playground/src/TxOriginUsedForAuth.sol @@ -0,0 +1,62 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +contract VulnerableContract { + address public owner; + mapping(address => bool) public authorizedUsers; + + event Authenticated(address indexed user); + event UnauthorizedAccess(address indexed user); + + constructor() { + owner = msg.sender; + } + + // GOOD + function authorizeUser(address _user) external { + require(msg.sender == owner, "Only owner can authorize users"); + authorizedUsers[_user] = true; + } + + // GOOD + function revokeUser(address _user) external { + require(msg.sender == owner, "Only owner can revoke users"); + authorizedUsers[_user] = false; + } + + // GOOD + function edgeCase() external { + // This uses both `tx.origin` as well as `msg.sender` so, it's OK (Same heuristic as slither) + require( + tx.origin == msg.sender && msg.sender == owner, + "Not authorized to perform this action" + ); + emit Authenticated(msg.sender); + } + + // BAD + function secureAction() external { + // Vulnerable use of tx.origin + if (tx.origin == owner) { + emit Authenticated(msg.sender); + } else { + emit UnauthorizedAccess(msg.sender); + revert("Not authorized"); + } + } + + // BAD + function checkAuthorization() external view returns (bool) { + // Vulnerable use of tx.origin + if (tx.origin == owner || authorizedUsers[tx.origin]) { + return true; + } + return false; + } + + // BAD + function performAction() external { + require(tx.origin == owner, "Not authorized to perform this action"); + emit Authenticated(msg.sender); + } +}