forked from Cyfrin/aderyn
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Detector: Bad use of
tx.origin (Cyfrin#642)
Co-authored-by: Alex Roan <alex@cyfrin.io>
- Loading branch information
1 parent
c9c1bd1
commit 4454c90
Showing
8 changed files
with
403 additions
and
13 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<bool, Box<dyn Error>> { | ||
| 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::<Vec<ASTNode>>(); | ||
|
|
||
| let ast_nodes: &[&ASTNode] = &(arguments.iter().collect::<Vec<_>>()); | ||
| 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<dyn Error>> { | ||
| // 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 | ||
| ); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.