Uniswap V2 Timestamp Deadline

report.md

@@ -6,6 +6,7 @@
 - [Medium Issues](#medium-issues)
   - [M-1: Centralization Risk for trusted owners](#M-1)
   - [M-2: Solmate's SafeTransferLib does not check for token contract's existence](#M-2)
+  - [M-3: Using `block.timestamp` for swap deadline offers no protection](#M-3)
 - [Low Issues](#low-issues)
   - [L-1: `abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()`](#L-1)
   - [L-2: `ecrecover` is susceptible to signature malleability](#L-2)
@@ -32,6 +33,7 @@ Contracts analyzed:
 - "src/AdminContract.sol"
 - "src/DeprecatedOZFunctions.sol"
 - "src/inheritance/InheritanceBase.sol"
+- "src/UniswapV2Swapper.sol"
 - "src/inheritance/IContractInheritance.sol"
 - "src/KeccakContract.sol"
 
@@ -72,6 +74,22 @@ https://github.com/transmissions11/solmate/blob/main/src/utils/SafeTransferLib.s
 - Found in src/DeprecatedOZFunctions.sol: Line: 27
 
 
+<a name="M-3"></a>
+## M-3: Using `block.timestamp` for swap deadline offers no protection
+
+In the PoS model, proposers know well in advance if they will propose one or consecutive blocks ahead of time. In such a scenario, a malicious validator can hold back the transaction and execute it at a more favourable block number.Consider allowing function caller to specify swap deadline input parameter.
+
+- Found in src/UniswapV2Swapper.sol: Line: 25
+- Found in src/UniswapV2Swapper.sol: Line: 32
+- Found in src/UniswapV2Swapper.sol: Line: 26
+- Found in src/UniswapV2Swapper.sol: Line: 24
+- Found in src/UniswapV2Swapper.sol: Line: 33
+- Found in src/UniswapV2Swapper.sol: Line: 28
+- Found in src/UniswapV2Swapper.sol: Line: 27
+- Found in src/UniswapV2Swapper.sol: Line: 34
+- Found in src/UniswapV2Swapper.sol: Line: 29
+
+
 # Low Issues
 
 <a name="L-1"></a>
@@ -107,10 +125,10 @@ Openzeppelin has deprecated several functions and replaced with newer versions.
 
 ERC20 functions may not behave as expected. For example: return values are not always meaningful. It is recommended to use OpenZeppelin's SafeERC20 library.
 
-- Found in src/DeprecatedOZFunctions.sol: Line: 47
 - Found in src/DeprecatedOZFunctions.sol: Line: 42
-- Found in src/DeprecatedOZFunctions.sol: Line: 38
 - Found in src/DeprecatedOZFunctions.sol: Line: 37
+- Found in src/DeprecatedOZFunctions.sol: Line: 47
+- Found in src/DeprecatedOZFunctions.sol: Line: 38
 - Found in src/DeprecatedOZFunctions.sol: Line: 32
 
 
@@ -119,8 +137,8 @@ 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;`
 
-- Found in src/inheritance/IContractInheritance.sol: Line: 2
 - Found in src/inheritance/InheritanceBase.sol: Line: 2
+- Found in src/inheritance/IContractInheritance.sol: Line: 2
 - Found in src/Counter.sol: Line: 2
 
 
@@ -131,6 +149,7 @@ Consider using a specific version of Solidity in your contracts instead of a wid
 
 Assigning values to address state variables without checking for `address(0)`.
 
+- Found in src/UniswapV2Swapper.sol: Line: 12
 - Found in src/StateVariables.sol: Line: 58
 
 
@@ -139,23 +158,26 @@ Assigning values to address state variables without checking for `address(0)`.
 
 
 
-- Found in src/Counter.sol: Line: 7
-- Found in src/StateVariables.sol: Line: 57
-- Found in src/StateVariables.sol: Line: 39
+- Found in src/UniswapV2Swapper.sol: Line: 15
+- Found in src/UniswapV2Swapper.sol: Line: 37
+- Found in src/UniswapV2Swapper.sol: Line: 11
 - Found in src/AdminContract.sol: Line: 9
 - Found in src/StateVariables.sol: Line: 52
-- Found in src/StateVariables.sol: Line: 67
-- Found in src/StateVariables.sol: Line: 47
+- Found in src/StateVariables.sol: Line: 57
+- Found in src/StateVariables.sol: Line: 39
 - Found in src/StateVariables.sol: Line: 61
+- Found in src/StateVariables.sol: Line: 47
+- Found in src/StateVariables.sol: Line: 67
+- Found in src/Counter.sol: Line: 7
 
 
 <a name="NC-3"></a>
 ## NC-3: Constants should be defined and used instead of literals
 
 
 
-- Found in src/Counter.sol: Line: 23
 - Found in src/inheritance/ExtendedInheritance.sol: Line: 15
+- Found in src/Counter.sol: Line: 23
 
 
 <a name="NC-4"></a>

src/context/loader.rs

@@ -244,6 +244,10 @@ impl ContextLoader {
         self.function_definitions.keys().collect()
     }
 
+    pub fn get_function_calls(&self) -> Vec<&FunctionCall> {
+        self.function_calls.keys().collect()
+    }
+
     pub fn get_member_accesses(&self) -> Vec<&MemberAccess> {
         self.member_accesses.keys().collect()
     }
@@ -318,7 +322,7 @@ impl ContextLoader {
         };
 
         // iterate through self.source_units until the source unit with the id matching `source_unit_id` is found, then return its `absolute_path`
-        
+
         source_unit_id.and_then(|&id| {
             self.source_units
                 .iter()

src/detect/detector.rs

@@ -9,6 +9,7 @@ use crate::{
             unspecific_solidity_pragma::UnspecificSolidityPragmaDetector,
         },
         medium::{
+            block_timestamp_deadline::BlockTimestampDeadlineDetector,
             centralization_risk::CentralizationRiskDetector,
             solmate_safe_transfer_lib::SolmateSafeTransferLibDetector,
         },
@@ -40,6 +41,7 @@ pub fn get_all_detectors() -> Vec<Box<dyn Detector>> {
         Box::<UnindexedEventsDetector>::default(),
         Box::<RequireWithStringDetector>::default(),
         Box::<NonReentrantBeforeOthersDetector>::default(),
+        Box::<BlockTimestampDeadlineDetector>::default(),
     ]
 }
 

src/detect/medium/block_timestamp_deadline.rs

@@ -0,0 +1,119 @@
+use std::error::Error;
+
+use crate::{
+    ast::Expression,
+    context::loader::{ASTNode, ContextLoader},
+    detect::detector::{Detector, IssueSeverity},
+};
+use eyre::Result;
+
+#[derive(Default)]
+pub struct BlockTimestampDeadlineDetector {
+    found_block_timestamp_deadlines: Vec<Option<ASTNode>>,
+}
+
+impl Detector for BlockTimestampDeadlineDetector {
+    fn detect(&mut self, loader: &ContextLoader) -> Result<bool, Box<dyn Error>> {
+        // Uniswap V2 - Function Calls
+        // For each FunctionCall, if the Expression is a MemberAccess that is named any of the following:
+        // [
+        //  swapExactTokensForTokens, swapTokensForExactTokens, swapExactETHForTokens, swapTokensForExactETH,
+        //  swapExactTokensForETH, swapETHForExactTokens, swapExactTokensForTokensSupportingFeeOnTransferTokens,
+        //  swapExactETHForTokensSupportingFeeOnTransferTokens, swapExactTokensForETHSupportingFeeOnTransferTokens
+        // ]
+        // If the last FunctionCall argument is a MemberAccess identifier with member_name "timestamp",
+        // and the MemberAccess expression.name is "block", add the node to the found_block_timestamp_deadlines vector.
+        let function_calls = loader.get_function_calls();
+        for call in function_calls {
+            if let Expression::MemberAccess(ref member_access) = *call.expression {
+                if member_access.member_name == "swapExactTokensForTokens"
+                    || member_access.member_name == "swapTokensForExactTokens"
+                    || member_access.member_name == "swapExactETHForTokens"
+                    || member_access.member_name == "swapTokensForExactETH"
+                    || member_access.member_name == "swapExactTokensForETH"
+                    || member_access.member_name == "swapETHForExactTokens"
+                    || member_access.member_name
+                        == "swapExactTokensForTokensSupportingFeeOnTransferTokens"
+                    || member_access.member_name
+                        == "swapExactETHForTokensSupportingFeeOnTransferTokens"
+                    || member_access.member_name
+                        == "swapExactTokensForETHSupportingFeeOnTransferTokens"
+                {
+                    if let Expression::MemberAccess(ref member_access) =
+                        call.arguments.last().unwrap()
+                    {
+                        if member_access.member_name == "timestamp" {
+                            if let Expression::Identifier(ref identifier) =
+                                *member_access.expression
+                            {
+                                if identifier.name == "block" {
+                                    self.found_block_timestamp_deadlines
+                                        .push(Some(ASTNode::FunctionCall(call.clone())));
+                                }
+                            }
+                        }
+                    }
+                }
+            }
+        }
+
+        // TODO: Uniswap V3 - Struct definitions
+        Ok(!self.found_block_timestamp_deadlines.is_empty())
+    }
+
+    fn severity(&self) -> IssueSeverity {
+        IssueSeverity::Medium
+    }
+
+    fn title(&self) -> String {
+        "Using `block.timestamp` for swap deadline offers no protection".to_string()
+    }
+
+    fn description(&self) -> String {
+        "In the PoS model, proposers know well in advance if they will propose one or consecutive blocks ahead of time. In such a scenario, a malicious validator can hold back the transaction and execute it at a more favourable block number.\
+        Consider allowing function caller to specify swap deadline input parameter.".to_string()
+    }
+
+    fn instances(&self) -> Vec<Option<ASTNode>> {
+        self.found_block_timestamp_deadlines.clone()
+    }
+}
+
+#[cfg(test)]
+mod block_timestamp_deadline_detector_tests {
+    use crate::detect::{
+        detector::{detector_test_helpers::load_contract, Detector},
+        medium::block_timestamp_deadline::BlockTimestampDeadlineDetector,
+    };
+
+    #[test]
+    fn test_block_timestamp_deadline_detector() {
+        let context_loader = load_contract(
+            "./tests/contract-playground/out/UniswapV2Swapper.sol/UniswapV2Swapper.json",
+        );
+        let mut detector = BlockTimestampDeadlineDetector::default();
+        let found = detector.detect(&context_loader).unwrap();
+        // assert that the detector found
+        assert!(found);
+        // assert that the number of instances found is 2
+        assert_eq!(detector.instances().len(), 9);
+        // assert that the severity is medium
+        assert_eq!(
+            detector.severity(),
+            crate::detect::detector::IssueSeverity::Medium
+        );
+        // assert that the title is correct
+        assert_eq!(
+            detector.title(),
+            String::from("Using `block.timestamp` for swap deadline offers no protection")
+        );
+        // assert that the description is correct
+        assert_eq!(
+            detector.description(),
+            String::from(
+                "In the PoS model, proposers know well in advance if they will propose one or consecutive blocks ahead of time. In such a scenario, a malicious validator can hold back the transaction and execute it at a more favourable block number.\
+        Consider allowing function caller to specify swap deadline input parameter."
+            )
+        );
+    }
+}

src/detect/medium/mod.rs

@@ -1,2 +1,3 @@
+pub mod block_timestamp_deadline;
 pub mod centralization_risk;
 pub mod solmate_safe_transfer_lib;