diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index 1302f49f..193a64fd 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -83,6 +83,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), Box::::default(), ] } @@ -163,6 +164,7 @@ pub(crate) enum IssueDetectorNamePool { ContractLocksEther, IncorrectERC20Interface, ReturnBomb, + OutOfOrderRetryable, // NOTE: `Undecided` will be the default name (for new bots). // If it's accepted, a new variant will be added to this enum before normalizing it in aderyn Undecided, @@ -172,6 +174,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { + Some(Box::::default()) + } IssueDetectorNamePool::FunctionInitializingState => { Some(Box::::default()) } diff --git a/aderyn_core/src/detect/high/mod.rs b/aderyn_core/src/detect/high/mod.rs index d6ce00ea..b19236ac 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -17,6 +17,7 @@ pub(crate) mod misused_boolean; pub(crate) mod msg_value_in_loops; pub(crate) mod multiple_constructors; pub(crate) mod nested_struct_in_mapping; +pub(crate) mod out_of_order_retryable; pub(crate) mod pre_declared_variable_usage; pub(crate) mod reused_contract_name; pub(crate) mod rtlo; @@ -55,6 +56,7 @@ pub use misused_boolean::MisusedBooleanDetector; pub use msg_value_in_loops::MsgValueUsedInLoopDetector; pub use multiple_constructors::MultipleConstructorsDetector; pub use nested_struct_in_mapping::NestedStructInMappingDetector; +pub use out_of_order_retryable::OutOfOrderRetryableDetector; pub use pre_declared_variable_usage::PreDeclaredLocalVariableUsageDetector; pub use reused_contract_name::ReusedContractNameDetector; pub use rtlo::RTLODetector; diff --git a/aderyn_core/src/detect/high/out_of_order_retryable.rs b/aderyn_core/src/detect/high/out_of_order_retryable.rs new file mode 100644 index 00000000..a3910226 --- /dev/null +++ b/aderyn_core/src/detect/high/out_of_order_retryable.rs @@ -0,0 +1,126 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::{Expression, MemberAccess, NodeID}; + +use crate::capture; +use crate::context::browser::ExtractFunctionCalls; +use crate::context::investigator::{ + StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor, +}; +use crate::detect::detector::IssueDetectorNamePool; +use crate::detect::helpers; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct OutOfOrderRetryableDetector { + // 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 OutOfOrderRetryableDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + for func in helpers::get_implemented_external_and_public_functions(context) { + let mut tracker = OutOfOrderRetryableTracker { + number_of_retry_calls: 0, + }; + let investigator = StandardInvestigator::new( + context, + &[&(func.into())], + StandardInvestigationStyle::Downstream, + )?; + investigator.investigate(context, &mut tracker)?; + if tracker.number_of_retry_calls >= 2 { + capture!(self, context, func); + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("Out of order retryable transactions.") + } + + fn description(&self) -> String { + String::from("Do not rely on the order or successful execution of retryable tickets. Functions like \ + createRetryableTicket, outboundTransferCustomRefund, unsafeCreateRetryableTicket are free to be re-tried in any + order if they fail in the first go. Since this operation happens off chain, the sequencer is in control of the + order of these transactions. Therefore, restrict the use to at most 1 ticket call per function.") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + format!("{}", IssueDetectorNamePool::OutOfOrderRetryable) + } +} + +struct OutOfOrderRetryableTracker { + number_of_retry_calls: usize, +} + +const SEQUENCER_FUNCTIONS: [&str; 3] = [ + "createRetryableTicket", + "outboundTransferCustomRefund", + "unsafeCreateRetryableTicket", +]; + +impl StandardInvestigatorVisitor for OutOfOrderRetryableTracker { + fn visit_any(&mut self, node: &crate::ast::ASTNode) -> eyre::Result<()> { + if self.number_of_retry_calls >= 2 { + return Ok(()); + } + let function_calls = ExtractFunctionCalls::from(node).extracted; + for func_call in function_calls { + if let Expression::MemberAccess(MemberAccess { member_name, .. }) = + func_call.expression.as_ref() + { + if SEQUENCER_FUNCTIONS.iter().any(|f| f == member_name) { + self.number_of_retry_calls += 1; + } + } + } + Ok(()) + } +} + +#[cfg(test)] +mod out_of_order_retryable_tests { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, high::out_of_order_retryable::OutOfOrderRetryableDetector, + }; + + #[test] + #[serial] + fn test_out_of_order_retryable() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/OutOfOrderRetryable.sol", + ); + + let mut detector = OutOfOrderRetryableDetector::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/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index 593df15a..10b67692 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -198,6 +198,7 @@ "tx-origin-used-for-auth", "msg-value-in-loop", "contract-locks-ether", - "incorrect-erc20-interface" + "incorrect-erc20-interface", + "out-of-order-retryable" ] } \ No newline at end of file diff --git a/reports/report.json b/reports/report.json index 440b799e..2661bf6c 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 83, - "total_sloc": 2480 + "total_source_units": 84, + "total_sloc": 2645 }, "files_details": { "files_details": [ @@ -153,6 +153,10 @@ "file_path": "src/OnceModifierExample.sol", "n_sloc": 8 }, + { + "file_path": "src/OutOfOrderRetryable.sol", + "n_sloc": 165 + }, { "file_path": "src/PreDeclaredVarUsage.sol", "n_sloc": 9 @@ -340,7 +344,7 @@ ] }, "issue_count": { - "high": 37, + "high": 38, "low": 30 }, "high_issues": { @@ -1619,6 +1623,30 @@ "src": "6342:11", "src_char": "6342:11" }, + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 63, + "src": "1649:11", + "src_char": "1649:11" + }, + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 90, + "src": "2557:22", + "src_char": "2557:22" + }, + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 167, + "src": "4685:13", + "src_char": "4685:13" + }, + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 175, + "src": "5072:7", + "src_char": "5072:7" + }, { "contract_path": "src/ReturnBomb.sol", "line_no": 32, @@ -1749,6 +1777,42 @@ "description": "Function returns a value but it is ignored.", "detector_name": "unchecked-return", "instances": [ + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 65, + "src": "1705:390", + "src_char": "1705:390" + }, + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 77, + "src": "2129:379", + "src_char": "2129:379" + }, + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 92, + "src": "2624:390", + "src_char": "2624:390" + }, + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 107, + "src": "3107:379", + "src_char": "3107:379" + }, + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 129, + "src": "3777:208", + "src_char": "3777:208" + }, + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 151, + "src": "4337:261", + "src_char": "4337:261" + }, { "contract_path": "src/UncheckedReturn.sol", "line_no": 14, @@ -2085,6 +2149,25 @@ "src_char": "1249:9" } ] + }, + { + "title": "Out of order retryable transactions.", + "description": "Do not rely on the order or successful execution of retryable tickets. Functions like createRetryableTicket, outboundTransferCustomRefund, unsafeCreateRetryableTicket are free to be re-tried in any\n order if they fail in the first go. Since this operation happens off chain, the sequencer is in control of the\n order of these transactions. Therefore, restrict the use to at most 1 ticket call per function.", + "detector_name": "out-of-order-retryable", + "instances": [ + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 63, + "src": "1649:11", + "src_char": "1649:11" + }, + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 90, + "src": "2557:22", + "src_char": "2557:22" + } + ] } ] }, @@ -2279,6 +2362,18 @@ "src": "1598:18", "src_char": "1598:18" }, + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 171, + "src": "4927:28", + "src_char": "4927:28" + }, + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 180, + "src": "5328:28", + "src_char": "5328:28" + }, { "contract_path": "src/SendEtherNoChecks.sol", "line_no": 67, @@ -2388,6 +2483,12 @@ "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/PreDeclaredVarUsage.sol", "line_no": 2, @@ -2479,6 +2580,18 @@ "src": "267:15", "src_char": "267:15" }, + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 54, + "src": "1416:14", + "src_char": "1416:14" + }, + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 55, + "src": "1440:24", + "src_char": "1440:24" + }, { "contract_path": "src/StateVariables.sol", "line_no": 58, @@ -2606,6 +2719,18 @@ "src": "1249:9", "src_char": "1249:9" }, + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 167, + "src": "4685:13", + "src_char": "4685:13" + }, + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 175, + "src": "5072:7", + "src_char": "5072:7" + }, { "contract_path": "src/ReturnBomb.sol", "line_no": 16, @@ -3416,6 +3541,12 @@ "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/ReturnBomb.sol", "line_no": 2, @@ -3706,6 +3837,12 @@ "src": "147:7", "src_char": "147:7" }, + { + "contract_path": "src/OutOfOrderRetryable.sol", + "line_no": 193, + "src": "5775:13", + "src_char": "5775:13" + }, { "contract_path": "src/SendEtherNoChecks.sol", "line_no": 53, @@ -4667,6 +4804,7 @@ "contract-locks-ether", "incorrect-erc20-interface", "return-bomb", + "out-of-order-retryable", "function-initializing-state" ] } \ No newline at end of file diff --git a/reports/report.md b/reports/report.md index b76291bb..d742675d 100644 --- a/reports/report.md +++ b/reports/report.md @@ -45,6 +45,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [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) - [H-37: Incorrect ERC20 interface.](#h-37-incorrect-erc20-interface) + - [H-38: Out of order retryable transactions.](#h-38-out-of-order-retryable-transactions) - [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) @@ -84,8 +85,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 83 | -| Total nSLOC | 2480 | +| .sol Files | 84 | +| Total nSLOC | 2645 | ## Files Details @@ -129,6 +130,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/MsgValueInLoop.sol | 55 | | src/MultipleConstructorSchemes.sol | 10 | | src/OnceModifierExample.sol | 8 | +| src/OutOfOrderRetryable.sol | 165 | | src/PreDeclaredVarUsage.sol | 9 | | src/PublicVariableReadInExternalContext.sol | 32 | | src/RTLO.sol | 7 | @@ -175,14 +177,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** | **2480** | +| **Total** | **2645** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 37 | +| High | 38 | | Low | 30 | @@ -1517,7 +1519,7 @@ The patterns `if (… || true)` and `if (.. && false)` will always evaluate to t Introduce checks for `msg.sender` in the function -
14 Found Instances +
18 Found Instances - Found in src/CallGraphTests.sol [Line: 38](../tests/contract-playground/src/CallGraphTests.sol#L38) @@ -1550,6 +1552,30 @@ Introduce checks for `msg.sender` in the function function takeEthBack(uint256 amount) external { ``` +- Found in src/OutOfOrderRetryable.sol [Line: 63](../tests/contract-playground/src/OutOfOrderRetryable.sol#L63) + + ```solidity + function doStuffOnL2() external { + ``` + +- Found in src/OutOfOrderRetryable.sol [Line: 90](../tests/contract-playground/src/OutOfOrderRetryable.sol#L90) + + ```solidity + function doStuffOnL2Alternative() external { + ``` + +- Found in src/OutOfOrderRetryable.sol [Line: 167](../tests/contract-playground/src/OutOfOrderRetryable.sol#L167) + + ```solidity + function claim_rewards() public { + ``` + +- Found in src/OutOfOrderRetryable.sol [Line: 175](../tests/contract-playground/src/OutOfOrderRetryable.sol#L175) + + ```solidity + function unstake() public { + ``` + - Found in src/ReturnBomb.sol [Line: 32](../tests/contract-playground/src/ReturnBomb.sol#L32) ```solidity @@ -1693,8 +1719,44 @@ Right to left override character may be misledaing and cause potential attacks b Function returns a value but it is ignored. -
2 Found Instances +
8 Found Instances + +- Found in src/OutOfOrderRetryable.sol [Line: 65](../tests/contract-playground/src/OutOfOrderRetryable.sol#L65) + + ```solidity + IInbox(inbox).createRetryableTicket({ + ``` + +- Found in src/OutOfOrderRetryable.sol [Line: 77](../tests/contract-playground/src/OutOfOrderRetryable.sol#L77) + + ```solidity + IInbox(inbox).createRetryableTicket({ + ``` + +- Found in src/OutOfOrderRetryable.sol [Line: 92](../tests/contract-playground/src/OutOfOrderRetryable.sol#L92) + + ```solidity + IInbox(inbox).createRetryableTicket({ + ``` + +- Found in src/OutOfOrderRetryable.sol [Line: 107](../tests/contract-playground/src/OutOfOrderRetryable.sol#L107) + + ```solidity + IInbox(inbox).createRetryableTicket({ + ``` + +- Found in src/OutOfOrderRetryable.sol [Line: 129](../tests/contract-playground/src/OutOfOrderRetryable.sol#L129) + + ```solidity + IInbox(inbox).outboundTransferCustomRefund( + ``` + +- Found in src/OutOfOrderRetryable.sol [Line: 151](../tests/contract-playground/src/OutOfOrderRetryable.sol#L151) + + ```solidity + IInbox(inbox).unsafeCreateRetryableTicket( + ``` - Found in src/UncheckedReturn.sol [Line: 14](../tests/contract-playground/src/UncheckedReturn.sol#L14) @@ -2079,6 +2141,31 @@ Incorrect return values for ERC20 functions. A contract compiled with Solidity > +## H-38: Out of order retryable transactions. + +Do not rely on the order or successful execution of retryable tickets. Functions like createRetryableTicket, outboundTransferCustomRefund, unsafeCreateRetryableTicket are free to be re-tried in any + order if they fail in the first go. Since this operation happens off chain, the sequencer is in control of the + order of these transactions. Therefore, restrict the use to at most 1 ticket call per function. + +
2 Found Instances + + +- Found in src/OutOfOrderRetryable.sol [Line: 63](../tests/contract-playground/src/OutOfOrderRetryable.sol#L63) + + ```solidity + function doStuffOnL2() external { + ``` + +- Found in src/OutOfOrderRetryable.sol [Line: 90](../tests/contract-playground/src/OutOfOrderRetryable.sol#L90) + + ```solidity + function doStuffOnL2Alternative() external { + ``` + +
+ + + # Low Issues ## L-1: Centralization Risk for trusted owners @@ -2234,7 +2321,7 @@ 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. -
12 Found Instances +
14 Found Instances - Found in src/ArbitraryTransferFrom.sol [Line: 16](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L16) @@ -2291,6 +2378,18 @@ ERC20 functions may not behave as expected. For example: return values are not a token.transferFrom(from, to, value); ``` +- Found in src/OutOfOrderRetryable.sol [Line: 171](../tests/contract-playground/src/OutOfOrderRetryable.sol#L171) + + ```solidity + payable(msg.sender).transfer(unclaimed_rewards); + ``` + +- Found in src/OutOfOrderRetryable.sol [Line: 180](../tests/contract-playground/src/OutOfOrderRetryable.sol#L180) + + ```solidity + payable(msg.sender).transfer(user_balance); + ``` + - Found in src/SendEtherNoChecks.sol [Line: 67](../tests/contract-playground/src/SendEtherNoChecks.sol#L67) ```solidity @@ -2317,7 +2416,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;` -
27 Found Instances +
28 Found Instances - Found in src/CompilerBugStorageSignedIntegerArray.sol [Line: 2](../tests/contract-playground/src/CompilerBugStorageSignedIntegerArray.sol#L2) @@ -2404,6 +2503,12 @@ Consider using a specific version of Solidity in your contracts instead of a wid pragma solidity ^0.8.0; ``` +- Found in src/OutOfOrderRetryable.sol [Line: 2](../tests/contract-playground/src/OutOfOrderRetryable.sol#L2) + + ```solidity + pragma solidity ^0.8.0; + ``` + - Found in src/PreDeclaredVarUsage.sol [Line: 2](../tests/contract-playground/src/PreDeclaredVarUsage.sol#L2) ```solidity @@ -2490,7 +2595,7 @@ Consider using a specific version of Solidity in your contracts instead of a wid Check for `address(0)` when assigning values to address state variables. -
6 Found Instances +
8 Found Instances - Found in src/ArbitraryTransferFrom.sol [Line: 12](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L12) @@ -2499,6 +2604,18 @@ Check for `address(0)` when assigning values to address state variables. s_token = token; ``` +- Found in src/OutOfOrderRetryable.sol [Line: 54](../tests/contract-playground/src/OutOfOrderRetryable.sol#L54) + + ```solidity + inbox = _inbox; + ``` + +- Found in src/OutOfOrderRetryable.sol [Line: 55](../tests/contract-playground/src/OutOfOrderRetryable.sol#L55) + + ```solidity + l2contract = _l2contract; + ``` + - Found in src/StateVariables.sol [Line: 58](../tests/contract-playground/src/StateVariables.sol#L58) ```solidity @@ -2537,7 +2654,7 @@ Check for `address(0)` when assigning values to address state variables. Instead of marking a function as `public`, consider marking it as `external` if it is not used internally. -
38 Found Instances +
40 Found Instances - Found in src/ArbitraryTransferFrom.sol [Line: 28](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L28) @@ -2630,6 +2747,18 @@ Instead of marking a function as `public`, consider marking it as `external` if function allowance( ``` +- Found in src/OutOfOrderRetryable.sol [Line: 167](../tests/contract-playground/src/OutOfOrderRetryable.sol#L167) + + ```solidity + function claim_rewards() public { + ``` + +- Found in src/OutOfOrderRetryable.sol [Line: 175](../tests/contract-playground/src/OutOfOrderRetryable.sol#L175) + + ```solidity + function unstake() public { + ``` + - Found in src/ReturnBomb.sol [Line: 16](../tests/contract-playground/src/ReturnBomb.sol#L16) ```solidity @@ -3359,7 +3488,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. -
34 Found Instances +
35 Found Instances - Found in src/AdminContract.sol [Line: 2](../tests/contract-playground/src/AdminContract.sol#L2) @@ -3440,6 +3569,12 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai pragma solidity ^0.8.0; ``` +- Found in src/OutOfOrderRetryable.sol [Line: 2](../tests/contract-playground/src/OutOfOrderRetryable.sol#L2) + + ```solidity + pragma solidity ^0.8.0; + ``` + - Found in src/ReturnBomb.sol [Line: 2](../tests/contract-playground/src/ReturnBomb.sol#L2) ```solidity @@ -3657,7 +3792,7 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai Consider removing empty blocks. -
27 Found Instances +
28 Found Instances - Found in src/AdminContract.sol [Line: 14](../tests/contract-playground/src/AdminContract.sol#L14) @@ -3738,6 +3873,12 @@ Consider removing empty blocks. function perform() external onlyOnce { ``` +- Found in src/OutOfOrderRetryable.sol [Line: 193](../tests/contract-playground/src/OutOfOrderRetryable.sol#L193) + + ```solidity + function _free_rewards(address user) internal { + ``` + - Found in src/SendEtherNoChecks.sol [Line: 53](../tests/contract-playground/src/SendEtherNoChecks.sol#L53) ```solidity diff --git a/reports/report.sarif b/reports/report.sarif index b4167d74..e14bc958 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -2253,6 +2253,50 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 11, + "byteOffset": 1649 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 22, + "byteOffset": 2557 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 13, + "byteOffset": 4685 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 7, + "byteOffset": 5072 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -2476,6 +2520,72 @@ { "level": "warning", "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 390, + "byteOffset": 1705 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 379, + "byteOffset": 2129 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 390, + "byteOffset": 2624 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 379, + "byteOffset": 3107 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 208, + "byteOffset": 3777 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 261, + "byteOffset": 4337 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3054,6 +3164,37 @@ }, "ruleId": "incorrect-erc20-interface" }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 11, + "byteOffset": 1649 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 22, + "byteOffset": 2557 + } + } + } + ], + "message": { + "text": "Do not rely on the order or successful execution of retryable tickets. Functions like createRetryableTicket, outboundTransferCustomRefund, unsafeCreateRetryableTicket are free to be re-tried in any\n order if they fail in the first go. Since this operation happens off chain, the sequencer is in control of the\n order of these transactions. Therefore, restrict the use to at most 1 ticket call per function." + }, + "ruleId": "out-of-order-retryable" + }, { "level": "note", "locations": [ @@ -3379,6 +3520,28 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 28, + "byteOffset": 4927 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 28, + "byteOffset": 5328 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3575,6 +3738,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3738,6 +3912,28 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 14, + "byteOffset": 1416 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 24, + "byteOffset": 1440 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3967,6 +4163,28 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 13, + "byteOffset": 4685 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 7, + "byteOffset": 5072 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -5429,6 +5647,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -5953,6 +6182,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/OutOfOrderRetryable.sol" + }, + "region": { + "byteLength": 13, + "byteOffset": 5775 + } + } + }, { "physicalLocation": { "artifactLocation": { diff --git a/tests/contract-playground/src/OutOfOrderRetryable.sol b/tests/contract-playground/src/OutOfOrderRetryable.sol new file mode 100644 index 00000000..4013bd6c --- /dev/null +++ b/tests/contract-playground/src/OutOfOrderRetryable.sol @@ -0,0 +1,201 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +interface IInbox { + function createRetryableTicket( + address to, + uint256 l2CallValue, + uint256 maxSubmissionCost, + address excessFeeRefundAddress, + address callValueRefundAddress, + uint256 gasLimit, + uint256 maxFeePerGas, + bytes calldata data + ) external payable returns (uint256); + + function outboundTransferCustomRefund( + address l1Token, + address to, + uint256 amount, + uint256 maxGas, + uint256 gasPriceBid, + bytes calldata data, + address excessFeeRefundAddress + ) external payable returns (bytes memory); + + function unsafeCreateRetryableTicket( + address to, + uint256 l2CallValue, + uint256 maxSubmissionCost, + address excessFeeRefundAddress, + address callValueRefundAddress, + uint256 gasLimit, + uint256 maxFeePerGas, + bytes calldata data + ) external payable returns (uint256); +} + +contract L1 { + address public inbox; + address public l2contract; + uint256 public maxSubmissionCost; + uint256 public gasLimit; + uint256 public maxFeePerGas; + uint256 public gas; + + constructor( + address _inbox, + address _l2contract, + uint256 _maxSubmissionCost, + uint256 _gasLimit, + uint256 _maxFeePerGas, + uint256 _gas + ) { + inbox = _inbox; + l2contract = _l2contract; + maxSubmissionCost = _maxSubmissionCost; + gasLimit = _gasLimit; + maxFeePerGas = _maxFeePerGas; + gas = _gas; + } + + // BAD (2 occurrences) + function doStuffOnL2() external { + // Retryable A + IInbox(inbox).createRetryableTicket({ + to: l2contract, + l2CallValue: 0, + maxSubmissionCost: maxSubmissionCost, + excessFeeRefundAddress: msg.sender, + callValueRefundAddress: msg.sender, + gasLimit: gasLimit, + maxFeePerGas: maxFeePerGas, + data: abi.encodeWithSelector(L2.claim_rewards.selector) + }); + + // Retryable B + IInbox(inbox).createRetryableTicket({ + to: l2contract, + l2CallValue: 0, + maxSubmissionCost: maxSubmissionCost, + excessFeeRefundAddress: msg.sender, + callValueRefundAddress: msg.sender, + gasLimit: gas, + maxFeePerGas: maxFeePerGas, + data: abi.encodeWithSelector(L2.unstake.selector) + }); + } + + // BAD (2 occurrences) + function doStuffOnL2Alternative() external { + // Retryable A + IInbox(inbox).createRetryableTicket({ + to: l2contract, + l2CallValue: 0, + maxSubmissionCost: maxSubmissionCost, + excessFeeRefundAddress: msg.sender, + callValueRefundAddress: msg.sender, + gasLimit: gasLimit, + maxFeePerGas: maxFeePerGas, + data: abi.encodeWithSelector(L2.claim_rewards.selector) + }); + _helper(); + } + + function _helper() internal { + // Retryable B + IInbox(inbox).createRetryableTicket({ + to: l2contract, + l2CallValue: 0, + maxSubmissionCost: maxSubmissionCost, + excessFeeRefundAddress: msg.sender, + callValueRefundAddress: msg.sender, + gasLimit: gas, + maxFeePerGas: maxFeePerGas, + data: abi.encodeWithSelector(L2.unstake.selector) + }); + } + + // GOOD (1 occurrence only) + function customRefund( + address l1Token, + address to, + uint256 amount, + uint256 maxGas, + uint256 gasPriceBid, + bytes calldata data, + address excessFeeRefundAddress + ) external payable { + IInbox(inbox).outboundTransferCustomRefund( + l1Token, + to, + amount, + maxGas, + gasPriceBid, + data, + excessFeeRefundAddress + ); + } + + // GOOD (1 occurrence only) + function unsafeRetryable( + address to, + uint256 l2CallValue, + uint256 maxSubmissionCost, + address excessFeeRefundAddress, + address callValueRefundAddress, + uint256 gasLimit, + uint256 maxFeePerGas, + bytes calldata data + ) external payable { + IInbox(inbox).unsafeCreateRetryableTicket( + to, + l2CallValue, + maxSubmissionCost, + excessFeeRefundAddress, + callValueRefundAddress, + gasLimit, + maxFeePerGas, + data + ); + } +} + +contract L2 { + mapping(address => uint256) public balance; + + function claim_rewards() public { + // rewards are computed based on balance and staking period + uint256 unclaimed_rewards = _compute_and_update_rewards(msg.sender); + // transfer rewards (in ETH, for example) to the caller + payable(msg.sender).transfer(unclaimed_rewards); + } + + // Call claim_rewards before unstaking, otherwise you lose your rewards + function unstake() public { + _free_rewards(msg.sender); // clean up rewards-related variables + uint256 user_balance = balance[msg.sender]; + balance[msg.sender] = 0; + // transfer staked balance (in ETH, for example) to the caller + payable(msg.sender).transfer(user_balance); + } + + function _compute_and_update_rewards( + address user + ) internal view returns (uint256) { + // Implementation of reward computation + // Example: return some computed value based on user's balance and staking period + uint256 rewards = balance[user] / 10; // Simplified example + // Update any necessary state here + return rewards; + } + + function _free_rewards(address user) internal { + // Implementation of cleanup logic for rewards + // Example: reset some state related to the user's rewards + } + + receive() external payable { + // Allow the contract to receive ETH + } +}