diff --git a/aderyn_core/src/detect/high/mod.rs b/aderyn_core/src/detect/high/mod.rs index 6cfc6610..757cc413 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -1,6 +1,5 @@ pub(crate) mod arbitrary_transfer_from; pub(crate) mod avoid_abi_encode_packed; -pub(crate) mod block_timestamp_deadline; pub(crate) mod const_func_change_state; pub(crate) mod contract_locks_ether; pub(crate) mod dangerous_strict_equality_balance; @@ -42,7 +41,6 @@ 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 const_func_change_state::ConstantFunctionChangingStateDetector; pub use contract_locks_ether::ContractLocksEtherDetector; pub use dangerous_strict_equality_balance::DangerousStrictEqualityOnBalanceDetector; diff --git a/aderyn_core/src/detect/high/block_timestamp_deadline.rs b/aderyn_core/src/detect/low/block_timestamp_deadline.rs similarity index 98% rename from aderyn_core/src/detect/high/block_timestamp_deadline.rs rename to aderyn_core/src/detect/low/block_timestamp_deadline.rs index f824ae66..6fafbaa3 100644 --- a/aderyn_core/src/detect/high/block_timestamp_deadline.rs +++ b/aderyn_core/src/detect/low/block_timestamp_deadline.rs @@ -96,7 +96,7 @@ impl IssueDetector for BlockTimestampDeadlineDetector { } fn severity(&self) -> IssueSeverity { - IssueSeverity::High + IssueSeverity::Low } fn title(&self) -> String { @@ -121,7 +121,7 @@ impl IssueDetector for BlockTimestampDeadlineDetector { mod block_timestamp_deadline_detector_tests { use serial_test::serial; - use crate::detect::{detector::IssueDetector, high::BlockTimestampDeadlineDetector}; + use crate::detect::{detector::IssueDetector, low::BlockTimestampDeadlineDetector}; #[test] #[serial] @@ -137,7 +137,7 @@ mod block_timestamp_deadline_detector_tests { // assert that the number of instances found is correct assert_eq!(detector.instances().len(), 9); // assert that the severity is High - assert_eq!(detector.severity(), crate::detect::detector::IssueSeverity::High); + assert_eq!(detector.severity(), crate::detect::detector::IssueSeverity::Low); // assert that the title is correct assert_eq!( detector.title(), @@ -167,7 +167,7 @@ mod block_timestamp_deadline_detector_tests { // assert that the number of instances found is correct assert_eq!(detector.instances().len(), 8); // assert that the severity is High - assert_eq!(detector.severity(), crate::detect::detector::IssueSeverity::High); + assert_eq!(detector.severity(), crate::detect::detector::IssueSeverity::Low); // assert that the title is correct assert_eq!( detector.title(), diff --git a/aderyn_core/src/detect/low/mod.rs b/aderyn_core/src/detect/low/mod.rs index 3b470632..0304305a 100644 --- a/aderyn_core/src/detect/low/mod.rs +++ b/aderyn_core/src/detect/low/mod.rs @@ -1,4 +1,5 @@ pub(crate) mod assert_state_change; +pub(crate) mod block_timestamp_deadline; pub(crate) mod boolean_equality; pub(crate) mod builtin_symbol_shadowing; pub(crate) mod cache_array_length; @@ -46,6 +47,7 @@ pub(crate) mod void_constructor; pub(crate) mod zero_address_check; pub use assert_state_change::AssertStateChangeDetector; +pub use block_timestamp_deadline::BlockTimestampDeadlineDetector; pub use boolean_equality::BooleanEqualityDetector; pub use builtin_symbol_shadowing::BuiltinSymbolShadowDetector; pub use cache_array_length::CacheArrayLengthDetector; diff --git a/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index 7664c684..c38712b2 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -177,7 +177,6 @@ "detectors_used": [ "delegate-call-in-loop", "hash-collision-due-to-abi-encode-packed", - "block-timestamp-is-weak-deadline", "arbitrary-transfer-from", "unprotected-initializer", "unsafe-casting-detector", diff --git a/reports/report.json b/reports/report.json index f37b6376..cdc663f8 100644 --- a/reports/report.json +++ b/reports/report.json @@ -456,8 +456,8 @@ ] }, "issue_count": { - "high": 41, - "low": 46 + "high": 40, + "low": 47 }, "high_issues": { "issues": [ @@ -499,115 +499,6 @@ } ] }, - { - "title": "Using `block.timestamp` for swap deadline offers no protection", - "description": "In the PoS model, proposers know well in advance if they will propose one or consecutive blocks ahead of time. In such a scenario, a malicious validator can hold back the transaction and execute it at a more favourable block number.Consider allowing function caller to specify swap deadline input parameter.", - "detector_name": "block-timestamp-is-weak-deadline", - "instances": [ - { - "contract_path": "src/Trump.sol", - "line_no": 290, - "src": "9949:190", - "src_char": "9949:190" - }, - { - "contract_path": "src/uniswap/UniswapV2Swapper.sol", - "line_no": 23, - "src": "670:83", - "src_char": "670:83" - }, - { - "contract_path": "src/uniswap/UniswapV2Swapper.sol", - "line_no": 24, - "src": "763:83", - "src_char": "763:83" - }, - { - "contract_path": "src/uniswap/UniswapV2Swapper.sol", - "line_no": 25, - "src": "856:70", - "src_char": "856:70" - }, - { - "contract_path": "src/uniswap/UniswapV2Swapper.sol", - "line_no": 26, - "src": "936:80", - "src_char": "936:80" - }, - { - "contract_path": "src/uniswap/UniswapV2Swapper.sol", - "line_no": 27, - "src": "1026:80", - "src_char": "1026:80" - }, - { - "contract_path": "src/uniswap/UniswapV2Swapper.sol", - "line_no": 31, - "src": "1278:112", - "src_char": "1278:112" - }, - { - "contract_path": "src/uniswap/UniswapV2Swapper.sol", - "line_no": 32, - "src": "1400:99", - "src_char": "1400:99" - }, - { - "contract_path": "src/uniswap/UniswapV2Swapper.sol", - "line_no": 33, - "src": "1509:109", - "src_char": "1509:109" - }, - { - "contract_path": "src/uniswap/UniswapV3Swapper.sol", - "line_no": 52, - "src": "1115:143", - "src_char": "1115:143" - }, - { - "contract_path": "src/uniswap/UniswapV3Swapper.sol", - "line_no": 55, - "src": "1293:321", - "src_char": "1293:321" - }, - { - "contract_path": "src/uniswap/UniswapV3Swapper.sol", - "line_no": 66, - "src": "1668:131", - "src_char": "1668:131" - }, - { - "contract_path": "src/uniswap/UniswapV3Swapper.sol", - "line_no": 69, - "src": "1828:236", - "src_char": "1828:236" - }, - { - "contract_path": "src/uniswap/UniswapV3Swapper.sol", - "line_no": 77, - "src": "2132:144", - "src_char": "2132:144" - }, - { - "contract_path": "src/uniswap/UniswapV3Swapper.sol", - "line_no": 80, - "src": "2312:322", - "src_char": "2312:322" - }, - { - "contract_path": "src/uniswap/UniswapV3Swapper.sol", - "line_no": 91, - "src": "2690:132", - "src_char": "2690:132" - }, - { - "contract_path": "src/uniswap/UniswapV3Swapper.sol", - "line_no": 94, - "src": "2852:237", - "src_char": "2852:237" - } - ] - }, { "title": "Arbitrary `from` passed to `transferFrom` (or `safeTransferFrom`)", "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. ", @@ -4252,6 +4143,115 @@ } ] }, + { + "title": "Using `block.timestamp` for swap deadline offers no protection", + "description": "In the PoS model, proposers know well in advance if they will propose one or consecutive blocks ahead of time. In such a scenario, a malicious validator can hold back the transaction and execute it at a more favourable block number.Consider allowing function caller to specify swap deadline input parameter.", + "detector_name": "block-timestamp-is-weak-deadline", + "instances": [ + { + "contract_path": "src/Trump.sol", + "line_no": 290, + "src": "9949:190", + "src_char": "9949:190" + }, + { + "contract_path": "src/uniswap/UniswapV2Swapper.sol", + "line_no": 23, + "src": "670:83", + "src_char": "670:83" + }, + { + "contract_path": "src/uniswap/UniswapV2Swapper.sol", + "line_no": 24, + "src": "763:83", + "src_char": "763:83" + }, + { + "contract_path": "src/uniswap/UniswapV2Swapper.sol", + "line_no": 25, + "src": "856:70", + "src_char": "856:70" + }, + { + "contract_path": "src/uniswap/UniswapV2Swapper.sol", + "line_no": 26, + "src": "936:80", + "src_char": "936:80" + }, + { + "contract_path": "src/uniswap/UniswapV2Swapper.sol", + "line_no": 27, + "src": "1026:80", + "src_char": "1026:80" + }, + { + "contract_path": "src/uniswap/UniswapV2Swapper.sol", + "line_no": 31, + "src": "1278:112", + "src_char": "1278:112" + }, + { + "contract_path": "src/uniswap/UniswapV2Swapper.sol", + "line_no": 32, + "src": "1400:99", + "src_char": "1400:99" + }, + { + "contract_path": "src/uniswap/UniswapV2Swapper.sol", + "line_no": 33, + "src": "1509:109", + "src_char": "1509:109" + }, + { + "contract_path": "src/uniswap/UniswapV3Swapper.sol", + "line_no": 52, + "src": "1115:143", + "src_char": "1115:143" + }, + { + "contract_path": "src/uniswap/UniswapV3Swapper.sol", + "line_no": 55, + "src": "1293:321", + "src_char": "1293:321" + }, + { + "contract_path": "src/uniswap/UniswapV3Swapper.sol", + "line_no": 66, + "src": "1668:131", + "src_char": "1668:131" + }, + { + "contract_path": "src/uniswap/UniswapV3Swapper.sol", + "line_no": 69, + "src": "1828:236", + "src_char": "1828:236" + }, + { + "contract_path": "src/uniswap/UniswapV3Swapper.sol", + "line_no": 77, + "src": "2132:144", + "src_char": "2132:144" + }, + { + "contract_path": "src/uniswap/UniswapV3Swapper.sol", + "line_no": 80, + "src": "2312:322", + "src_char": "2312:322" + }, + { + "contract_path": "src/uniswap/UniswapV3Swapper.sol", + "line_no": 91, + "src": "2690:132", + "src_char": "2690:132" + }, + { + "contract_path": "src/uniswap/UniswapV3Swapper.sol", + "line_no": 94, + "src": "2852:237", + "src_char": "2852:237" + } + ] + }, { "title": "Using `ERC721::_mint()` can be dangerous", "description": "Using `ERC721::_mint()` can mint ERC721 tokens to addresses which don't support ERC721 tokens. Use `_safeMint()` instead of `_mint()` for ERC721.", diff --git a/reports/report.md b/reports/report.md index 9159cb73..f44c2a82 100644 --- a/reports/report.md +++ b/reports/report.md @@ -10,45 +10,44 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [High Issues](#high-issues) - [H-1: Using `delegatecall` in loop](#h-1-using-delegatecall-in-loop) - [H-2: `abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()`](#h-2-abiencodepacked-should-not-be-used-with-dynamic-types-when-passing-the-result-to-a-hash-function-such-as-keccak256) - - [H-3: Using `block.timestamp` for swap deadline offers no protection](#h-3-using-blocktimestamp-for-swap-deadline-offers-no-protection) - - [H-4: Arbitrary `from` passed to `transferFrom` (or `safeTransferFrom`)](#h-4-arbitrary-from-passed-to-transferfrom-or-safetransferfrom) - - [H-5: Unprotected initializer](#h-5-unprotected-initializer) - - [H-6: Unsafe Casting](#h-6-unsafe-casting) - - [H-7: EnumerableSet.remove in loop corrupts the set order.](#h-7-enumerablesetremove-in-loop-corrupts-the-set-order) - - [H-8: Experimental ABI Encoder](#h-8-experimental-abi-encoder) - - [H-9: Incorrect Assembly Shift Parameter Order](#h-9-incorrect-assembly-shift-parameter-order) - - [H-10: Storage Array Edited with Memory](#h-10-storage-array-edited-with-memory) - - [H-11: Contract Has Multiple Constructors](#h-11-contract-has-multiple-constructors) - - [H-12: Contract Name Reused in Different Files](#h-12-contract-name-reused-in-different-files) - - [H-13: Nested Structs in Mappings pre-0.5.0](#h-13-nested-structs-in-mappings-pre-050) - - [H-14: Depracated EVM Instruction for `selfdestruct` should not be used.](#h-14-depracated-evm-instruction-for-selfdestruct-should-not-be-used) - - [H-15: Array length value has a direct assignment.](#h-15-array-length-value-has-a-direct-assignment) - - [H-16: Uninitialized State Variables](#h-16-uninitialized-state-variables) - - [H-17: Incorrect use of caret operator on a non hexadcimal constant](#h-17-incorrect-use-of-caret-operator-on-a-non-hexadcimal-constant) - - [H-18: Yul block contains `return` function call.](#h-18-yul-block-contains-return-function-call) - - [H-19: Shadowed State Variables in Inheritance Hierarchy](#h-19-shadowed-state-variables-in-inheritance-hierarchy) - - [H-20: Unchecked `bool success` value for send call.](#h-20-unchecked-bool-success-value-for-send-call) - - [H-21: Misused boolean with logical operators](#h-21-misused-boolean-with-logical-operators) - - [H-22: Functions send eth away from contract but performs no checks on any address.](#h-22-functions-send-eth-away-from-contract-but-performs-no-checks-on-any-address) - - [H-23: Delegatecall made by the function without checks on any address.](#h-23-delegatecall-made-by-the-function-without-checks-on-any-address) - - [H-24: Tautological comparison.](#h-24-tautological-comparison) - - [H-25: RTLO character detected in file. \u{202e}](#h-25-rtlo-character-detected-in-file-u202e) - - [H-26: Dangerous unary operator found in assignment.](#h-26-dangerous-unary-operator-found-in-assignment) - - [H-27: Tautology or Contradiction in comparison.](#h-27-tautology-or-contradiction-in-comparison) - - [H-28: Dangerous strict equality checks on contract balances.](#h-28-dangerous-strict-equality-checks-on-contract-balances) - - [H-29: Compiler Bug: Signed array in storage detected for compiler version `<0.5.10`](#h-29-compiler-bug-signed-array-in-storage-detected-for-compiler-version-0510) - - [H-30: Weak Randomness](#h-30-weak-randomness) - - [H-31: Usage of variable before declaration.](#h-31-usage-of-variable-before-declaration) - - [H-32: Deletion from a nested mappping.](#h-32-deletion-from-a-nested-mappping) - - [H-33: Potential use of `tx.origin` for authentication.](#h-33-potential-use-of-txorigin-for-authentication) - - [H-34: Loop contains `msg.value`.](#h-34-loop-contains-msgvalue) - - [H-35: Contract locks Ether without a withdraw function.](#h-35-contract-locks-ether-without-a-withdraw-function) - - [H-36: Incorrect ERC721 interface.](#h-36-incorrect-erc721-interface) - - [H-37: Incorrect ERC20 interface.](#h-37-incorrect-erc20-interface) - - [H-38: Out of order retryable transactions.](#h-38-out-of-order-retryable-transactions) - - [H-39: Constant functions changing state](#h-39-constant-functions-changing-state) - - [H-40: Function selector collides with other functions](#h-40-function-selector-collides-with-other-functions) - - [H-41: Unchecked Low level calls](#h-41-unchecked-low-level-calls) + - [H-3: Arbitrary `from` passed to `transferFrom` (or `safeTransferFrom`)](#h-3-arbitrary-from-passed-to-transferfrom-or-safetransferfrom) + - [H-4: Unprotected initializer](#h-4-unprotected-initializer) + - [H-5: Unsafe Casting](#h-5-unsafe-casting) + - [H-6: EnumerableSet.remove in loop corrupts the set order.](#h-6-enumerablesetremove-in-loop-corrupts-the-set-order) + - [H-7: Experimental ABI Encoder](#h-7-experimental-abi-encoder) + - [H-8: Incorrect Assembly Shift Parameter Order](#h-8-incorrect-assembly-shift-parameter-order) + - [H-9: Storage Array Edited with Memory](#h-9-storage-array-edited-with-memory) + - [H-10: Contract Has Multiple Constructors](#h-10-contract-has-multiple-constructors) + - [H-11: Contract Name Reused in Different Files](#h-11-contract-name-reused-in-different-files) + - [H-12: Nested Structs in Mappings pre-0.5.0](#h-12-nested-structs-in-mappings-pre-050) + - [H-13: Depracated EVM Instruction for `selfdestruct` should not be used.](#h-13-depracated-evm-instruction-for-selfdestruct-should-not-be-used) + - [H-14: Array length value has a direct assignment.](#h-14-array-length-value-has-a-direct-assignment) + - [H-15: Uninitialized State Variables](#h-15-uninitialized-state-variables) + - [H-16: Incorrect use of caret operator on a non hexadcimal constant](#h-16-incorrect-use-of-caret-operator-on-a-non-hexadcimal-constant) + - [H-17: Yul block contains `return` function call.](#h-17-yul-block-contains-return-function-call) + - [H-18: Shadowed State Variables in Inheritance Hierarchy](#h-18-shadowed-state-variables-in-inheritance-hierarchy) + - [H-19: Unchecked `bool success` value for send call.](#h-19-unchecked-bool-success-value-for-send-call) + - [H-20: Misused boolean with logical operators](#h-20-misused-boolean-with-logical-operators) + - [H-21: Functions send eth away from contract but performs no checks on any address.](#h-21-functions-send-eth-away-from-contract-but-performs-no-checks-on-any-address) + - [H-22: Delegatecall made by the function without checks on any address.](#h-22-delegatecall-made-by-the-function-without-checks-on-any-address) + - [H-23: Tautological comparison.](#h-23-tautological-comparison) + - [H-24: RTLO character detected in file. \u{202e}](#h-24-rtlo-character-detected-in-file-u202e) + - [H-25: Dangerous unary operator found in assignment.](#h-25-dangerous-unary-operator-found-in-assignment) + - [H-26: Tautology or Contradiction in comparison.](#h-26-tautology-or-contradiction-in-comparison) + - [H-27: Dangerous strict equality checks on contract balances.](#h-27-dangerous-strict-equality-checks-on-contract-balances) + - [H-28: Compiler Bug: Signed array in storage detected for compiler version `<0.5.10`](#h-28-compiler-bug-signed-array-in-storage-detected-for-compiler-version-0510) + - [H-29: Weak Randomness](#h-29-weak-randomness) + - [H-30: Usage of variable before declaration.](#h-30-usage-of-variable-before-declaration) + - [H-31: Deletion from a nested mappping.](#h-31-deletion-from-a-nested-mappping) + - [H-32: Potential use of `tx.origin` for authentication.](#h-32-potential-use-of-txorigin-for-authentication) + - [H-33: Loop contains `msg.value`.](#h-33-loop-contains-msgvalue) + - [H-34: Contract locks Ether without a withdraw function.](#h-34-contract-locks-ether-without-a-withdraw-function) + - [H-35: Incorrect ERC721 interface.](#h-35-incorrect-erc721-interface) + - [H-36: Incorrect ERC20 interface.](#h-36-incorrect-erc20-interface) + - [H-37: Out of order retryable transactions.](#h-37-out-of-order-retryable-transactions) + - [H-38: Constant functions changing state](#h-38-constant-functions-changing-state) + - [H-39: Function selector collides with other functions](#h-39-function-selector-collides-with-other-functions) + - [H-40: Unchecked Low level calls](#h-40-unchecked-low-level-calls) - [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) @@ -62,40 +61,41 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [L-10: Event is missing `indexed` fields](#l-10-event-is-missing-indexed-fields) - [L-11: Empty `require()` / `revert()` statements](#l-11-empty-require--revert-statements) - [L-12: The `nonReentrant` `modifier` should occur before all other modifiers](#l-12-the-nonreentrant-modifier-should-occur-before-all-other-modifiers) - - [L-13: Using `ERC721::_mint()` can be dangerous](#l-13-using-erc721mint-can-be-dangerous) - - [L-14: PUSH0 is not supported by all chains](#l-14-push0-is-not-supported-by-all-chains) - - [L-15: Modifiers invoked only once can be shoe-horned into the function](#l-15-modifiers-invoked-only-once-can-be-shoe-horned-into-the-function) - - [L-16: Empty Block](#l-16-empty-block) - - [L-17: Large literal values multiples of 10000 can be replaced with scientific notation](#l-17-large-literal-values-multiples-of-10000-can-be-replaced-with-scientific-notation) - - [L-18: Internal functions called only once can be inlined](#l-18-internal-functions-called-only-once-can-be-inlined) - - [L-19: Contract still has TODOs](#l-19-contract-still-has-todos) - - [L-20: Inconsistency in declaring uint256/uint (or) int256/int variables within a contract. Use explicit size declarations (uint256 or int256).](#l-20-inconsistency-in-declaring-uint256uint-or-int256int-variables-within-a-contract-use-explicit-size-declarations-uint256-or-int256) - - [L-21: Unused Custom Error](#l-21-unused-custom-error) - - [L-22: Loop contains `require`/`revert` statements](#l-22-loop-contains-requirerevert-statements) - - [L-23: Incorrect Order of Division and Multiplication](#l-23-incorrect-order-of-division-and-multiplication) - - [L-24: Redundant statements have no effect.](#l-24-redundant-statements-have-no-effect) - - [L-25: Public variables of a contract read in an external context (using `this`).](#l-25-public-variables-of-a-contract-read-in-an-external-context-using-this) - - [L-26: Potentially unused `private` / `internal` state variables found.](#l-26-potentially-unused-private--internal-state-variables-found) - - [L-27: Functions declared `pure` / `view` but contains assembly](#l-27-functions-declared-pure--view-but-contains-assembly) - - [L-28: Boolean equality is not required.](#l-28-boolean-equality-is-not-required) - - [L-29: Local variable shadows state variables in the contract hirearchy](#l-29-local-variable-shadows-state-variables-in-the-contract-hirearchy) - - [L-30: Uninitialized local variables.](#l-30-uninitialized-local-variables) - - [L-31: Return Bomb](#l-31-return-bomb) - - [L-32: Function initializing state.](#l-32-function-initializing-state) - - [L-33: Dead Code](#l-33-dead-code) - - [L-34: Loop condition contains `state_variable.length` that could be cached outside.](#l-34-loop-condition-contains-statevariablelength-that-could-be-cached-outside) - - [L-35: Incorrect use of `assert()`](#l-35-incorrect-use-of-assert) - - [L-36: Costly operations inside loops.](#l-36-costly-operations-inside-loops) - - [L-37: Builtin Symbol Shadowing](#l-37-builtin-symbol-shadowing) - - [L-38: Void constructor](#l-38-void-constructor) - - [L-39: Potentially missing inheritance for contract.](#l-39-potentially-missing-inheritance-for-contract) - - [L-40: Unused Imports](#l-40-unused-imports) - - [L-41: Function pointers used in constructors.](#l-41-function-pointers-used-in-constructors) - - [L-42: State variable could be declared constant](#l-42-state-variable-could-be-declared-constant) - - [L-43: State variable changes but no event is emitted.](#l-43-state-variable-changes-but-no-event-is-emitted) - - [L-44: State variable could be declared immutable](#l-44-state-variable-could-be-declared-immutable) - - [L-45: Modifier has multiple placeholders.](#l-45-modifier-has-multiple-placeholders) - - [L-46: Return value of the function call is not checked.](#l-46-return-value-of-the-function-call-is-not-checked) + - [L-13: Using `block.timestamp` for swap deadline offers no protection](#l-13-using-blocktimestamp-for-swap-deadline-offers-no-protection) + - [L-14: Using `ERC721::_mint()` can be dangerous](#l-14-using-erc721mint-can-be-dangerous) + - [L-15: PUSH0 is not supported by all chains](#l-15-push0-is-not-supported-by-all-chains) + - [L-16: Modifiers invoked only once can be shoe-horned into the function](#l-16-modifiers-invoked-only-once-can-be-shoe-horned-into-the-function) + - [L-17: Empty Block](#l-17-empty-block) + - [L-18: Large literal values multiples of 10000 can be replaced with scientific notation](#l-18-large-literal-values-multiples-of-10000-can-be-replaced-with-scientific-notation) + - [L-19: Internal functions called only once can be inlined](#l-19-internal-functions-called-only-once-can-be-inlined) + - [L-20: Contract still has TODOs](#l-20-contract-still-has-todos) + - [L-21: Inconsistency in declaring uint256/uint (or) int256/int variables within a contract. Use explicit size declarations (uint256 or int256).](#l-21-inconsistency-in-declaring-uint256uint-or-int256int-variables-within-a-contract-use-explicit-size-declarations-uint256-or-int256) + - [L-22: Unused Custom Error](#l-22-unused-custom-error) + - [L-23: Loop contains `require`/`revert` statements](#l-23-loop-contains-requirerevert-statements) + - [L-24: Incorrect Order of Division and Multiplication](#l-24-incorrect-order-of-division-and-multiplication) + - [L-25: Redundant statements have no effect.](#l-25-redundant-statements-have-no-effect) + - [L-26: Public variables of a contract read in an external context (using `this`).](#l-26-public-variables-of-a-contract-read-in-an-external-context-using-this) + - [L-27: Potentially unused `private` / `internal` state variables found.](#l-27-potentially-unused-private--internal-state-variables-found) + - [L-28: Functions declared `pure` / `view` but contains assembly](#l-28-functions-declared-pure--view-but-contains-assembly) + - [L-29: Boolean equality is not required.](#l-29-boolean-equality-is-not-required) + - [L-30: Local variable shadows state variables in the contract hirearchy](#l-30-local-variable-shadows-state-variables-in-the-contract-hirearchy) + - [L-31: Uninitialized local variables.](#l-31-uninitialized-local-variables) + - [L-32: Return Bomb](#l-32-return-bomb) + - [L-33: Function initializing state.](#l-33-function-initializing-state) + - [L-34: Dead Code](#l-34-dead-code) + - [L-35: Loop condition contains `state_variable.length` that could be cached outside.](#l-35-loop-condition-contains-statevariablelength-that-could-be-cached-outside) + - [L-36: Incorrect use of `assert()`](#l-36-incorrect-use-of-assert) + - [L-37: Costly operations inside loops.](#l-37-costly-operations-inside-loops) + - [L-38: Builtin Symbol Shadowing](#l-38-builtin-symbol-shadowing) + - [L-39: Void constructor](#l-39-void-constructor) + - [L-40: Potentially missing inheritance for contract.](#l-40-potentially-missing-inheritance-for-contract) + - [L-41: Unused Imports](#l-41-unused-imports) + - [L-42: Function pointers used in constructors.](#l-42-function-pointers-used-in-constructors) + - [L-43: State variable could be declared constant](#l-43-state-variable-could-be-declared-constant) + - [L-44: State variable changes but no event is emitted.](#l-44-state-variable-changes-but-no-event-is-emitted) + - [L-45: State variable could be declared immutable](#l-45-state-variable-could-be-declared-immutable) + - [L-46: Modifier has multiple placeholders.](#l-46-modifier-has-multiple-placeholders) + - [L-47: Return value of the function call is not checked.](#l-47-return-value-of-the-function-call-is-not-checked) # Summary @@ -231,8 +231,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 41 | -| Low | 46 | +| High | 40 | +| Low | 47 | # High Issues @@ -284,120 +284,7 @@ If all arguments are strings and or bytes, `bytes.concat()` should be used inste -## H-3: Using `block.timestamp` for swap deadline offers no protection - -In the PoS model, proposers know well in advance if they will propose one or consecutive blocks ahead of time. In such a scenario, a malicious validator can hold back the transaction and execute it at a more favourable block number.Consider allowing function caller to specify swap deadline input parameter. - -
17 Found Instances - - -- Found in src/Trump.sol [Line: 290](../tests/contract-playground/src/Trump.sol#L290) - - ```solidity - uniswapV2Router.swapExactTokensForETHSupportingFeeOnTransferTokens( - ``` - -- Found in src/uniswap/UniswapV2Swapper.sol [Line: 23](../tests/contract-playground/src/uniswap/UniswapV2Swapper.sol#L23) - - ```solidity - router1.swapExactTokensForTokens(amountIn, amountOutMin, path, to, block.timestamp); - ``` - -- Found in src/uniswap/UniswapV2Swapper.sol [Line: 24](../tests/contract-playground/src/uniswap/UniswapV2Swapper.sol#L24) - - ```solidity - router1.swapTokensForExactTokens(amountOut, amountInMax, path, to, block.timestamp); - ``` - -- Found in src/uniswap/UniswapV2Swapper.sol [Line: 25](../tests/contract-playground/src/uniswap/UniswapV2Swapper.sol#L25) - - ```solidity - router1.swapExactETHForTokens(amountOutMin, path, to, block.timestamp); - ``` - -- Found in src/uniswap/UniswapV2Swapper.sol [Line: 26](../tests/contract-playground/src/uniswap/UniswapV2Swapper.sol#L26) - - ```solidity - router1.swapTokensForExactETH(amountOut, amountInMax, path, to, block.timestamp); - ``` - -- Found in src/uniswap/UniswapV2Swapper.sol [Line: 27](../tests/contract-playground/src/uniswap/UniswapV2Swapper.sol#L27) - - ```solidity - router1.swapExactTokensForETH(amountIn, amountOutMin, path, to, block.timestamp); - ``` - -- Found in src/uniswap/UniswapV2Swapper.sol [Line: 31](../tests/contract-playground/src/uniswap/UniswapV2Swapper.sol#L31) - - ```solidity - router2.swapExactTokensForTokensSupportingFeeOnTransferTokens(amountIn, amountOutMin, path, to, block.timestamp); - ``` - -- Found in src/uniswap/UniswapV2Swapper.sol [Line: 32](../tests/contract-playground/src/uniswap/UniswapV2Swapper.sol#L32) - - ```solidity - router2.swapExactETHForTokensSupportingFeeOnTransferTokens(amountOutMin, path, to, block.timestamp); - ``` - -- Found in src/uniswap/UniswapV2Swapper.sol [Line: 33](../tests/contract-playground/src/uniswap/UniswapV2Swapper.sol#L33) - - ```solidity - router2.swapExactTokensForETHSupportingFeeOnTransferTokens(amountIn, amountOutMin, path, to, block.timestamp); - ``` - -- Found in src/uniswap/UniswapV3Swapper.sol [Line: 52](../tests/contract-playground/src/uniswap/UniswapV3Swapper.sol#L52) - - ```solidity - ExactInputSingleParams memory exactInputSingleParams = ExactInputSingleParams( - ``` - -- Found in src/uniswap/UniswapV3Swapper.sol [Line: 55](../tests/contract-playground/src/uniswap/UniswapV3Swapper.sol#L55) - - ```solidity - exactInputSingleParams = ExactInputSingleParams({ - ``` - -- Found in src/uniswap/UniswapV3Swapper.sol [Line: 66](../tests/contract-playground/src/uniswap/UniswapV3Swapper.sol#L66) - - ```solidity - ExactInputParams memory exactInputParams = ExactInputParams( - ``` - -- Found in src/uniswap/UniswapV3Swapper.sol [Line: 69](../tests/contract-playground/src/uniswap/UniswapV3Swapper.sol#L69) - - ```solidity - exactInputParams = ExactInputParams({ - ``` - -- Found in src/uniswap/UniswapV3Swapper.sol [Line: 77](../tests/contract-playground/src/uniswap/UniswapV3Swapper.sol#L77) - - ```solidity - ExactOutputSingleParams memory exactOutputSingleParams = ExactOutputSingleParams( - ``` - -- Found in src/uniswap/UniswapV3Swapper.sol [Line: 80](../tests/contract-playground/src/uniswap/UniswapV3Swapper.sol#L80) - - ```solidity - exactOutputSingleParams = ExactOutputSingleParams({ - ``` - -- Found in src/uniswap/UniswapV3Swapper.sol [Line: 91](../tests/contract-playground/src/uniswap/UniswapV3Swapper.sol#L91) - - ```solidity - ExactOutputParams memory exactOutputParams = ExactOutputParams( - ``` - -- Found in src/uniswap/UniswapV3Swapper.sol [Line: 94](../tests/contract-playground/src/uniswap/UniswapV3Swapper.sol#L94) - - ```solidity - exactOutputParams = ExactOutputParams({ - ``` - -
- - - -## H-4: Arbitrary `from` passed to `transferFrom` (or `safeTransferFrom`) +## H-3: 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. @@ -444,7 +331,7 @@ Passing an arbitrary `from` address to `transferFrom` (or `safeTransferFrom`) ca -## H-5: Unprotected initializer +## H-4: Unprotected initializer Consider protecting the initializer functions with modifiers. @@ -461,7 +348,7 @@ Consider protecting the initializer functions with modifiers. -## H-6: Unsafe Casting +## H-5: 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. @@ -1036,7 +923,7 @@ Downcasting int/uints in Solidity can be unsafe due to the potential for data lo -## H-7: EnumerableSet.remove in loop corrupts the set order. +## H-6: EnumerableSet.remove in loop corrupts the set order. If the order of an EnumerableSet is required, removing items in a loop using `at` and `remove` corrupts this order. Consider using a different data structure or removing items by collecting them during the loop, then removing after the loop. @@ -1078,7 +965,7 @@ Consider using a different data structure or removing items by collecting them d -## H-8: Experimental ABI Encoder +## H-7: Experimental ABI Encoder Experimental encoders should not be used in production. There are multiple known compiler bugs that are caused by the experimental encoder. Upgrade your solidity version to remove the need for experimental features. @@ -1095,7 +982,7 @@ Experimental encoders should not be used in production. There are multiple known -## H-9: Incorrect Assembly Shift Parameter Order +## H-8: Incorrect Assembly Shift Parameter Order Example: `shl(shifted, 4)` will shift the right constant `4` by `a` bits. The correct order is `shl(4, shifted)`. @@ -1118,7 +1005,7 @@ Example: `shl(shifted, 4)` will shift the right constant `4` by `a` bits. The co -## H-10: Storage Array Edited with Memory +## H-9: Storage Array Edited with Memory Storage reference is passed to a function with a memory parameter. This will not update the storage variable as expected. Consider using storage parameters instead. @@ -1135,7 +1022,7 @@ Storage reference is passed to a function with a memory parameter. This will not -## H-11: Contract Has Multiple Constructors +## H-10: Contract Has Multiple Constructors In some versions of Solidity, contracts compile with multiple constructors. The first constructor takes precedence. This can lead to unexpected behavior. @@ -1152,7 +1039,7 @@ In some versions of Solidity, contracts compile with multiple constructors. The -## H-12: Contract Name Reused in Different Files +## H-11: 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. @@ -1187,7 +1074,7 @@ When compiling contracts with certain development frameworks (for example: Truff -## H-13: Nested Structs in Mappings pre-0.5.0 +## H-12: Nested Structs in Mappings pre-0.5.0 Prior to updates in Solidity 0.5.0, public mappings with nested structs compiled, but produced incorrect values. Refrain from using these, or update to a more recent version of Solidity. @@ -1204,7 +1091,7 @@ Prior to updates in Solidity 0.5.0, public mappings with nested structs compiled -## H-14: Depracated EVM Instruction for `selfdestruct` should not be used. +## H-13: Depracated EVM Instruction for `selfdestruct` should not be used. @@ -1221,7 +1108,7 @@ Prior to updates in Solidity 0.5.0, public mappings with nested structs compiled -## H-15: Array length value has a direct assignment. +## H-14: Array length value has a direct assignment. If the length of a dynamic array (storage variable) directly assigned to, it may allow access to other storage slots by tweaking it's value. This practice has been depracated in newer Solidity versions @@ -1262,7 +1149,7 @@ If the length of a dynamic array (storage variable) directly assigned to, it may -## H-16: Uninitialized State Variables +## H-15: Uninitialized State Variables Solidity does initialize variables by default when you declare them, however it's good practice to explicitly declare an initial value. For example, if you transfer money to an address we must make sure that the address has been initialized. @@ -1483,7 +1370,7 @@ Solidity does initialize variables by default when you declare them, however it' -## H-17: Incorrect use of caret operator on a non hexadcimal constant +## H-16: Incorrect use of caret operator on a non hexadcimal constant The caret operator is usually mistakenly thought of as an exponentiation operator but actually, it's a bitwise xor operator. @@ -1524,7 +1411,7 @@ The caret operator is usually mistakenly thought of as an exponentiation operato -## H-18: Yul block contains `return` function call. +## H-17: Yul block contains `return` function call. Remove this, as this causes execution to halt. Nothing after that call will execute, including code following the assembly block. @@ -1541,7 +1428,7 @@ Remove this, as this causes execution to halt. Nothing after that call will exec -## H-19: Shadowed State Variables in Inheritance Hierarchy +## H-18: Shadowed State Variables in Inheritance Hierarchy This vulnerability arises when a derived contract unintentionally shadows a state variable from a parent contract by declaring a variable with the same name. This can be misleading. To prevent this, ensure variable names are unique across the inheritance hierarchy or use proper visibility and scope controls. @@ -1558,7 +1445,7 @@ This vulnerability arises when a derived contract unintentionally shadows a stat -## H-20: Unchecked `bool success` value for send call. +## H-19: Unchecked `bool success` value for send call. The transaction `address(payable?).send(address)` may fail because of reasons like out-of-gas, invalid receipient address or revert from the recipient. Therefore, the boolean returned by this function call must be checked to be `true` in order to verify that the transaction was successful @@ -1575,7 +1462,7 @@ The transaction `address(payable?).send(address)` may fail because of reasons li -## H-21: Misused boolean with logical operators +## H-20: Misused boolean with logical operators The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively. @@ -1646,7 +1533,7 @@ The patterns `if (… || true)` and `if (.. && false)` will always evaluate to t -## H-22: Functions send eth away from contract but performs no checks on any address. +## H-21: Functions send eth away from contract but performs no checks on any address. Consider introducing checks for `msg.sender` to ensure the recipient of the money is as intended. @@ -1777,7 +1664,7 @@ Consider introducing checks for `msg.sender` to ensure the recipient of the mone -## H-23: Delegatecall made by the function without checks on any address. +## H-22: Delegatecall made by the function without checks on any address. Introduce checks on the address @@ -1818,7 +1705,7 @@ Introduce checks on the address -## H-24: Tautological comparison. +## H-23: Tautological comparison. The left hand side and the right hand side of the binary operation has the same value. This makes the condition always true or always false. @@ -1853,7 +1740,7 @@ The left hand side and the right hand side of the binary operation has the same -## H-25: RTLO character detected in file. \u{202e} +## H-24: RTLO character detected in file. \u{202e} Right to left override character may be misledaing and cause potential attacks by visually misordering method arguments! @@ -1870,7 +1757,7 @@ Right to left override character may be misledaing and cause potential attacks b -## H-26: Dangerous unary operator found in assignment. +## H-25: Dangerous unary operator found in assignment. Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in between. @@ -1893,7 +1780,7 @@ Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in -## H-27: Tautology or Contradiction in comparison. +## H-26: Tautology or Contradiction in comparison. The condition has been determined to be either always true or always false due to the integer range in which we're operating. @@ -1916,7 +1803,7 @@ The condition has been determined to be either always true or always false due t -## H-28: Dangerous strict equality checks on contract balances. +## H-27: Dangerous strict equality checks on contract balances. A contract's balance can be forcibly manipulated by another selfdestructing contract. Therefore, it's recommended to use >, <, >= or <= instead of strict equality. @@ -1945,7 +1832,7 @@ A contract's balance can be forcibly manipulated by another selfdestructing cont -## H-29: Compiler Bug: Signed array in storage detected for compiler version `<0.5.10` +## H-28: Compiler Bug: Signed array in storage detected for compiler version `<0.5.10` If you want to leverage signed arrays in storage by assigning a literal array with at least one negative number, then you mus use solidity version 0.5.10 or above. This is because of a bug in older compilers. @@ -1962,7 +1849,7 @@ If you want to leverage signed arrays in storage by assigning a literal array wi -## H-30: Weak Randomness +## H-29: 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. @@ -2027,7 +1914,7 @@ The use of keccak256 hash functions on predictable values like block.timestamp, -## H-31: Usage of variable before declaration. +## H-30: Usage of variable before declaration. This is a bad practice that may lead to unintended consequences. Please declare the variable before using it. @@ -2044,7 +1931,7 @@ This is a bad practice that may lead to unintended consequences. Please declare -## H-32: Deletion from a nested mappping. +## H-31: 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. @@ -2061,7 +1948,7 @@ A deletion in a structure containing a mapping will not delete the mapping. The -## H-33: Potential use of `tx.origin` for authentication. +## H-32: Potential use of `tx.origin` for authentication. Using `tx.origin` may lead to problems when users are interacting via smart contract with your protocol. It is recommended to use `msg.sender` for authentication. @@ -2090,7 +1977,7 @@ Using `tx.origin` may lead to problems when users are interacting via smart cont -## H-34: Loop contains `msg.value`. +## H-33: Loop contains `msg.value`. Provide an explicit array of amounts alongside the receivers array, and check that the sum of all amounts matches `msg.value`. @@ -2125,7 +2012,7 @@ Provide an explicit array of amounts alongside the receivers array, and check th -## H-35: Contract locks Ether without a withdraw function. +## 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. @@ -2196,7 +2083,7 @@ It appears that the contract includes a payable function to accept Ether but lac -## H-36: Incorrect ERC721 interface. +## H-35: Incorrect ERC721 interface. Incorrect return values for ERC721 functions. A contract compiled with Solidity > 0.4.22 interacting with these functions will fail to execute them, as the return value is missing. Set the appropriate return values and types for the defined ERC721 functions. @@ -2255,7 +2142,7 @@ Incorrect return values for ERC721 functions. A contract compiled with Solidity -## H-37: Incorrect ERC20 interface. +## H-36: Incorrect ERC20 interface. Incorrect return values for ERC20 functions. A contract compiled with Solidity > 0.4.22 interacting with these functions will fail to execute them, as the return value is missing. Set the appropriate return values and types for the defined ERC20 functions. @@ -2296,7 +2183,7 @@ Incorrect return values for ERC20 functions. A contract compiled with Solidity > -## H-38: Out of order retryable transactions. +## H-37: 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 @@ -2321,7 +2208,7 @@ Do not rely on the order or successful execution of retryable tickets. Functions -## H-39: Constant functions changing state +## H-38: Constant functions changing state Function is declared constant/view but it changes state. Ensure that the attributes of contract compiled prior to 0.5 are correct. @@ -2338,7 +2225,7 @@ Function is declared constant/view but it changes state. Ensure that the attribu -## H-40: Function selector collides with other functions +## H-39: Function selector collides with other functions Function selector collides with other functions. This may cause the solidity function dispatcher to invoke the wrong function if the functions happen to be included in the same contract through an inheritance hirearchy later down the line. It is recommended to rename this function or change its parameters. @@ -2363,7 +2250,7 @@ Function selector collides with other functions. This may cause the solidity fun -## H-41: Unchecked Low level calls +## H-40: Unchecked Low level calls The return value of the low-level call is not checked, so if the call fails, the Ether will be locked in the contract. If the low level is used to prevent blocking operations, consider logging failed calls. Ensure that the return value of a low-level call is checked or logged. @@ -4221,7 +4108,120 @@ This is a best-practice to protect against reentrancy in other modifiers. -## L-13: Using `ERC721::_mint()` can be dangerous +## L-13: Using `block.timestamp` for swap deadline offers no protection + +In the PoS model, proposers know well in advance if they will propose one or consecutive blocks ahead of time. In such a scenario, a malicious validator can hold back the transaction and execute it at a more favourable block number.Consider allowing function caller to specify swap deadline input parameter. + +
17 Found Instances + + +- Found in src/Trump.sol [Line: 290](../tests/contract-playground/src/Trump.sol#L290) + + ```solidity + uniswapV2Router.swapExactTokensForETHSupportingFeeOnTransferTokens( + ``` + +- Found in src/uniswap/UniswapV2Swapper.sol [Line: 23](../tests/contract-playground/src/uniswap/UniswapV2Swapper.sol#L23) + + ```solidity + router1.swapExactTokensForTokens(amountIn, amountOutMin, path, to, block.timestamp); + ``` + +- Found in src/uniswap/UniswapV2Swapper.sol [Line: 24](../tests/contract-playground/src/uniswap/UniswapV2Swapper.sol#L24) + + ```solidity + router1.swapTokensForExactTokens(amountOut, amountInMax, path, to, block.timestamp); + ``` + +- Found in src/uniswap/UniswapV2Swapper.sol [Line: 25](../tests/contract-playground/src/uniswap/UniswapV2Swapper.sol#L25) + + ```solidity + router1.swapExactETHForTokens(amountOutMin, path, to, block.timestamp); + ``` + +- Found in src/uniswap/UniswapV2Swapper.sol [Line: 26](../tests/contract-playground/src/uniswap/UniswapV2Swapper.sol#L26) + + ```solidity + router1.swapTokensForExactETH(amountOut, amountInMax, path, to, block.timestamp); + ``` + +- Found in src/uniswap/UniswapV2Swapper.sol [Line: 27](../tests/contract-playground/src/uniswap/UniswapV2Swapper.sol#L27) + + ```solidity + router1.swapExactTokensForETH(amountIn, amountOutMin, path, to, block.timestamp); + ``` + +- Found in src/uniswap/UniswapV2Swapper.sol [Line: 31](../tests/contract-playground/src/uniswap/UniswapV2Swapper.sol#L31) + + ```solidity + router2.swapExactTokensForTokensSupportingFeeOnTransferTokens(amountIn, amountOutMin, path, to, block.timestamp); + ``` + +- Found in src/uniswap/UniswapV2Swapper.sol [Line: 32](../tests/contract-playground/src/uniswap/UniswapV2Swapper.sol#L32) + + ```solidity + router2.swapExactETHForTokensSupportingFeeOnTransferTokens(amountOutMin, path, to, block.timestamp); + ``` + +- Found in src/uniswap/UniswapV2Swapper.sol [Line: 33](../tests/contract-playground/src/uniswap/UniswapV2Swapper.sol#L33) + + ```solidity + router2.swapExactTokensForETHSupportingFeeOnTransferTokens(amountIn, amountOutMin, path, to, block.timestamp); + ``` + +- Found in src/uniswap/UniswapV3Swapper.sol [Line: 52](../tests/contract-playground/src/uniswap/UniswapV3Swapper.sol#L52) + + ```solidity + ExactInputSingleParams memory exactInputSingleParams = ExactInputSingleParams( + ``` + +- Found in src/uniswap/UniswapV3Swapper.sol [Line: 55](../tests/contract-playground/src/uniswap/UniswapV3Swapper.sol#L55) + + ```solidity + exactInputSingleParams = ExactInputSingleParams({ + ``` + +- Found in src/uniswap/UniswapV3Swapper.sol [Line: 66](../tests/contract-playground/src/uniswap/UniswapV3Swapper.sol#L66) + + ```solidity + ExactInputParams memory exactInputParams = ExactInputParams( + ``` + +- Found in src/uniswap/UniswapV3Swapper.sol [Line: 69](../tests/contract-playground/src/uniswap/UniswapV3Swapper.sol#L69) + + ```solidity + exactInputParams = ExactInputParams({ + ``` + +- Found in src/uniswap/UniswapV3Swapper.sol [Line: 77](../tests/contract-playground/src/uniswap/UniswapV3Swapper.sol#L77) + + ```solidity + ExactOutputSingleParams memory exactOutputSingleParams = ExactOutputSingleParams( + ``` + +- Found in src/uniswap/UniswapV3Swapper.sol [Line: 80](../tests/contract-playground/src/uniswap/UniswapV3Swapper.sol#L80) + + ```solidity + exactOutputSingleParams = ExactOutputSingleParams({ + ``` + +- Found in src/uniswap/UniswapV3Swapper.sol [Line: 91](../tests/contract-playground/src/uniswap/UniswapV3Swapper.sol#L91) + + ```solidity + ExactOutputParams memory exactOutputParams = ExactOutputParams( + ``` + +- Found in src/uniswap/UniswapV3Swapper.sol [Line: 94](../tests/contract-playground/src/uniswap/UniswapV3Swapper.sol#L94) + + ```solidity + exactOutputParams = ExactOutputParams({ + ``` + +
+ + + +## L-14: Using `ERC721::_mint()` can be dangerous Using `ERC721::_mint()` can mint ERC721 tokens to addresses which don't support ERC721 tokens. Use `_safeMint()` instead of `_mint()` for ERC721. @@ -4238,7 +4238,7 @@ Using `ERC721::_mint()` can mint ERC721 tokens to addresses which don't support -## L-14: PUSH0 is not supported by all chains +## L-15: PUSH0 is not supported by all chains 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. @@ -4507,7 +4507,7 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai -## L-15: Modifiers invoked only once can be shoe-horned into the function +## L-16: Modifiers invoked only once can be shoe-horned into the function @@ -4602,7 +4602,7 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai -## L-16: Empty Block +## L-17: Empty Block Consider removing empty blocks. @@ -4817,7 +4817,7 @@ Consider removing empty blocks. -## L-17: Large literal values multiples of 10000 can be replaced with scientific notation +## L-18: Large literal values multiples of 10000 can be replaced with scientific notation Use `e` notation, for example: `1e18`, instead of its full numeric value. @@ -4990,7 +4990,7 @@ Use `e` notation, for example: `1e18`, instead of its full numeric value. -## L-18: Internal functions called only once can be inlined +## L-19: Internal functions called only once can be inlined Instead of separating the logic into a separate function, consider inlining the logic into the calling function. This can reduce the number of function calls and improve readability. @@ -5115,7 +5115,7 @@ Instead of separating the logic into a separate function, consider inlining the -## L-19: Contract still has TODOs +## L-20: Contract still has TODOs Contract contains comments with TODOS @@ -5138,7 +5138,7 @@ Contract contains comments with TODOS -## L-20: Inconsistency in declaring uint256/uint (or) int256/int variables within a contract. Use explicit size declarations (uint256 or int256). +## L-21: Inconsistency in declaring uint256/uint (or) int256/int variables within a contract. Use explicit size declarations (uint256 or int256). Consider keeping the naming convention consistent in a given contract. Explicit size declarations are preferred (uint256, int256) over implicit ones (uint, int) to avoid confusion. @@ -5323,7 +5323,7 @@ Consider keeping the naming convention consistent in a given contract. Explicit -## L-21: Unused Custom Error +## L-22: Unused Custom Error it is recommended that the definition be removed when custom error is unused @@ -5352,7 +5352,7 @@ it is recommended that the definition be removed when custom error is unused -## L-22: Loop contains `require`/`revert` statements +## L-23: Loop contains `require`/`revert` statements Avoid `require` / `revert` statements in a loop because a single bad item can cause the whole transaction to fail. It's better to forgive on fail and return failed elements post processing of the loop @@ -5375,7 +5375,7 @@ Avoid `require` / `revert` statements in a loop because a single bad item can ca -## L-23: Incorrect Order of Division and Multiplication +## L-24: Incorrect Order of Division and Multiplication Division operations followed directly by multiplication operations can lead to precision loss due to the way integer arithmetic is handled in Solidity. @@ -5410,7 +5410,7 @@ Division operations followed directly by multiplication operations can lead to p -## L-24: Redundant statements have no effect. +## L-25: Redundant statements have no effect. Remove the redundant statements because no code will be generated and it just congests the codebase. @@ -5457,7 +5457,7 @@ Remove the redundant statements because no code will be generated and it just co -## L-25: Public variables of a contract read in an external context (using `this`). +## L-26: Public variables of a contract read in an external context (using `this`). The contract reads it's own variable using `this` which adds an unnecessary STATICCALL. Remove `this` and access the variable like storage. @@ -5492,7 +5492,7 @@ The contract reads it's own variable using `this` which adds an unnecessary STAT -## L-26: Potentially unused `private` / `internal` state variables found. +## L-27: Potentially unused `private` / `internal` state variables found. State variable appears to be unused. No analysis has been performed to see if any inilne assembly references it. So if that's not the case, consider removing this unused variable. @@ -5665,7 +5665,7 @@ State variable appears to be unused. No analysis has been performed to see if an -## L-27: Functions declared `pure` / `view` but contains assembly +## L-28: Functions declared `pure` / `view` but contains assembly If the assembly code contains bugs or unintended side effects, it can lead to incorrect results or vulnerabilities, which are hard to debug and resolve, especially when the function is meant to be simple and predictable. @@ -5694,7 +5694,7 @@ If the assembly code contains bugs or unintended side effects, it can lead to in -## L-28: Boolean equality is not required. +## L-29: Boolean equality is not required. If `x` is a boolean, there is no need to do `if(x == true)` or `if(x == false)`. Just use `if(x)` and `if(!x)` respectively. @@ -5729,7 +5729,7 @@ If `x` is a boolean, there is no need to do `if(x == true)` or `if(x == false)`. -## L-29: Local variable shadows state variables in the contract hirearchy +## L-30: Local variable shadows state variables in the contract hirearchy Rename the local variables that shadow another component. @@ -5788,7 +5788,7 @@ Rename the local variables that shadow another component. -## L-30: Uninitialized local variables. +## L-31: Uninitialized local variables. Initialize all the variables. If a variable is meant to be initialized to zero, explicitly set it to zero to improve code readability. @@ -5889,7 +5889,7 @@ Initialize all the variables. If a variable is meant to be initialized to zero, -## L-31: Return Bomb +## L-32: Return Bomb A low level callee may consume all callers gas unexpectedly. Avoid unlimited implicit decoding of returndata on calls to unchecked addresses. You can limit the gas by passing a gas limit as an option to the call. For example, `unknownAdress.call{gas: gasLimitHere}("calldata")` That would act as a safety net from OOG errors. @@ -5907,7 +5907,7 @@ A low level callee may consume all callers gas unexpectedly. Avoid unlimited imp -## L-32: Function initializing state. +## L-33: Function initializing state. Detects the immediate initialization of state variables through function calls that are not pure/constant, or that use non-constant state variable. Remove any initialization of state variables via non-constant state variables or function calls. If variables must be set upon contract deployment, locate initialization in the constructor instead. @@ -5936,7 +5936,7 @@ Detects the immediate initialization of state variables through function calls t -## L-33: Dead Code +## L-34: Dead Code Functions that are not used. Consider removing them. @@ -6019,7 +6019,7 @@ Functions that are not used. Consider removing them. -## L-34: Loop condition contains `state_variable.length` that could be cached outside. +## L-35: Loop condition contains `state_variable.length` that could be cached outside. Cache the lengths of storage arrays if they are used and not modified in for loops. @@ -6048,7 +6048,7 @@ Cache the lengths of storage arrays if they are used and not modified in for loo -## L-35: Incorrect use of `assert()` +## L-36: Incorrect use of `assert()` Argument to `assert()` modifies the state. Use `require` for invariants modifying state. @@ -6065,7 +6065,7 @@ Argument to `assert()` modifies the state. Use `require` for invariants modifyin -## L-36: Costly operations inside loops. +## L-37: Costly operations inside loops. Invoking `SSTORE`operations in loops may lead to Out-of-gas errors. Use a local variable to hold the loop computation result. @@ -6160,7 +6160,7 @@ Invoking `SSTORE`operations in loops may lead to Out-of-gas errors. Use a local -## L-37: Builtin Symbol Shadowing +## L-38: Builtin Symbol Shadowing Name clashes with a built-in-symbol. Consider renaming it. @@ -6195,7 +6195,7 @@ Name clashes with a built-in-symbol. Consider renaming it. -## L-38: Void constructor +## L-39: Void constructor Call to a constructor that is not implemented. @@ -6212,7 +6212,7 @@ Call to a constructor that is not implemented. -## L-39: Potentially missing inheritance for contract. +## L-40: Potentially missing inheritance for contract. There is an interface / abstract contract that is potentially missing (not included in) the inheritance of this contract. @@ -6244,7 +6244,7 @@ There is an interface / abstract contract that is potentially missing (not inclu -## L-40: Unused Imports +## L-41: Unused Imports Redundant import statement. Consider removing it. @@ -6273,7 +6273,7 @@ Redundant import statement. Consider removing it. -## L-41: Function pointers used in constructors. +## L-42: Function pointers used in constructors. solc versions below 0.5.9 contain a compiler bug leading to unexpected behavior when calling uninitialized function pointers in constructors. It is recommended to not use function pointers in constructors. @@ -6290,7 +6290,7 @@ solc versions below 0.5.9 contain a compiler bug leading to unexpected behavior -## L-42: State variable could be declared constant +## L-43: State variable could be declared constant State variables that are not updated following deployment should be declared constant to save gas. Add the `constant` attribute to state variables that never change. @@ -6571,7 +6571,7 @@ State variables that are not updated following deployment should be declared con -## L-43: State variable changes but no event is emitted. +## L-44: State variable changes but no event is emitted. State variable changes in this function but no event is emitted. @@ -7182,7 +7182,7 @@ State variable changes in this function but no event is emitted. -## L-44: State variable could be declared immutable +## L-45: State variable could be declared immutable State variables that are should be declared immutable to save gas. Add the `immutable` attribute to state variables that are only changed in the constructor @@ -7379,7 +7379,7 @@ State variables that are should be declared immutable to save gas. Add the `immu -## L-45: Modifier has multiple placeholders. +## L-46: Modifier has multiple placeholders. Design the modifier to only contain 1 placeholder statement. If it's not possible, split the logic into multiple modifiers. @@ -7396,7 +7396,7 @@ Design the modifier to only contain 1 placeholder statement. If it's not possibl -## L-46: Return value of the function call is not checked. +## L-47: Return value of the function call is not checked. Function returns a value but it is ignored. diff --git a/reports/report.sarif b/reports/report.sarif index 66210539..9498df3e 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -66,202 +66,6 @@ }, "ruleId": "hash-collision-due-to-abi-encode-packed" }, - { - "level": "warning", - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/Trump.sol" - }, - "region": { - "byteLength": 190, - "byteOffset": 9949 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/uniswap/UniswapV2Swapper.sol" - }, - "region": { - "byteLength": 83, - "byteOffset": 670 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/uniswap/UniswapV2Swapper.sol" - }, - "region": { - "byteLength": 83, - "byteOffset": 763 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/uniswap/UniswapV2Swapper.sol" - }, - "region": { - "byteLength": 70, - "byteOffset": 856 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/uniswap/UniswapV2Swapper.sol" - }, - "region": { - "byteLength": 80, - "byteOffset": 936 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/uniswap/UniswapV2Swapper.sol" - }, - "region": { - "byteLength": 80, - "byteOffset": 1026 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/uniswap/UniswapV2Swapper.sol" - }, - "region": { - "byteLength": 112, - "byteOffset": 1278 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/uniswap/UniswapV2Swapper.sol" - }, - "region": { - "byteLength": 99, - "byteOffset": 1400 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/uniswap/UniswapV2Swapper.sol" - }, - "region": { - "byteLength": 109, - "byteOffset": 1509 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/uniswap/UniswapV3Swapper.sol" - }, - "region": { - "byteLength": 143, - "byteOffset": 1115 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/uniswap/UniswapV3Swapper.sol" - }, - "region": { - "byteLength": 321, - "byteOffset": 1293 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/uniswap/UniswapV3Swapper.sol" - }, - "region": { - "byteLength": 131, - "byteOffset": 1668 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/uniswap/UniswapV3Swapper.sol" - }, - "region": { - "byteLength": 236, - "byteOffset": 1828 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/uniswap/UniswapV3Swapper.sol" - }, - "region": { - "byteLength": 144, - "byteOffset": 2132 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/uniswap/UniswapV3Swapper.sol" - }, - "region": { - "byteLength": 322, - "byteOffset": 2312 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/uniswap/UniswapV3Swapper.sol" - }, - "region": { - "byteLength": 132, - "byteOffset": 2690 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/uniswap/UniswapV3Swapper.sol" - }, - "region": { - "byteLength": 237, - "byteOffset": 2852 - } - } - } - ], - "message": { - "text": "In the PoS model, proposers know well in advance if they will propose one or consecutive blocks ahead of time. In such a scenario, a malicious validator can hold back the transaction and execute it at a more favourable block number.Consider allowing function caller to specify swap deadline input parameter." - }, - "ruleId": "block-timestamp-is-weak-deadline" - }, { "level": "warning", "locations": [ @@ -6746,6 +6550,202 @@ }, "ruleId": "non-reentrant-is-not-before-others" }, + { + "level": "note", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/Trump.sol" + }, + "region": { + "byteLength": 190, + "byteOffset": 9949 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/uniswap/UniswapV2Swapper.sol" + }, + "region": { + "byteLength": 83, + "byteOffset": 670 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/uniswap/UniswapV2Swapper.sol" + }, + "region": { + "byteLength": 83, + "byteOffset": 763 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/uniswap/UniswapV2Swapper.sol" + }, + "region": { + "byteLength": 70, + "byteOffset": 856 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/uniswap/UniswapV2Swapper.sol" + }, + "region": { + "byteLength": 80, + "byteOffset": 936 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/uniswap/UniswapV2Swapper.sol" + }, + "region": { + "byteLength": 80, + "byteOffset": 1026 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/uniswap/UniswapV2Swapper.sol" + }, + "region": { + "byteLength": 112, + "byteOffset": 1278 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/uniswap/UniswapV2Swapper.sol" + }, + "region": { + "byteLength": 99, + "byteOffset": 1400 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/uniswap/UniswapV2Swapper.sol" + }, + "region": { + "byteLength": 109, + "byteOffset": 1509 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/uniswap/UniswapV3Swapper.sol" + }, + "region": { + "byteLength": 143, + "byteOffset": 1115 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/uniswap/UniswapV3Swapper.sol" + }, + "region": { + "byteLength": 321, + "byteOffset": 1293 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/uniswap/UniswapV3Swapper.sol" + }, + "region": { + "byteLength": 131, + "byteOffset": 1668 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/uniswap/UniswapV3Swapper.sol" + }, + "region": { + "byteLength": 236, + "byteOffset": 1828 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/uniswap/UniswapV3Swapper.sol" + }, + "region": { + "byteLength": 144, + "byteOffset": 2132 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/uniswap/UniswapV3Swapper.sol" + }, + "region": { + "byteLength": 322, + "byteOffset": 2312 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/uniswap/UniswapV3Swapper.sol" + }, + "region": { + "byteLength": 132, + "byteOffset": 2690 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/uniswap/UniswapV3Swapper.sol" + }, + "region": { + "byteLength": 237, + "byteOffset": 2852 + } + } + } + ], + "message": { + "text": "In the PoS model, proposers know well in advance if they will propose one or consecutive blocks ahead of time. In such a scenario, a malicious validator can hold back the transaction and execute it at a more favourable block number.Consider allowing function caller to specify swap deadline input parameter." + }, + "ruleId": "block-timestamp-is-weak-deadline" + }, { "level": "note", "locations": [ diff --git a/reports/uniswap_profile.md b/reports/uniswap_profile.md index e0eff425..b1d88998 100644 --- a/reports/uniswap_profile.md +++ b/reports/uniswap_profile.md @@ -7,12 +7,11 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [Files Summary](#files-summary) - [Files Details](#files-details) - [Issue Summary](#issue-summary) -- [High Issues](#high-issues) - - [H-1: Using `block.timestamp` for swap deadline offers no protection](#h-1-using-blocktimestamp-for-swap-deadline-offers-no-protection) - [Low Issues](#low-issues) - [L-1: Missing checks for `address(0)` when assigning values to address state variables](#l-1-missing-checks-for-address0-when-assigning-values-to-address-state-variables) - - [L-2: PUSH0 is not supported by all chains](#l-2-push0-is-not-supported-by-all-chains) - - [L-3: State variable could be declared immutable](#l-3-state-variable-could-be-declared-immutable) + - [L-2: Using `block.timestamp` for swap deadline offers no protection](#l-2-using-blocktimestamp-for-swap-deadline-offers-no-protection) + - [L-3: PUSH0 is not supported by all chains](#l-3-push0-is-not-supported-by-all-chains) + - [L-4: State variable could be declared immutable](#l-4-state-variable-could-be-declared-immutable) # Summary @@ -38,13 +37,30 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 1 | -| Low | 3 | +| High | 0 | +| Low | 4 | -# High Issues +# Low Issues + +## L-1: Missing checks for `address(0)` when assigning values to address state variables + +Check for `address(0)` when assigning values to address state variables. + +
1 Found Instances -## H-1: Using `block.timestamp` for swap deadline offers no protection + +- Found in src/uniswap/UniswapV2Swapper.sol [Line: 11](../tests/contract-playground/src/uniswap/UniswapV2Swapper.sol#L11) + + ```solidity + s_router = router; + ``` + +
+ + + +## L-2: Using `block.timestamp` for swap deadline offers no protection In the PoS model, proposers know well in advance if they will propose one or consecutive blocks ahead of time. In such a scenario, a malicious validator can hold back the transaction and execute it at a more favourable block number.Consider allowing function caller to specify swap deadline input parameter. @@ -151,26 +167,7 @@ In the PoS model, proposers know well in advance if they will propose one or con -# Low Issues - -## L-1: Missing checks for `address(0)` when assigning values to address state variables - -Check for `address(0)` when assigning values to address state variables. - -
1 Found Instances - - -- Found in src/uniswap/UniswapV2Swapper.sol [Line: 11](../tests/contract-playground/src/uniswap/UniswapV2Swapper.sol#L11) - - ```solidity - s_router = router; - ``` - -
- - - -## L-2: PUSH0 is not supported by all chains +## L-3: PUSH0 is not supported by all chains 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. @@ -193,7 +190,7 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai -## L-3: State variable could be declared immutable +## L-4: State variable could be declared immutable State variables that are should be declared immutable to save gas. Add the `immutable` attribute to state variables that are only changed in the constructor