diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index d45c5d23e..fa7152c9e 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(), ] } @@ -144,6 +145,7 @@ pub(crate) enum IssueDetectorNamePool { WeakRandomness, PreDeclaredLocalVariableUsage, DeleteNestedMapping, + ContractLocksEther, // 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, @@ -302,6 +304,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { Some(Box::::default()) } + IssueDetectorNamePool::ContractLocksEther => { + Some(Box::::default()) + } IssueDetectorNamePool::Undecided => None, } } diff --git a/aderyn_core/src/detect/helpers.rs b/aderyn_core/src/detect/helpers.rs index 97851e33d..de986d566 100644 --- a/aderyn_core/src/detect/helpers.rs +++ b/aderyn_core/src/detect/helpers.rs @@ -116,6 +116,9 @@ pub fn has_calls_that_sends_native_eth(ast_node: &ASTNode) -> bool { if let Expression::Literal(literal) = c { return literal.value.is_some(); } + if let Expression::Identifier(_) = c { + return true; + } false }); if !call_carries_value { @@ -189,9 +192,9 @@ pub fn has_binary_checks_on_some_address(ast_node: &ASTNode) -> bool { binary_operations.into_iter().any(|op| { [op.left_expression, op.right_expression].iter().any(|op| { op.as_ref().type_descriptions().is_some_and(|desc| { - desc.type_string - .as_ref() - .is_some_and(|type_string| type_string == "address") + desc.type_string.as_ref().is_some_and(|type_string| { + type_string == "address" || type_string == "address payable" + }) }) }) }) diff --git a/aderyn_core/src/detect/high/contract_locks_ether.rs b/aderyn_core/src/detect/high/contract_locks_ether.rs new file mode 100644 index 000000000..00c7618dc --- /dev/null +++ b/aderyn_core/src/detect/high/contract_locks_ether.rs @@ -0,0 +1,182 @@ +use std::collections::BTreeMap; + +use std::convert::identity; +use std::error::Error; + +use crate::ast::NodeID; + +use crate::capture; +use crate::detect::detector::IssueDetectorNamePool; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; + +use eyre::Result; + +#[derive(Default)] +pub struct ContractLocksEtherDetector { + // 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 ContractLocksEtherDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + for contract in context.contract_definitions() { + // If a contract can accept eth, but doesn't allow for withdrawal capture it! + if contract.can_accept_eth(context).is_some_and(identity) + && !contract + .allows_withdrawal_of_eth(context) + .is_some_and(identity) + { + capture!(self, context, contract); + } + } + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("Contract locks Ether without a withdraw function.") + } + + fn description(&self) -> String { + String::from( + "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." + ) + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + IssueDetectorNamePool::ContractLocksEther.to_string() + } +} + +/// Handles tasks related to contract level analysis for eth +mod contract_eth_helper { + use crate::{ + ast::{ASTNode, ContractDefinition, StateMutability, Visibility}, + context::{ + browser::ExtractFunctionDefinitions, investigator::*, + workspace_context::WorkspaceContext, + }, + detect::helpers, + }; + + impl ContractDefinition { + pub fn can_accept_eth(&self, context: &WorkspaceContext) -> Option { + let contracts = self.linearized_base_contracts.as_ref()?; + for contract_id in contracts { + let funcs = + ExtractFunctionDefinitions::from(context.nodes.get(contract_id)?).extracted; + let num_payable_funcs = funcs + .into_iter() + .filter(|f| f.implemented && *f.state_mutability() == StateMutability::Payable) + .count(); + if num_payable_funcs > 0 { + return Some(true); + } + } + Some(false) + } + + pub fn allows_withdrawal_of_eth(&self, context: &WorkspaceContext) -> Option { + /* + For all the contracts in the hirearchy try and see if there is exists a public/external function that + can be called which takes the execution flow in a path where there is possibility to send back eth away from + the contract using the low level `call{value: XXX}` or `transfer` or `send`. + */ + let contracts = self.linearized_base_contracts.as_ref()?; + for contract_id in contracts { + if let ASTNode::ContractDefinition(contract) = context.nodes.get(contract_id)? { + let funcs = contract + .function_definitions() + .into_iter() + .filter(|f| { + f.implemented + && (f.visibility == Visibility::Public + || f.visibility == Visibility::External) + }) + .map(|f| f.into()) + .collect::>(); + + let mut tracker = EthWithdrawalAllowerTracker::default(); + + let investigator = StandardInvestigator::new( + context, + funcs.iter().collect::>().as_slice(), + StandardInvestigationStyle::Downstream, + ) + .ok()?; + + investigator.investigate(context, &mut tracker).ok()?; + + if tracker.has_calls_that_sends_native_eth { + return Some(true); + } + } + } + // At this point we have successfully gone through all the contracts in the inheritance heirarchy + // but tracker has determined that none of them have have calls that sends native eth Even if they are by + // some chance, they are not reachable from external & public functions + Some(false) + } + } + + #[derive(Default)] + struct EthWithdrawalAllowerTracker { + has_calls_that_sends_native_eth: bool, + } + + impl StandardInvestigatorVisitor for EthWithdrawalAllowerTracker { + fn visit_any(&mut self, ast_node: &ASTNode) -> eyre::Result<()> { + if !self.has_calls_that_sends_native_eth + && helpers::has_calls_that_sends_native_eth(ast_node) + { + self.has_calls_that_sends_native_eth = true; + } + Ok(()) + } + } +} + +#[cfg(test)] +mod contract_locks_ether_detector_tests { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, high::contract_locks_ether::ContractLocksEtherDetector, + }; + + #[test] + #[serial] + fn test_contract_locks_ether() { + let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs( + "../tests/contract-playground/src/ContractLocksEther.sol", + ); + + let mut detector = ContractLocksEtherDetector::default(); + let found = detector.detect(&context).unwrap(); + + println!("{:#?}", detector.instances()); + + // 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/aderyn_core/src/detect/high/mod.rs b/aderyn_core/src/detect/high/mod.rs index 66c8902cc..047342ff4 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -1,6 +1,7 @@ pub(crate) mod arbitrary_transfer_from; pub(crate) mod avoid_abi_encode_packed; pub(crate) mod block_timestamp_deadline; +pub(crate) mod contract_locks_ether; pub(crate) mod dangerous_strict_equality_balance; pub(crate) mod dangerous_unary_operator; pub(crate) mod delegate_call_in_loop; @@ -35,6 +36,7 @@ pub(crate) mod yul_return; pub use arbitrary_transfer_from::ArbitraryTransferFromDetector; pub use avoid_abi_encode_packed::AvoidAbiEncodePackedDetector; pub use block_timestamp_deadline::BlockTimestampDeadlineDetector; +pub use contract_locks_ether::ContractLocksEtherDetector; pub use dangerous_strict_equality_balance::DangerousStrictEqualityOnBalanceDetector; pub use dangerous_unary_operator::DangerousUnaryOperatorDetector; pub use delegate_call_in_loop::DelegateCallInLoopDetector; diff --git a/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index 32906d660..e5a591225 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -194,6 +194,7 @@ "signed-storage-array", "weak-randomness", "pre-declared-local-variable-usage", - "delete-nested-mapping" + "delete-nested-mapping", + "contract-locks-ether" ] } \ No newline at end of file diff --git a/reports/report.json b/reports/report.json index 44d3abfce..1dc1f1452 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 74, - "total_sloc": 2007 + "total_source_units": 75, + "total_sloc": 2128 }, "files_details": { "files_details": [ @@ -37,6 +37,10 @@ "file_path": "src/ConstantsLiterals.sol", "n_sloc": 28 }, + { + "file_path": "src/ContractLocksEther.sol", + "n_sloc": 121 + }, { "file_path": "src/ContractWithTodo.sol", "n_sloc": 7 @@ -304,7 +308,7 @@ ] }, "issue_count": { - "high": 33, + "high": 34, "low": 25 }, "high_issues": { @@ -1517,6 +1521,30 @@ "src": "686:16", "src_char": "686:16" }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 94, + "src": "2981:11", + "src_char": "2981:11" + }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 131, + "src": "4205:11", + "src_char": "4205:11" + }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 167, + "src": "5373:11", + "src_char": "5373:11" + }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 194, + "src": "6342:11", + "src_char": "6342:11" + }, { "contract_path": "src/SendEtherNoChecks.sol", "line_no": 53, @@ -1817,6 +1845,43 @@ "src_char": "426:25" } ] + }, + { + "title": "Contract locks Ether without a withdraw function.", + "description": "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.", + "detector_name": "contract-locks-ether", + "instances": [ + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 5, + "src": "73:10", + "src_char": "73:10" + }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 31, + "src": "822:11", + "src_char": "822:11" + }, + { + "contract_path": "src/EmptyBlocks.sol", + "line_no": 20, + "src": "344:39", + "src_char": "344:39" + }, + { + "contract_path": "src/EmptyBlocks.sol", + "line_no": 44, + "src": "630:11", + "src_char": "630:11" + }, + { + "contract_path": "src/eth2/DepositContract.sol", + "line_no": 58, + "src": "4547:15", + "src_char": "3059:15" + } + ] } ] }, @@ -1975,6 +2040,12 @@ "src": "1517:20", "src_char": "1517:20" }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 162, + "src": "5185:18", + "src_char": "5185:18" + }, { "contract_path": "src/DeprecatedOZFunctions.sol", "line_no": 32, @@ -2036,6 +2107,12 @@ "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/ContractWithTodo.sol", "line_no": 2, @@ -2212,6 +2289,42 @@ "src": "113:1", "src_char": "113:1" }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 20, + "src": "539:10", + "src_char": "539:10" + }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 49, + "src": "1379:10", + "src_char": "1379:10" + }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 81, + "src": "2414:10", + "src_char": "2414:10" + }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 118, + "src": "3653:10", + "src_char": "3653:10" + }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 155, + "src": "4877:10", + "src_char": "4877:10" + }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 181, + "src": "5775:10", + "src_char": "5775:10" + }, { "contract_path": "src/ContractWithTodo.sol", "line_no": 13, @@ -2580,6 +2693,72 @@ "description": "Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.", "detector_name": "unindexed-events", "instances": [ + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 7, + "src": "119:56", + "src_char": "119:56" + }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 33, + "src": "869:56", + "src_char": "869:56" + }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 36, + "src": "961:54", + "src_char": "961:54" + }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 65, + "src": "1904:56", + "src_char": "1904:56" + }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 68, + "src": "1996:54", + "src_char": "1996:54" + }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 102, + "src": "3143:56", + "src_char": "3143:56" + }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 105, + "src": "3235:54", + "src_char": "3235:54" + }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 139, + "src": "4367:56", + "src_char": "4367:56" + }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 142, + "src": "4459:54", + "src_char": "4459:54" + }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 175, + "src": "5563:56", + "src_char": "5563:56" + }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 178, + "src": "5655:54", + "src_char": "5655:54" + }, { "contract_path": "src/TestERC20.sol", "line_no": 14, @@ -2788,6 +2967,12 @@ "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/ContractLocksEther.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/ContractWithTodo.sol", "line_no": 2, @@ -3779,6 +3964,7 @@ "public-variable-read-in-external-context", "weak-randomness", "pre-declared-local-variable-usage", - "delete-nested-mapping" + "delete-nested-mapping", + "contract-locks-ether" ] } \ No newline at end of file diff --git a/reports/report.md b/reports/report.md index 939710410..11584a82e 100644 --- a/reports/report.md +++ b/reports/report.md @@ -41,6 +41,7 @@ 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: Contract locks Ether without a withdraw function.](#h-34-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) @@ -75,8 +76,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 74 | -| Total nSLOC | 2007 | +| .sol Files | 75 | +| Total nSLOC | 2128 | ## Files Details @@ -91,6 +92,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/Casting.sol | 126 | | src/CompilerBugStorageSignedIntegerArray.sol | 13 | | src/ConstantsLiterals.sol | 28 | +| src/ContractLocksEther.sol | 121 | | src/ContractWithTodo.sol | 7 | | src/Counter.sol | 20 | | src/CrazyPragma.sol | 4 | @@ -157,14 +159,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** | **2007** | +| **Total** | **2128** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 33 | +| High | 34 | | Low | 25 | @@ -1457,7 +1459,7 @@ The patterns `if (… || true)` and `if (.. && false)` will always evaluate to t Introduce checks for `msg.sender` in the function -
9 Found Instances +
13 Found Instances - Found in src/CallGraphTests.sol [Line: 38](../tests/contract-playground/src/CallGraphTests.sol#L38) @@ -1466,6 +1468,30 @@ Introduce checks for `msg.sender` in the function function enterTenthFloor2(address x) external passThroughNinthFloor2(x) { ``` +- Found in src/ContractLocksEther.sol [Line: 94](../tests/contract-playground/src/ContractLocksEther.sol#L94) + + ```solidity + function takeEthBack(uint256 amount) external { + ``` + +- Found in src/ContractLocksEther.sol [Line: 131](../tests/contract-playground/src/ContractLocksEther.sol#L131) + + ```solidity + function takeEthBack(uint256 amount) external { + ``` + +- Found in src/ContractLocksEther.sol [Line: 167](../tests/contract-playground/src/ContractLocksEther.sol#L167) + + ```solidity + function takeEthBack(uint256 amount) external { + ``` + +- Found in src/ContractLocksEther.sol [Line: 194](../tests/contract-playground/src/ContractLocksEther.sol#L194) + + ```solidity + function takeEthBack(uint256 amount) external { + ``` + - Found in src/SendEtherNoChecks.sol [Line: 53](../tests/contract-playground/src/SendEtherNoChecks.sol#L53) ```solidity @@ -1813,6 +1839,47 @@ A deletion in a structure containing a mapping will not delete the mapping. The +## H-34: 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. + +
5 Found Instances + + +- Found in src/ContractLocksEther.sol [Line: 5](../tests/contract-playground/src/ContractLocksEther.sol#L5) + + ```solidity + contract NoWithdraw { + ``` + +- Found in src/ContractLocksEther.sol [Line: 31](../tests/contract-playground/src/ContractLocksEther.sol#L31) + + ```solidity + contract NoWithdraw2 { + ``` + +- Found in src/EmptyBlocks.sol [Line: 20](../tests/contract-playground/src/EmptyBlocks.sol#L20) + + ```solidity + contract EmptyBlocksNestedInReceiverAndFallbacks { + ``` + +- Found in src/EmptyBlocks.sol [Line: 44](../tests/contract-playground/src/EmptyBlocks.sol#L44) + + ```solidity + contract EmptyBlocks { + ``` + +- Found in src/eth2/DepositContract.sol [Line: 58](../tests/contract-playground/src/eth2/DepositContract.sol#L58) + + ```solidity + contract DepositContract is IDepositContract, ERC165 { + ``` + +
+ + + # Low Issues ## L-1: Centralization Risk for trusted owners @@ -1968,7 +2035,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. -
11 Found Instances +
12 Found Instances - Found in src/ArbitraryTransferFrom.sol [Line: 16](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L16) @@ -1989,6 +2056,12 @@ ERC20 functions may not behave as expected. For example: return values are not a s_token.transferFrom(msg.sender, to, amount); ``` +- Found in src/ContractLocksEther.sol [Line: 162](../tests/contract-playground/src/ContractLocksEther.sol#L162) + + ```solidity + recipient.transfer(amount); + ``` + - Found in src/DeprecatedOZFunctions.sol [Line: 32](../tests/contract-playground/src/DeprecatedOZFunctions.sol#L32) ```solidity @@ -2045,7 +2118,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;` -
20 Found Instances +
21 Found Instances - Found in src/CompilerBugStorageSignedIntegerArray.sol [Line: 2](../tests/contract-playground/src/CompilerBugStorageSignedIntegerArray.sol#L2) @@ -2054,6 +2127,12 @@ Consider using a specific version of Solidity in your contracts instead of a wid pragma solidity ^0.4.0; ``` +- Found in src/ContractLocksEther.sol [Line: 2](../tests/contract-playground/src/ContractLocksEther.sol#L2) + + ```solidity + pragma solidity ^0.8.0; + ``` + - Found in src/ContractWithTodo.sol [Line: 2](../tests/contract-playground/src/ContractWithTodo.sol#L2) ```solidity @@ -2223,7 +2302,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. -
23 Found Instances +
29 Found Instances - Found in src/ArbitraryTransferFrom.sol [Line: 28](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L28) @@ -2238,6 +2317,42 @@ Instead of marking a function as `public`, consider marking it as `external` if function f(uint x) public view returns (uint r) { ``` +- Found in src/ContractLocksEther.sol [Line: 20](../tests/contract-playground/src/ContractLocksEther.sol#L20) + + ```solidity + function getBalance() public view returns (uint256) { + ``` + +- Found in src/ContractLocksEther.sol [Line: 49](../tests/contract-playground/src/ContractLocksEther.sol#L49) + + ```solidity + function getBalance() public view returns (uint256) { + ``` + +- Found in src/ContractLocksEther.sol [Line: 81](../tests/contract-playground/src/ContractLocksEther.sol#L81) + + ```solidity + function getBalance() public view returns (uint256) { + ``` + +- Found in src/ContractLocksEther.sol [Line: 118](../tests/contract-playground/src/ContractLocksEther.sol#L118) + + ```solidity + function getBalance() public view returns (uint256) { + ``` + +- Found in src/ContractLocksEther.sol [Line: 155](../tests/contract-playground/src/ContractLocksEther.sol#L155) + + ```solidity + function getBalance() public view returns (uint256) { + ``` + +- Found in src/ContractLocksEther.sol [Line: 181](../tests/contract-playground/src/ContractLocksEther.sol#L181) + + ```solidity + function getBalance() public view returns (uint256) { + ``` + - Found in src/ContractWithTodo.sol [Line: 13](../tests/contract-playground/src/ContractWithTodo.sol#L13) ```solidity @@ -2599,8 +2714,74 @@ If the same constant literal value is used multiple times, create a constant sta Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed. -
7 Found Instances +
18 Found Instances + + +- Found in src/ContractLocksEther.sol [Line: 7](../tests/contract-playground/src/ContractLocksEther.sol#L7) + + ```solidity + event Deposited(address indexed sender, uint256 amount); + ``` + +- Found in src/ContractLocksEther.sol [Line: 33](../tests/contract-playground/src/ContractLocksEther.sol#L33) + + ```solidity + event Deposited(address indexed sender, uint256 amount); + ``` + +- Found in src/ContractLocksEther.sol [Line: 36](../tests/contract-playground/src/ContractLocksEther.sol#L36) + + ```solidity + event Transferred(address indexed to, uint256 amount); + ``` + +- Found in src/ContractLocksEther.sol [Line: 65](../tests/contract-playground/src/ContractLocksEther.sol#L65) + + ```solidity + event Deposited(address indexed sender, uint256 amount); + ``` +- Found in src/ContractLocksEther.sol [Line: 68](../tests/contract-playground/src/ContractLocksEther.sol#L68) + + ```solidity + event Transferred(address indexed to, uint256 amount); + ``` + +- Found in src/ContractLocksEther.sol [Line: 102](../tests/contract-playground/src/ContractLocksEther.sol#L102) + + ```solidity + event Deposited(address indexed sender, uint256 amount); + ``` + +- Found in src/ContractLocksEther.sol [Line: 105](../tests/contract-playground/src/ContractLocksEther.sol#L105) + + ```solidity + event Transferred(address indexed to, uint256 amount); + ``` + +- Found in src/ContractLocksEther.sol [Line: 139](../tests/contract-playground/src/ContractLocksEther.sol#L139) + + ```solidity + event Deposited(address indexed sender, uint256 amount); + ``` + +- Found in src/ContractLocksEther.sol [Line: 142](../tests/contract-playground/src/ContractLocksEther.sol#L142) + + ```solidity + event Transferred(address indexed to, uint256 amount); + ``` + +- Found in src/ContractLocksEther.sol [Line: 175](../tests/contract-playground/src/ContractLocksEther.sol#L175) + + ```solidity + event Deposited(address indexed sender, uint256 amount); + ``` + +- Found in src/ContractLocksEther.sol [Line: 178](../tests/contract-playground/src/ContractLocksEther.sol#L178) + + ```solidity + event Transferred(address indexed to, uint256 amount); + ``` - Found in src/TestERC20.sol [Line: 14](../tests/contract-playground/src/TestERC20.sol#L14) @@ -2817,7 +2998,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. -
28 Found Instances +
29 Found Instances - Found in src/AdminContract.sol [Line: 2](../tests/contract-playground/src/AdminContract.sol#L2) @@ -2826,6 +3007,12 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai pragma solidity 0.8.20; ``` +- Found in src/ContractLocksEther.sol [Line: 2](../tests/contract-playground/src/ContractLocksEther.sol#L2) + + ```solidity + pragma solidity ^0.8.0; + ``` + - Found in src/ContractWithTodo.sol [Line: 2](../tests/contract-playground/src/ContractWithTodo.sol#L2) ```solidity diff --git a/reports/report.sarif b/reports/report.sarif index c3891be16..2ef90121c 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -2132,6 +2132,50 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 11, + "byteOffset": 2981 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 11, + "byteOffset": 4205 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 11, + "byteOffset": 5373 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 11, + "byteOffset": 6342 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -2644,6 +2688,70 @@ }, "ruleId": "delete-nested-mapping" }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 10, + "byteOffset": 73 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 11, + "byteOffset": 822 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/EmptyBlocks.sol" + }, + "region": { + "byteLength": 39, + "byteOffset": 344 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/EmptyBlocks.sol" + }, + "region": { + "byteLength": 11, + "byteOffset": 630 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/eth2/DepositContract.sol" + }, + "region": { + "byteLength": 15, + "byteOffset": 4547 + } + } + } + ], + "message": { + "text": "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." + }, + "ruleId": "contract-locks-ether" + }, { "level": "note", "locations": [ @@ -2903,6 +3011,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 18, + "byteOffset": 5185 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3011,6 +3130,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3326,6 +3456,72 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 10, + "byteOffset": 539 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 10, + "byteOffset": 1379 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 10, + "byteOffset": 2414 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 10, + "byteOffset": 3653 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 10, + "byteOffset": 4877 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 10, + "byteOffset": 5775 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3993,6 +4189,127 @@ { "level": "note", "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 56, + "byteOffset": 119 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 56, + "byteOffset": 869 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 54, + "byteOffset": 961 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 56, + "byteOffset": 1904 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 54, + "byteOffset": 1996 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 56, + "byteOffset": 3143 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 54, + "byteOffset": 3235 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 56, + "byteOffset": 4367 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 54, + "byteOffset": 4459 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 56, + "byteOffset": 5563 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 54, + "byteOffset": 5655 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -4359,6 +4676,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ContractLocksEther.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { diff --git a/reports/templegold-report.md b/reports/templegold-report.md index e6f22785c..278751712 100644 --- a/reports/templegold-report.md +++ b/reports/templegold-report.md @@ -13,9 +13,11 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [H-3: Unsafe Casting](#h-3-unsafe-casting) - [H-4: Contract Name Reused in Different Files](#h-4-contract-name-reused-in-different-files) - [H-5: Uninitialized State Variables](#h-5-uninitialized-state-variables) - - [H-6: Return value of the function call is not checked.](#h-6-return-value-of-the-function-call-is-not-checked) - - [H-7: Weak Randomness](#h-7-weak-randomness) - - [H-8: Deletion from a nested mappping.](#h-8-deletion-from-a-nested-mappping) + - [H-6: Sending native Eth is not protected from these functions.](#h-6-sending-native-eth-is-not-protected-from-these-functions) + - [H-7: Return value of the function call is not checked.](#h-7-return-value-of-the-function-call-is-not-checked) + - [H-8: Weak Randomness](#h-8-weak-randomness) + - [H-9: Deletion from a nested mappping.](#h-9-deletion-from-a-nested-mappping) + - [H-10: Contract locks Ether without a withdraw function.](#h-10-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: `ecrecover` is susceptible to signature malleability](#l-2-ecrecover-is-susceptible-to-signature-malleability) @@ -188,7 +190,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 8 | +| High | 10 | | Low | 19 | @@ -399,7 +401,36 @@ Solidity does initialize variables by default when you declare them, however it' -## H-6: Return value of the function call is not checked. +## H-6: Sending native Eth is not protected from these functions. + +Introduce checks for `msg.sender` in the function + +
3 Found Instances + + +- Found in contracts/amm/TempleStableAMMRouter.sol [Line: 226](../tests/2024-07-templegold/protocol/contracts/amm/TempleStableAMMRouter.sol#L226) + + ```solidity + function withdraw(address token, address to, uint256 amount) external onlyOwner { + ``` + +- Found in contracts/core/VaultProxy.sol [Line: 125](../tests/2024-07-templegold/protocol/contracts/core/VaultProxy.sol#L125) + + ```solidity + function withdraw(address token, address to, uint256 amount) external onlyOwner { + ``` + +- Found in contracts/core/VaultedTemple.sol [Line: 52](../tests/2024-07-templegold/protocol/contracts/core/VaultedTemple.sol#L52) + + ```solidity + function withdraw(address token, address to, uint256 amount) external onlyOwner { + ``` + +
+ + + +## H-7: Return value of the function call is not checked. Function returns a value but it is ignored. @@ -488,7 +519,7 @@ Function returns a value but it is ignored. -## H-7: Weak Randomness +## H-8: Weak Randomness The use of keccak256 hash functions on predictable values like block.timestamp, block.number, or similar data, including modulo operations on these values, should be avoided for generating randomness, as they are easily predictable and manipulable. The `PREVRANDAO` opcode also should not be used as a source of randomness. Instead, utilize Chainlink VRF for cryptographically secure and provably random values to ensure protocol integrity. @@ -505,7 +536,7 @@ The use of keccak256 hash functions on predictable values like block.timestamp, -## H-8: Deletion from a nested mappping. +## H-9: Deletion from a nested mappping. A deletion in a structure containing a mapping will not delete the mapping. The remaining data may be used to compromise the contract. @@ -522,6 +553,41 @@ A deletion in a structure containing a mapping will not delete the mapping. The +## H-10: 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. + +
4 Found Instances + + +- Found in contracts/fakes/UniswapV2Router02NoEth.sol [Line: 21](../tests/2024-07-templegold/protocol/contracts/fakes/UniswapV2Router02NoEth.sol#L21) + + ```solidity + contract UniswapV2Router02NoEth is IUniswapV2Router02 { + ``` + +- Found in contracts/fakes/templegold/TempleGoldMock.sol [Line: 27](../tests/2024-07-templegold/protocol/contracts/fakes/templegold/TempleGoldMock.sol#L27) + + ```solidity + contract TempleGoldMock is OFT { + ``` + +- Found in contracts/templegold/TempleGold.sol [Line: 29](../tests/2024-07-templegold/protocol/contracts/templegold/TempleGold.sol#L29) + + ```solidity + contract TempleGold is ITempleGold, OFT { + ``` + +- Found in contracts/templegold/TempleTeleporter.sol [Line: 19](../tests/2024-07-templegold/protocol/contracts/templegold/TempleTeleporter.sol#L19) + + ```solidity + contract TempleTeleporter is ITempleTeleporter, OApp { + ``` + +
+ + + # Low Issues ## L-1: Centralization Risk for trusted owners diff --git a/tests/contract-playground/src/ContractLocksEther.sol b/tests/contract-playground/src/ContractLocksEther.sol new file mode 100644 index 000000000..4e607cbdc --- /dev/null +++ b/tests/contract-playground/src/ContractLocksEther.sol @@ -0,0 +1,210 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +// BAD +contract NoWithdraw { + // Event to log deposits + event Deposited(address indexed sender, uint256 amount); + + // Public payable function to receive Ether + receive() external payable { + emit Deposited(msg.sender, msg.value); + } + + // Public payable fallback function to handle any data sent with Ether + fallback() external payable { + emit Deposited(msg.sender, msg.value); + } + + // Function to get the balance of the contract + function getBalance() public view returns (uint256) { + return address(this).balance; + } +} + +/* + Even though the `NoWithdraw2` has an internal function that can send eth away, it's not reachable + by any public / external function hence it is a bad contract. +*/ + +// BAD +contract NoWithdraw2 { + // Event to log deposits + event Deposited(address indexed sender, uint256 amount); + + // Event to log transfers + event Transferred(address indexed to, uint256 amount); + + // Public payable function to receive Ether + receive() external payable { + emit Deposited(msg.sender, msg.value); + } + + // Public payable fallback function to handle any data sent with Ether + fallback() external payable { + emit Deposited(msg.sender, msg.value); + } + + // Function to get the balance of the contract + function getBalance() public view returns (uint256) { + return address(this).balance; + } + + // Internal function to send Ether to a given address + function _sendEther(address payable recipient, uint256 amount) internal { + require(address(this).balance >= amount, "Insufficient balance"); + (bool success, ) = recipient.call{value: amount}(""); + require(success, "Transfer failed"); + emit Transferred(recipient, amount); + } +} + +// GOOD +contract CanWithdraw { + // Event to log deposits + event Deposited(address indexed sender, uint256 amount); + + // Event to log transfers + event Transferred(address indexed to, uint256 amount); + + // Public payable function to receive Ether + receive() external payable { + emit Deposited(msg.sender, msg.value); + } + + // Public payable fallback function to handle any data sent with Ether + fallback() external payable { + emit Deposited(msg.sender, msg.value); + } + + // Function to get the balance of the contract + function getBalance() public view returns (uint256) { + return address(this).balance; + } + + // Internal function to send Ether to a given address + function _sendEther(address payable recipient, uint256 amount) internal { + require(address(this).balance >= amount, "Insufficient balance"); + (bool success, ) = recipient.call{value: amount}(""); + require(success, "Transfer failed"); + emit Transferred(recipient, amount); + } + + // This function allows for the withdrawal of eth. Hence this contract is a GOOD contract. + function takeEthBack(uint256 amount) external { + _sendEther(payable(msg.sender), amount); + } +} + +// GOOD +contract CanWithdraw2 { + // Event to log deposits + event Deposited(address indexed sender, uint256 amount); + + // Event to log transfers + event Transferred(address indexed to, uint256 amount); + + // Public payable function to receive Ether + receive() external payable { + emit Deposited(msg.sender, msg.value); + } + + // Public payable fallback function to handle any data sent with Ether + fallback() external payable { + emit Deposited(msg.sender, msg.value); + } + + // Function to get the balance of the contract + function getBalance() public view returns (uint256) { + return address(this).balance; + } + + // Internal function to send Ether to a given address + function _sendEther(address payable recipient, uint256 amount) internal { + require(address(this).balance >= amount, "Insufficient balance"); + bool success = recipient.send(amount); + require(success, "Transfer failed"); + emit Transferred(recipient, amount); + } + + // This function allows for the withdrawal of eth. Hence this contract is a GOOD contract. + function takeEthBack(uint256 amount) external { + _sendEther(payable(msg.sender), amount); + } +} + +// GOOD +contract CanWithdraw3 { + // Event to log deposits + event Deposited(address indexed sender, uint256 amount); + + // Event to log transfers + event Transferred(address indexed to, uint256 amount); + + // Public payable function to receive Ether + receive() external payable { + emit Deposited(msg.sender, msg.value); + } + + // Public payable fallback function to handle any data sent with Ether + fallback() external payable { + emit Deposited(msg.sender, msg.value); + } + + // Function to get the balance of the contract + function getBalance() public view returns (uint256) { + return address(this).balance; + } + + // Internal function to send Ether to a given address + function _sendEther(address payable recipient, uint256 amount) internal { + require(address(this).balance >= amount, "Insufficient balance"); + recipient.transfer(amount); + emit Transferred(recipient, amount); + } + + // This function allows for the withdrawal of eth. Hence this contract is a GOOD contract. + function takeEthBack(uint256 amount) external { + _sendEther(payable(msg.sender), amount); + } +} + +// GOOD (no payable functions) +contract CanWithdrawParent { + // Event to log deposits + event Deposited(address indexed sender, uint256 amount); + + // Event to log transfers + event Transferred(address indexed to, uint256 amount); + + // Function to get the balance of the contract + function getBalance() public view returns (uint256) { + return address(this).balance; + } + + // Internal function to send Ether to a given address + function _sendEther(address payable recipient, uint256 amount) internal { + require(address(this).balance >= amount, "Insufficient balance"); + (bool success, ) = recipient.call{value: amount}(""); + require(success, "Transfer failed"); + emit Transferred(recipient, amount); + } + + // This function allows for the withdrawal of eth. Hence this contract is a GOOD contract. + function takeEthBack(uint256 amount) external { + _sendEther(payable(msg.sender), amount); + } +} + +// GOOD (It has payable functions, but we can withdraw (look at the parent's contract) +contract CanWithdrawChild is CanWithdrawParent { + // Public payable function to receive Ether + receive() external payable { + emit Deposited(msg.sender, msg.value); + } + + // Public payable fallback function to handle any data sent with Ether + fallback() external payable { + emit Deposited(msg.sender, msg.value); + } +}