diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index fab9562d2..4151759f0 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -97,3 +97,4 @@ from .assembly.return_instead_of_leave import ReturnInsteadOfLeave from .operations.incorrect_exp import IncorrectOperatorExponentiation from .statements.tautological_compare import TautologicalCompare from .statements.return_bomb import ReturnBomb +from .functions.out_of_order_retryable import OutOfOrderRetryable diff --git a/slither/detectors/functions/out_of_order_retryable.py b/slither/detectors/functions/out_of_order_retryable.py new file mode 100644 index 000000000..db9096f95 --- /dev/null +++ b/slither/detectors/functions/out_of_order_retryable.py @@ -0,0 +1,155 @@ +from typing import List + +from slither.core.cfg.node import Node +from slither.core.declarations import Function, FunctionContract +from slither.slithir.operations import HighLevelCall +from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.utils.output import Output + + +class OutOfOrderRetryable(AbstractDetector): + + ARGUMENT = "out-of-order-retryable" + HELP = "Out-of-order retryable transactions" + IMPACT = DetectorClassification.MEDIUM + CONFIDENCE = DetectorClassification.MEDIUM + + WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#out-of-order-retryable-transactions" + + WIKI_TITLE = "Out-of-order retryable transactions" + WIKI_DESCRIPTION = "Out-of-order retryable transactions" + + # region wiki_exploit_scenario + WIKI_EXPLOIT_SCENARIO = """ +```solidity +contract L1 { + 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.encodeCall(l2contract.claim_rewards, ()) + }); + // Retryable B + IInbox(inbox).createRetryableTicket({ + to: l2contract, + l2CallValue: 0, + maxSubmissionCost: maxSubmissionCost, + excessFeeRefundAddress: msg.sender, + callValueRefundAddress: msg.sender, + gasLimit: gas, + maxFeePerGas: maxFeePerGas, + data: abi.encodeCall(l2contract.unstake, ()) + }); + } +} + +contract L2 { + function claim_rewards() public { + // rewards is computed based on balance and staking period + uint unclaimed_rewards = _compute_and_update_rewards(); + token.safeTransfer(msg.sender, unclaimed_rewards); + } + + // Call claim_rewards before unstaking, otherwise you lose your rewards + function unstake() public { + _free_rewards(); // clean up rewards related variables + balance = balance[msg.sender]; + balance[msg.sender] = 0; + staked_token.safeTransfer(msg.sender, balance); + } +} +``` +Bob calls `doStuffOnL2` but the first retryable ticket calling `claim_rewards` fails. The second retryable ticket calling `unstake` is executed successfully. As a result, Bob loses his rewards.""" + # endregion wiki_exploit_scenario + + WIKI_RECOMMENDATION = "Do not rely on the order or successful execution of retryable tickets." + + key = "OUTOFORDERRETRYABLE" + + # pylint: disable=too-many-branches + def _detect_multiple_tickets( + self, function: FunctionContract, node: Node, visited: List[Node] + ) -> None: + if node in visited: + return + + visited = visited + [node] + + fathers_context = [] + + for father in node.fathers: + if self.key in father.context: + fathers_context += father.context[self.key] + + # Exclude path that dont bring further information + if node in self.visited_all_paths: + if all(f_c in self.visited_all_paths[node] for f_c in fathers_context): + return + else: + self.visited_all_paths[node] = [] + + self.visited_all_paths[node] = self.visited_all_paths[node] + fathers_context + + if self.key not in node.context: + node.context[self.key] = fathers_context + + # include ops from internal function calls + internal_ops = [] + for internal_call in node.internal_calls: + if isinstance(internal_call, Function): + internal_ops += internal_call.all_slithir_operations() + + # analyze node for retryable tickets + for ir in node.irs + internal_ops: + if ( + isinstance(ir, HighLevelCall) + and isinstance(ir.function, Function) + and ir.function.name + in [ + "createRetryableTicket", + "outboundTransferCustomRefund", + "unsafeCreateRetryableTicket", + ] + ): + node.context[self.key].append(node) + + if len(node.context[self.key]) > 1: + self.results.append(node.context[self.key]) + return + + for son in node.sons: + self._detect_multiple_tickets(function, son, visited) + + def _detect(self) -> List[Output]: + results = [] + + # pylint: disable=attribute-defined-outside-init + self.results = [] + self.visited_all_paths = {} + + for contract in self.compilation_unit.contracts: + for function in contract.functions: + if ( + function.is_implemented + and function.contract_declarer == contract + and function.entry_point + ): + function.entry_point.context[self.key] = [] + self._detect_multiple_tickets(function, function.entry_point, []) + + for multiple_tickets in self.results: + info = ["Multiple retryable tickets created in the same function:\n"] + + for x in multiple_tickets: + info += ["\t -", x, "\n"] + + json = self.generate_result(info) + results.append(json) + + return results diff --git a/tests/e2e/detectors/snapshots/detectors__detector_OutOfOrderRetryable_0_8_20_out_of_order_retryable_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_OutOfOrderRetryable_0_8_20_out_of_order_retryable_sol__0.txt new file mode 100644 index 000000000..4b0371a8c --- /dev/null +++ b/tests/e2e/detectors/snapshots/detectors__detector_OutOfOrderRetryable_0_8_20_out_of_order_retryable_sol__0.txt @@ -0,0 +1,16 @@ +Multiple retryable tickets created in the same function: + -Y(msg.sender).createRetryableTicket(address(1),0,0,address(0),address(0),0,0,) (tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol#62-70) + -Y(msg.sender).createRetryableTicket(address(2),0,0,address(0),address(0),0,0,) (tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol#72-80) + +Multiple retryable tickets created in the same function: + -good2() (tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol#95) + -good2() (tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol#96) + +Multiple retryable tickets created in the same function: + -Y(msg.sender).createRetryableTicket(address(1),0,0,address(0),address(0),0,0,) (tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol#40-48) + -Y(msg.sender).createRetryableTicket(address(2),0,0,address(0),address(0),0,0,) (tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol#50-58) + +Multiple retryable tickets created in the same function: + -Y(msg.sender).createRetryableTicket(address(1),0,0,address(0),address(0),0,0,) (tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol#83-91) + -good2() (tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol#92) + diff --git a/tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol b/tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol new file mode 100644 index 000000000..e3f8feb2e --- /dev/null +++ b/tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol @@ -0,0 +1,109 @@ +interface Y { + function createRetryableTicket( + address to, + uint256 l2CallValue, + uint256 maxSubmissionCost, + address excessFeeRefundAddress, + address callValueRefundAddress, + uint256 gasLimit, + uint256 maxFeePerGas, + bytes calldata data + ) external payable returns (uint256); +} + +contract X { +function good() external { + if (true) { + Y(msg.sender).createRetryableTicket( + address(1), + 0, + 0, + address(0), + address(0), + 0, + 0, + ""); + } else { + Y(msg.sender).createRetryableTicket( + address(2), + 0, + 0, + address(0), + address(0), + 0, + 0, + ""); + } +} +function bad1() external { + if (true) { + Y(msg.sender).createRetryableTicket( + address(1), + 0, + 0, + address(0), + address(0), + 0, + 0, + ""); + } + Y(msg.sender).createRetryableTicket( + address(2), + 0, + 0, + address(0), + address(0), + 0, + 0, + ""); + +} +function bad2() external { + Y(msg.sender).createRetryableTicket( + address(1), + 0, + 0, + address(0), + address(0), + 0, + 0, + ""); + + Y(msg.sender).createRetryableTicket( + address(2), + 0, + 0, + address(0), + address(0), + 0, + 0, + ""); +} +function bad3() external { + Y(msg.sender).createRetryableTicket( + address(1), + 0, + 0, + address(0), + address(0), + 0, + 0, + ""); + good2(); +} +function bad4() external { + good2(); + good2(); +} +function good2() internal { + Y(msg.sender).createRetryableTicket( + address(2), + 0, + 0, + address(0), + address(0), + 0, + 0, + ""); +} +} \ No newline at end of file diff --git a/tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol-0.8.20.zip b/tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol-0.8.20.zip new file mode 100644 index 000000000..fd52cf180 Binary files /dev/null and b/tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol-0.8.20.zip differ diff --git a/tests/e2e/detectors/test_detectors.py b/tests/e2e/detectors/test_detectors.py index 28dcc5e75..611db0292 100644 --- a/tests/e2e/detectors/test_detectors.py +++ b/tests/e2e/detectors/test_detectors.py @@ -1679,6 +1679,11 @@ ALL_TESTS = [ "return_bomb.sol", "0.8.20", ), + Test( + all_detectors.OutOfOrderRetryable, + "out_of_order_retryable.sol", + "0.8.20", + ), ] GENERIC_PATH = "/GENERIC_PATH"