diff --git a/aderyn_core/src/detect/high/arbitrary_transfer_from.rs b/aderyn_core/src/detect/high/arbitrary_transfer_from.rs index 2588e59c..8782bbab 100644 --- a/aderyn_core/src/detect/high/arbitrary_transfer_from.rs +++ b/aderyn_core/src/detect/high/arbitrary_transfer_from.rs @@ -1,11 +1,13 @@ use std::{collections::BTreeMap, error::Error}; -use crate::ast::{Expression, FunctionCall, NodeID, TypeName}; - use crate::{ + ast::{Expression, Identifier, NodeID}, capture, - context::workspace_context::WorkspaceContext, - detect::detector::{IssueDetector, IssueDetectorNamePool, IssueSeverity}, + context::{browser::ExtractFunctionCalls, workspace_context::WorkspaceContext}, + detect::{ + detector::{IssueDetector, IssueDetectorNamePool, IssueSeverity}, + helpers::{get_implemented_external_and_public_functions, has_msg_sender_binary_operation}, + }, }; use eyre::Result; @@ -16,51 +18,69 @@ pub struct ArbitraryTransferFromDetector { found_instances: BTreeMap<(String, usize, String), NodeID>, } -// Check if the first argument of the function call is valid -// In function calls with 3 args, the first arg [0] is the `from` address -// In function calls with 4 args, the second arg [1] is the `from` address -fn check_argument_validity(function_call: &FunctionCall) -> bool { - let arg_index = if function_call.arguments.len() == 3 { - 0 - } else if function_call.arguments.len() == 4 { - 1 - } else { - return false; - }; - - match &function_call.arguments[arg_index] { - Expression::MemberAccess(arg_member_access) => { - !(arg_member_access.member_name == "sender" - && matches!(&*arg_member_access.expression, Expression::Identifier(identifier) if identifier.name == "msg")) - } - Expression::FunctionCall(arg_function_call) => { - !(matches!(&*arg_function_call.expression, Expression::ElementaryTypeNameExpression(arg_el_type_name_exp) if matches!(&arg_el_type_name_exp.type_name, TypeName::ElementaryTypeName(type_name) if type_name.name == "address")) - && matches!(arg_function_call.arguments.first(), Some(Expression::Identifier(arg_identifier)) if arg_identifier.name == "this")) - } - _ => true, - } -} - impl IssueDetector for ArbitraryTransferFromDetector { fn detect(&mut self, context: &WorkspaceContext) -> Result> { - let transfer_from_function_calls = - context.function_calls().into_iter().filter(|&function_call| { - // For each function call, check if the function call is a member access - // and if the member name is "transferFrom" or "safeTransferFrom", then check if the - // first argument is valid If the first argument is valid, add the - // function call to found_instances - if let Expression::MemberAccess(member_access) = &*function_call.expression { - if member_access.member_name == "transferFrom" - || member_access.member_name == "safeTransferFrom" - { - return check_argument_validity(function_call); - } - } - false + // Applying devtooligan's suggestion + // * Operate on public and external functions only + // * See that msg.sender is not checked + // * Check that the argument passed in is from the parameter list of the said function + + let suspected_functions = + get_implemented_external_and_public_functions(context).filter(|function_definition| { + !has_msg_sender_binary_operation(&((*function_definition).into())) + && function_definition.modifiers.is_empty() // If there are modifiers, assume + // the function is safe because + // sometime modifiers' definition + // may not be in scope }); - for item in transfer_from_function_calls { - capture!(self, context, item); + for func in suspected_functions { + let func_parameters_ids = + &func.parameters.parameters.iter().map(|f| f.id).collect::>(); + + let transfer_func_calls = ExtractFunctionCalls::from(func) + .extracted + .into_iter() + .filter(|function_call| { + // For each function call, check if the function call is a member access + // and if the member name is "transferFrom" or "safeTransferFrom", then check if + // the first argument is valid If the first argument is + // valid, add the function call to found_instances + if let Expression::MemberAccess(member_access) = &*function_call.expression { + if member_access.member_name == "transferFrom" + || member_access.member_name == "safeTransferFrom" + { + return true; + } + } + false + }) + .collect::>(); + + for func in transfer_func_calls { + // Check if the first argument of the function call is valid + // In function calls with 3 args, the first arg [0] is the `from` address + // In function calls with 4 args, the second arg [1] is the `from` address + let arg_index = if func.arguments.len() == 3 { + 0 + } else if func.arguments.len() == 4 { + 1 + } else { + continue; + }; + + let arg = &func.arguments[arg_index]; + + if let Expression::Identifier(Identifier { + referenced_declaration: Some(referenced_id), + .. + }) = arg + { + if func_parameters_ids.iter().any(|r| r == referenced_id) { + capture!(self, context, func); + } + } + } } Ok(!self.found_instances.is_empty()) @@ -107,7 +127,7 @@ mod arbitrary_transfer_from_tests { // assert that the detector found an issue assert!(found); // assert that the detector found the correct number of instances - assert_eq!(detector.instances().len(), 4); + assert_eq!(detector.instances().len(), 2); // assert the severity is high assert_eq!(detector.severity(), crate::detect::detector::IssueSeverity::High); // assert the title is correct diff --git a/reports/report.json b/reports/report.json index e2a5bc25..a3db849d 100644 --- a/reports/report.json +++ b/reports/report.json @@ -491,29 +491,17 @@ "description": "Passing an arbitrary `from` address to `transferFrom` (or `safeTransferFrom`) can lead to loss of funds, because anyone can transfer tokens from the `from` address if an approval is made. ", "detector_name": "arbitrary-transfer-from", "instances": [ - { - "contract_path": "src/ArbitraryTransferFrom.sol", - "line_no": 16, - "src": "370:38", - "src_char": "370:38" - }, - { - "contract_path": "src/ArbitraryTransferFrom.sol", - "line_no": 20, - "src": "496:42", - "src_char": "496:42" - }, { "contract_path": "src/ArbitraryTransferFrom.sol", "line_no": 24, - "src": "634:53", - "src_char": "634:53" + "src": "738:42", + "src_char": "738:42" }, { "contract_path": "src/ArbitraryTransferFrom.sol", - "line_no": 30, - "src": "864:44", - "src_char": "864:44" + "line_no": 29, + "src": "879:53", + "src_char": "879:53" }, { "contract_path": "src/DeprecatedOZFunctions.sol", @@ -2353,21 +2341,21 @@ "instances": [ { "contract_path": "src/ArbitraryTransferFrom.sol", - "line_no": 16, - "src": "370:20", - "src_char": "370:20" + "line_no": 19, + "src": "601:20", + "src_char": "601:20" }, { "contract_path": "src/ArbitraryTransferFrom.sol", - "line_no": 30, - "src": "864:20", - "src_char": "864:20" + "line_no": 34, + "src": "1046:20", + "src_char": "1046:20" }, { "contract_path": "src/ArbitraryTransferFrom.sol", - "line_no": 50, - "src": "1517:20", - "src_char": "1517:20" + "line_no": 54, + "src": "1699:20", + "src_char": "1699:20" }, { "contract_path": "src/ContractLocksEther.sol", @@ -2770,9 +2758,9 @@ }, { "contract_path": "src/ArbitraryTransferFrom.sol", - "line_no": 28, - "src": "772:5", - "src_char": "772:5" + "line_no": 32, + "src": "954:5", + "src_char": "954:5" }, { "contract_path": "src/AssemblyExample.sol", @@ -5682,9 +5670,9 @@ "instances": [ { "contract_path": "src/ArbitraryTransferFrom.sol", - "line_no": 15, - "src": "304:4", - "src_char": "304:4" + "line_no": 18, + "src": "535:4", + "src_char": "535:4" }, { "contract_path": "src/ContractLocksEther.sol", diff --git a/reports/report.md b/reports/report.md index 3a1567b6..3938a440 100644 --- a/reports/report.md +++ b/reports/report.md @@ -270,33 +270,21 @@ If all arguments are strings and or bytes, `bytes.concat()` should be used inste Passing an arbitrary `from` address to `transferFrom` (or `safeTransferFrom`) can lead to loss of funds, because anyone can transfer tokens from the `from` address if an approval is made. -
6 Found Instances - - -- Found in src/ArbitraryTransferFrom.sol [Line: 16](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L16) +
4 Found Instances - ```solidity - s_token.transferFrom(from, to, amount); - ``` -- Found in src/ArbitraryTransferFrom.sol [Line: 20](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L20) +- Found in src/ArbitraryTransferFrom.sol [Line: 24](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L24) ```solidity s_token.safeTransferFrom(from, to, amount); ``` -- Found in src/ArbitraryTransferFrom.sol [Line: 24](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L24) +- Found in src/ArbitraryTransferFrom.sol [Line: 29](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L29) ```solidity SafeERC20.safeTransferFrom(s_token, from, to, amount); ``` -- Found in src/ArbitraryTransferFrom.sol [Line: 30](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L30) - - ```solidity - s_token.transferFrom(from_msgsender, to, am); - ``` - - Found in src/DeprecatedOZFunctions.sol [Line: 17](../tests/contract-playground/src/DeprecatedOZFunctions.sol#L17) ```solidity @@ -2305,19 +2293,19 @@ ERC20 functions may not behave as expected. For example: return values are not a
19 Found Instances -- Found in src/ArbitraryTransferFrom.sol [Line: 16](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L16) +- Found in src/ArbitraryTransferFrom.sol [Line: 19](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L19) ```solidity s_token.transferFrom(from, to, amount); ``` -- Found in src/ArbitraryTransferFrom.sol [Line: 30](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L30) +- Found in src/ArbitraryTransferFrom.sol [Line: 34](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L34) ```solidity s_token.transferFrom(from_msgsender, to, am); ``` -- Found in src/ArbitraryTransferFrom.sol [Line: 50](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L50) +- Found in src/ArbitraryTransferFrom.sol [Line: 54](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L54) ```solidity s_token.transferFrom(msg.sender, to, amount); @@ -2734,7 +2722,7 @@ Instead of marking a function as `public`, consider marking it as `external` if function f4() public { ``` -- Found in src/ArbitraryTransferFrom.sol [Line: 28](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L28) +- Found in src/ArbitraryTransferFrom.sol [Line: 32](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L32) ```solidity function good1(address to, uint256 am) public { @@ -5727,7 +5715,7 @@ Functions that are not used. Consider removing them.
12 Found Instances -- Found in src/ArbitraryTransferFrom.sol [Line: 15](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L15) +- Found in src/ArbitraryTransferFrom.sol [Line: 18](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L18) ```solidity function bad1(address from, address to, uint256 amount) internal { diff --git a/reports/report.sarif b/reports/report.sarif index a58610af..41e8e77b 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -49,17 +49,6 @@ { "level": "warning", "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/ArbitraryTransferFrom.sol" - }, - "region": { - "byteLength": 38, - "byteOffset": 370 - } - } - }, { "physicalLocation": { "artifactLocation": { @@ -67,7 +56,7 @@ }, "region": { "byteLength": 42, - "byteOffset": 496 + "byteOffset": 738 } } }, @@ -78,18 +67,7 @@ }, "region": { "byteLength": 53, - "byteOffset": 634 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/ArbitraryTransferFrom.sol" - }, - "region": { - "byteLength": 44, - "byteOffset": 864 + "byteOffset": 879 } } }, @@ -3300,7 +3278,7 @@ }, "region": { "byteLength": 20, - "byteOffset": 370 + "byteOffset": 601 } } }, @@ -3311,7 +3289,7 @@ }, "region": { "byteLength": 20, - "byteOffset": 864 + "byteOffset": 1046 } } }, @@ -3322,7 +3300,7 @@ }, "region": { "byteLength": 20, - "byteOffset": 1517 + "byteOffset": 1699 } } }, @@ -4053,7 +4031,7 @@ }, "region": { "byteLength": 5, - "byteOffset": 772 + "byteOffset": 954 } } }, @@ -9292,7 +9270,7 @@ }, "region": { "byteLength": 4, - "byteOffset": 304 + "byteOffset": 535 } } }, diff --git a/reports/templegold-report.md b/reports/templegold-report.md index 21207582..b8377e2d 100644 --- a/reports/templegold-report.md +++ b/reports/templegold-report.md @@ -8,12 +8,11 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [Files Details](#files-details) - [Issue Summary](#issue-summary) - [High Issues](#high-issues) - - [H-1: Arbitrary `from` passed to `transferFrom` (or `safeTransferFrom`)](#h-1-arbitrary-from-passed-to-transferfrom-or-safetransferfrom) - - [H-2: Unsafe Casting](#h-2-unsafe-casting) - - [H-3: Contract Name Reused in Different Files](#h-3-contract-name-reused-in-different-files) - - [H-4: Weak Randomness](#h-4-weak-randomness) - - [H-5: Deletion from a nested mappping.](#h-5-deletion-from-a-nested-mappping) - - [H-6: Contract locks Ether without a withdraw function.](#h-6-contract-locks-ether-without-a-withdraw-function) + - [H-1: Unsafe Casting](#h-1-unsafe-casting) + - [H-2: Contract Name Reused in Different Files](#h-2-contract-name-reused-in-different-files) + - [H-3: Weak Randomness](#h-3-weak-randomness) + - [H-4: Deletion from a nested mappping.](#h-4-deletion-from-a-nested-mappping) + - [H-5: Contract locks Ether without a withdraw function.](#h-5-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) @@ -195,48 +194,13 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 6 | +| High | 5 | | Low | 28 | # High Issues -## H-1: Arbitrary `from` passed to `transferFrom` (or `safeTransferFrom`) - -Passing an arbitrary `from` address to `transferFrom` (or `safeTransferFrom`) can lead to loss of funds, because anyone can transfer tokens from the `from` address if an approval is made. - -
4 Found Instances - - -- Found in contracts/core/MultiOtcOffer.sol [Line: 306](../tests/2024-07-templegold/protocol/contracts/core/MultiOtcOffer.sol#L306) - - ```solidity - _userBuyToken.safeTransferFrom(_fundsOwner, msg.sender, buyTokenAmount); - ``` - -- Found in contracts/core/OtcOffer.sol [Line: 140](../tests/2024-07-templegold/protocol/contracts/core/OtcOffer.sol#L140) - - ```solidity - userBuyToken.safeTransferFrom(_fundsOwner, msg.sender, buyTokenAmount); - ``` - -- Found in contracts/v2/TreasuryReservesVault.sol [Line: 776](../tests/2024-07-templegold/protocol/contracts/v2/TreasuryReservesVault.sol#L776) - - ```solidity - token.safeTransferFrom(from, address(this), repayAmount); - ``` - -- Found in contracts/v2/templeLineOfCredit/TempleLineOfCredit.sol [Line: 717](../tests/2024-07-templegold/protocol/contracts/v2/templeLineOfCredit/TempleLineOfCredit.sol#L717) - - ```solidity - daiToken.safeTransferFrom(_fromAccount, address(this), _repayAmount); - ``` - -
- - - -## H-2: Unsafe Casting +## H-1: Unsafe Casting Downcasting int/uints in Solidity can be unsafe due to the potential for data loss and unintended behavior.When downcasting a larger integer type to a smaller one (e.g., uint256 to uint128), the value may exceed the range of the target type,leading to truncation and loss of significant digits. Use OpenZeppelin's SafeCast library to safely downcast integers. @@ -259,7 +223,7 @@ Downcasting int/uints in Solidity can be unsafe due to the potential for data lo -## H-3: Contract Name Reused in Different Files +## H-2: Contract Name Reused in Different Files When compiling contracts with certain development frameworks (for example: Truffle), having contracts with the same name across different files can lead to one being overwritten. @@ -294,7 +258,7 @@ When compiling contracts with certain development frameworks (for example: Truff -## H-4: Weak Randomness +## H-3: 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. @@ -311,7 +275,7 @@ The use of keccak256 hash functions on predictable values like block.timestamp, -## H-5: Deletion from a nested mappping. +## H-4: 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. @@ -328,7 +292,7 @@ A deletion in a structure containing a mapping will not delete the mapping. The -## H-6: Contract locks Ether without a withdraw function. +## H-5: 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. diff --git a/tests/contract-playground/src/ArbitraryTransferFrom.sol b/tests/contract-playground/src/ArbitraryTransferFrom.sol index 2f051755..39e2515f 100644 --- a/tests/contract-playground/src/ArbitraryTransferFrom.sol +++ b/tests/contract-playground/src/ArbitraryTransferFrom.sol @@ -12,19 +12,23 @@ contract ArbitraryTransferFrom { s_token = token; } + // This maybe bad but it's not safe to conclude because it's an internal function + // Parameters of this function could be vetted before calling. @devtooligan suggested to stick + // to public & etxernal functions for now function bad1(address from, address to, uint256 amount) internal { s_token.transferFrom(from, to, amount); } + // BAD function bad2(address from, address to, uint256 amount) external { s_token.safeTransferFrom(from, to, amount); - } - + } + + // BAD function bad3(address from, address to, uint256 amount) external { SafeERC20.safeTransferFrom(s_token, from, to, amount); } - // ArbitraryTransferFromDetector has a false positive here function good1(address to, uint256 am) public { address from_msgsender = msg.sender; s_token.transferFrom(from_msgsender, to, am); @@ -49,5 +53,5 @@ contract ArbitraryTransferFrom { function good6(address to, uint256 amount) external { s_token.transferFrom(msg.sender, to, amount); } +} -} \ No newline at end of file