diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index b9e0f6a34..8d175e62a 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -6,19 +6,19 @@ use crate::{ context::workspace_context::WorkspaceContext, detect::{ high::{ - ArbitraryTransferFromDetector, BlockTimestampDeadlineDetector, - DelegateCallInLoopDetector, + ArbitraryTransferFromDetector, AvoidAbiEncodePackedDetector, + BlockTimestampDeadlineDetector, DelegateCallInLoopDetector, + UnprotectedInitializerDetector, }, low::{ - AvoidAbiEncodePackedDetector, CentralizationRiskDetector, - ConstantsInsteadOfLiteralsDetector, ContractsWithTodosDetector, - DeprecatedOZFunctionsDetector, EcrecoverDetector, EmptyBlockDetector, - InconsistentTypeNamesDetector, LargeLiteralValueDetector, + CentralizationRiskDetector, ConstantsInsteadOfLiteralsDetector, + ContractsWithTodosDetector, DeprecatedOZFunctionsDetector, EcrecoverDetector, + EmptyBlockDetector, InconsistentTypeNamesDetector, LargeLiteralValueDetector, NonReentrantBeforeOthersDetector, PushZeroOpcodeDetector, RequireWithStringDetector, - SolmateSafeTransferLibDetector, UnindexedEventsDetector, - UnprotectedInitializerDetector, UnsafeERC20FunctionsDetector, UnsafeERC721MintDetector, - UnspecificSolidityPragmaDetector, UselessInternalFunctionDetector, - UselessModifierDetector, UselessPublicFunctionDetector, ZeroAddressCheckDetector, + SolmateSafeTransferLibDetector, UnindexedEventsDetector, UnsafeERC20FunctionsDetector, + UnsafeERC721MintDetector, UnspecificSolidityPragmaDetector, + UselessInternalFunctionDetector, UselessModifierDetector, + UselessPublicFunctionDetector, ZeroAddressCheckDetector, }, }, }; diff --git a/aderyn_core/src/detect/low/avoid_abi_encode_packed.rs b/aderyn_core/src/detect/high/avoid_abi_encode_packed.rs similarity index 98% rename from aderyn_core/src/detect/low/avoid_abi_encode_packed.rs rename to aderyn_core/src/detect/high/avoid_abi_encode_packed.rs index 40c78ddf2..37a9ca325 100644 --- a/aderyn_core/src/detect/low/avoid_abi_encode_packed.rs +++ b/aderyn_core/src/detect/high/avoid_abi_encode_packed.rs @@ -58,7 +58,7 @@ impl IssueDetector for AvoidAbiEncodePackedDetector { } fn severity(&self) -> IssueSeverity { - IssueSeverity::Low + IssueSeverity::High } fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { @@ -92,7 +92,7 @@ mod avoid_abi_encode_packed_tests { // assert that the severity is low assert_eq!( detector.severity(), - crate::detect::detector::IssueSeverity::Low + crate::detect::detector::IssueSeverity::High ); // assert that the title is correct assert_eq!( diff --git a/aderyn_core/src/detect/high/mod.rs b/aderyn_core/src/detect/high/mod.rs index 4b277ca63..841bcdccf 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -1,7 +1,11 @@ pub(crate) mod arbitrary_transfer_from; +pub(crate) mod avoid_abi_encode_packed; pub(crate) mod block_timestamp_deadline; pub(crate) mod delegate_call_in_loop; +pub(crate) mod unprotected_init_function; pub use arbitrary_transfer_from::ArbitraryTransferFromDetector; +pub use avoid_abi_encode_packed::AvoidAbiEncodePackedDetector; pub use block_timestamp_deadline::BlockTimestampDeadlineDetector; pub use delegate_call_in_loop::DelegateCallInLoopDetector; +pub use unprotected_init_function::UnprotectedInitializerDetector; diff --git a/aderyn_core/src/detect/low/unprotected_init_function.rs b/aderyn_core/src/detect/high/unprotected_init_function.rs similarity index 98% rename from aderyn_core/src/detect/low/unprotected_init_function.rs rename to aderyn_core/src/detect/high/unprotected_init_function.rs index 06e245dfb..cb13b4ff6 100644 --- a/aderyn_core/src/detect/low/unprotected_init_function.rs +++ b/aderyn_core/src/detect/high/unprotected_init_function.rs @@ -43,7 +43,7 @@ impl IssueDetector for UnprotectedInitializerDetector { } fn severity(&self) -> IssueSeverity { - IssueSeverity::Low + IssueSeverity::High } fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { diff --git a/aderyn_core/src/detect/low/mod.rs b/aderyn_core/src/detect/low/mod.rs index 9e7180edc..cf67f295c 100644 --- a/aderyn_core/src/detect/low/mod.rs +++ b/aderyn_core/src/detect/low/mod.rs @@ -1,4 +1,3 @@ -pub(crate) mod avoid_abi_encode_packed; pub(crate) mod centralization_risk; pub(crate) mod constants_instead_of_literals; pub(crate) mod contracts_with_todos; @@ -12,7 +11,6 @@ pub(crate) mod push_0_opcode; pub(crate) mod require_with_string; pub(crate) mod solmate_safe_transfer_lib; pub(crate) mod unindexed_events; -pub(crate) mod unprotected_init_function; pub(crate) mod unsafe_erc20_functions; pub(crate) mod unsafe_oz_erc721_mint; pub(crate) mod unspecific_solidity_pragma; @@ -21,7 +19,6 @@ pub(crate) mod useless_modifier; pub(crate) mod useless_public_function; pub(crate) mod zero_address_check; -pub use avoid_abi_encode_packed::AvoidAbiEncodePackedDetector; pub use centralization_risk::CentralizationRiskDetector; pub use constants_instead_of_literals::ConstantsInsteadOfLiteralsDetector; pub use contracts_with_todos::ContractsWithTodosDetector; @@ -35,7 +32,6 @@ pub use push_0_opcode::PushZeroOpcodeDetector; pub use require_with_string::RequireWithStringDetector; pub use solmate_safe_transfer_lib::SolmateSafeTransferLibDetector; pub use unindexed_events::UnindexedEventsDetector; -pub use unprotected_init_function::UnprotectedInitializerDetector; pub use unsafe_erc20_functions::UnsafeERC20FunctionsDetector; pub use unsafe_oz_erc721_mint::UnsafeERC721MintDetector; pub use unspecific_solidity_pragma::UnspecificSolidityPragmaDetector; diff --git a/judgeops/current/report.judge.md b/judgeops/current/report.judge.md index ccaaf750a..5505ce530 100644 --- a/judgeops/current/report.judge.md +++ b/judgeops/current/report.judge.md @@ -9,31 +9,31 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [Issue Summary](#issue-summary) - [High Issues](#high-issues) - [H-1: Using `delegatecall` in loop](#h-1-using-delegatecall-in-loop) - - [H-2: Using `block.timestamp` for swap deadline offers no protection](#h-2-using-blocktimestamp-for-swap-deadline-offers-no-protection) - - [H-3: Arbitrary `from` passed to `transferFrom` (or `safeTransferFrom`)](#h-3-arbitrary-from-passed-to-transferfrom-or-safetransferfrom) + - [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) - [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) - - [L-3: `abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()`](#l-3-abiencodepacked-should-not-be-used-with-dynamic-types-when-passing-the-result-to-a-hash-function-such-as-keccak256) - - [L-4: `ecrecover` is susceptible to signature malleability](#l-4-ecrecover-is-susceptible-to-signature-malleability) - - [L-5: Deprecated OpenZeppelin functions should not be used](#l-5-deprecated-openzeppelin-functions-should-not-be-used) - - [L-6: Unsafe ERC20 Operations should not be used](#l-6-unsafe-erc20-operations-should-not-be-used) - - [L-7: Solidity pragma should be specific, not wide](#l-7-solidity-pragma-should-be-specific-not-wide) - - [L-8: Missing checks for `address(0)` when assigning values to address state variables](#l-8-missing-checks-for-address0-when-assigning-values-to-address-state-variables) - - [L-9: `public` functions not used internally could be marked `external`](#l-9-public-functions-not-used-internally-could-be-marked-external) - - [L-10: Define and use `constant` variables instead of using literals](#l-10-define-and-use-constant-variables-instead-of-using-literals) - - [L-11: Event is missing `indexed` fields](#l-11-event-is-missing-indexed-fields) - - [L-12: Empty `require()` / `revert()` statements](#l-12-empty-require--revert-statements) - - [L-13: The `nonReentrant` `modifier` should occur before all other modifiers](#l-13-the-nonreentrant-modifier-should-occur-before-all-other-modifiers) - - [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](#l-21-inconsistency-in-declaring-uint256uint-or-int256int-variables-within-a-contract) - - [L-22: Unprotected initializer](#l-22-unprotected-initializer) + - [L-3: `ecrecover` is susceptible to signature malleability](#l-3-ecrecover-is-susceptible-to-signature-malleability) + - [L-4: Deprecated OpenZeppelin functions should not be used](#l-4-deprecated-openzeppelin-functions-should-not-be-used) + - [L-5: Unsafe ERC20 Operations should not be used](#l-5-unsafe-erc20-operations-should-not-be-used) + - [L-6: Solidity pragma should be specific, not wide](#l-6-solidity-pragma-should-be-specific-not-wide) + - [L-7: Missing checks for `address(0)` when assigning values to address state variables](#l-7-missing-checks-for-address0-when-assigning-values-to-address-state-variables) + - [L-8: `public` functions not used internally could be marked `external`](#l-8-public-functions-not-used-internally-could-be-marked-external) + - [L-9: Define and use `constant` variables instead of using literals](#l-9-define-and-use-constant-variables-instead-of-using-literals) + - [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](#l-20-inconsistency-in-declaring-uint256uint-or-int256int-variables-within-a-contract) # Summary @@ -89,8 +89,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 3 | -| Low | 22 | +| High | 5 | +| Low | 20 | ## Detectors Used @@ -101,7 +101,7 @@ centralization-risk:Low solmate-safe-transfer-lib:Low -avoid-abi-encode-packed:Low +avoid-abi-encode-packed:High ecrecover:Low @@ -143,7 +143,7 @@ contract-with-todos:Low inconsistent-type-names:Low -unprotected-initializer:Low +unprotected-initializer:High # High Issues @@ -161,7 +161,34 @@ When calling `delegatecall` the same `msg.value` amount will be accredited multi -## H-2: Using `block.timestamp` for swap deadline offers no protection +## H-2: `abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()` + +Use `abi.encode()` instead which will pad items to 32 bytes, which will [prevent hash collisions](https://docs.soliditylang.org/en/v0.8.13/abi-spec.html#non-standard-packed-mode) (e.g. `abi.encodePacked(0x123,0x456)` => `0x123456` => `abi.encodePacked(0x1,0x23456)`, but `abi.encode(0x123,0x456)` => `0x0...1230...456`). Unless there is a compelling reason, `abi.encode` should be preferred. If there is only one argument to `abi.encodePacked()` it can often be cast to `bytes()` or `bytes32()` [instead](https://ethereum.stackexchange.com/questions/30912/how-to-compare-strings-in-solidity#answer-82739). +If all arguments are strings and or bytes, `bytes.concat()` should be used instead. + +### Responsible : avoid-abi-encode-packed + +- Found in src/KeccakContract.sol [Line: 18](../../tests/contract-playground/src/KeccakContract.sol#L18) + + ```solidity + return keccak256(abi.encodePacked(a, b)); + ``` + +- Found in src/KeccakContract.sol [Line: 22](../../tests/contract-playground/src/KeccakContract.sol#L22) + + ```solidity + return keccak256(abi.encodePacked(a, b)); + ``` + +- Found in src/KeccakContract.sol [Line: 26](../../tests/contract-playground/src/KeccakContract.sol#L26) + + ```solidity + return keccak256(abi.encodePacked(a, b)); + ``` + + + +## 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. @@ -271,7 +298,7 @@ In the PoS model, proposers know well in advance if they will propose one or con -## H-3: Arbitrary `from` passed to `transferFrom` (or `safeTransferFrom`) +## H-4: 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. @@ -315,6 +342,20 @@ Passing an arbitrary `from` address to `transferFrom` (or `safeTransferFrom`) ca +## H-5: Unprotected initializer + +Consider protecting the initializer functions with modifiers. + +### Responsible : unprotected-initializer + +- Found in src/UnprotectedInitialize.sol [Line: 35](../../tests/contract-playground/src/UnprotectedInitialize.sol#L35) + + ```solidity + function initializeWithoutModifierOrRevert() external { + ``` + + + # Low Issues ## L-1: Centralization Risk for trusted owners @@ -396,34 +437,7 @@ https://github.com/transmissions11/solmate/blob/main/src/utils/SafeTransferLib.s -## L-3: `abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()` - -Use `abi.encode()` instead which will pad items to 32 bytes, which will [prevent hash collisions](https://docs.soliditylang.org/en/v0.8.13/abi-spec.html#non-standard-packed-mode) (e.g. `abi.encodePacked(0x123,0x456)` => `0x123456` => `abi.encodePacked(0x1,0x23456)`, but `abi.encode(0x123,0x456)` => `0x0...1230...456`). Unless there is a compelling reason, `abi.encode` should be preferred. If there is only one argument to `abi.encodePacked()` it can often be cast to `bytes()` or `bytes32()` [instead](https://ethereum.stackexchange.com/questions/30912/how-to-compare-strings-in-solidity#answer-82739). -If all arguments are strings and or bytes, `bytes.concat()` should be used instead. - -### Responsible : avoid-abi-encode-packed - -- Found in src/KeccakContract.sol [Line: 18](../../tests/contract-playground/src/KeccakContract.sol#L18) - - ```solidity - return keccak256(abi.encodePacked(a, b)); - ``` - -- Found in src/KeccakContract.sol [Line: 22](../../tests/contract-playground/src/KeccakContract.sol#L22) - - ```solidity - return keccak256(abi.encodePacked(a, b)); - ``` - -- Found in src/KeccakContract.sol [Line: 26](../../tests/contract-playground/src/KeccakContract.sol#L26) - - ```solidity - return keccak256(abi.encodePacked(a, b)); - ``` - - - -## L-4: `ecrecover` is susceptible to signature malleability +## L-3: `ecrecover` is susceptible to signature malleability The `ecrecover` function is susceptible to signature malleability. This means that the same message can be signed in multiple ways, allowing an attacker to change the message signature without invalidating it. This can lead to unexpected behavior in smart contracts, such as the loss of funds or the ability to bypass access control. Consider using OpenZeppelin's ECDSA library instead of the built-in function. @@ -437,7 +451,7 @@ The `ecrecover` function is susceptible to signature malleability. This means th -## L-5: Deprecated OpenZeppelin functions should not be used +## L-4: Deprecated OpenZeppelin functions should not be used Openzeppelin has deprecated several functions and replaced with newer versions. Please consult https://docs.openzeppelin.com/ @@ -457,7 +471,7 @@ Openzeppelin has deprecated several functions and replaced with newer versions. -## L-6: Unsafe ERC20 Operations should not be used +## L-5: Unsafe ERC20 Operations should not be used ERC20 functions may not behave as expected. For example: return values are not always meaningful. It is recommended to use OpenZeppelin's SafeERC20 library. @@ -513,7 +527,7 @@ ERC20 functions may not behave as expected. For example: return values are not a -## L-7: Solidity pragma should be specific, not wide +## L-6: Solidity pragma should be specific, not wide 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;` @@ -569,7 +583,7 @@ Consider using a specific version of Solidity in your contracts instead of a wid -## L-8: Missing checks for `address(0)` when assigning values to address state variables +## L-7: Missing checks for `address(0)` when assigning values to address state variables Check for `address(0)` when assigning values to address state variables. @@ -613,7 +627,7 @@ Check for `address(0)` when assigning values to address state variables. -## L-9: `public` functions not used internally could be marked `external` +## L-8: `public` functions not used internally could be marked `external` Instead of marking a function as `public`, consider marking it as `external` if it is not used internally. @@ -717,7 +731,7 @@ Instead of marking a function as `public`, consider marking it as `external` if -## L-10: Define and use `constant` variables instead of using literals +## L-9: Define and use `constant` variables instead of using literals If the same constant literal value is used multiple times, create a constant state variable and reference it throughout the contract. @@ -779,7 +793,7 @@ If the same constant literal value is used multiple times, create a constant sta -## L-11: Event is missing `indexed` fields +## L-10: Event is missing `indexed` fields 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. @@ -805,7 +819,7 @@ Index event fields make the field more quickly accessible to off-chain tools tha -## L-12: Empty `require()` / `revert()` statements +## L-11: Empty `require()` / `revert()` statements Use descriptive reason strings or custom errors for revert paths. @@ -867,7 +881,7 @@ Use descriptive reason strings or custom errors for revert paths. -## L-13: The `nonReentrant` `modifier` should occur before all other modifiers +## L-12: The `nonReentrant` `modifier` should occur before all other modifiers This is a best-practice to protect against reentrancy in other modifiers. @@ -887,7 +901,7 @@ This is a best-practice to protect against reentrancy in other modifiers. -## L-14: Using `ERC721::_mint()` can be dangerous +## L-13: 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. @@ -901,7 +915,7 @@ Using `ERC721::_mint()` can mint ERC721 tokens to addresses which don't support -## L-15: PUSH0 is not supported by all chains +## L-14: 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. @@ -1041,7 +1055,7 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai -## L-16: Modifiers invoked only once can be shoe-horned into the function +## L-15: Modifiers invoked only once can be shoe-horned into the function @@ -1067,7 +1081,7 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai -## L-17: Empty Block +## L-16: Empty Block Consider removing empty blocks. @@ -1165,7 +1179,7 @@ Consider removing empty blocks. -## L-18: 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 Use `e` notation, for example: `1e18`, instead of its full numeric value. @@ -1293,7 +1307,7 @@ Use `e` notation, for example: `1e18`, instead of its full numeric value. -## L-19: Internal functions called only once can be inlined +## L-18: 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. @@ -1307,7 +1321,7 @@ Instead of separating the logic into a separate function, consider inlining the -## L-20: Contract still has TODOs +## L-19: Contract still has TODOs Contract contains comments with TODOS @@ -1327,7 +1341,7 @@ Contract contains comments with TODOS -## L-21: Inconsistency in declaring uint256/uint (or) int256/int variables within a contract +## L-20: Inconsistency in declaring uint256/uint (or) int256/int variables within a contract Consider keeping the naming convention consistent in a given contract @@ -1347,17 +1361,3 @@ Consider keeping the naming convention consistent in a given contract -## L-22: Unprotected initializer - -Consider protecting the initializer functions with modifiers. - -### Responsible : unprotected-initializer - -- Found in src/UnprotectedInitialize.sol [Line: 35](../../tests/contract-playground/src/UnprotectedInitialize.sol#L35) - - ```solidity - function initializeWithoutModifierOrRevert() external { - ``` - - - diff --git a/report.json b/report.json index d1b014ffb..9f24652ad 100644 --- a/report.json +++ b/report.json @@ -176,6 +176,28 @@ ], "title": "Using `delegatecall` in loop" }, + { + "description": "Use `abi.encode()` instead which will pad items to 32 bytes, which will [prevent hash collisions](https://docs.soliditylang.org/en/v0.8.13/abi-spec.html#non-standard-packed-mode) (e.g. `abi.encodePacked(0x123,0x456)` => `0x123456` => `abi.encodePacked(0x1,0x23456)`, but `abi.encode(0x123,0x456)` => `0x0...1230...456`). Unless there is a compelling reason, `abi.encode` should be preferred. If there is only one argument to `abi.encodePacked()` it can often be cast to `bytes()` or `bytes32()` [instead](https://ethereum.stackexchange.com/questions/30912/how-to-compare-strings-in-solidity#answer-82739).\nIf all arguments are strings and or bytes, `bytes.concat()` should be used instead.", + "detector_name": "avoid-abi-encode-packed", + "instances": [ + { + "contract_path": "src/KeccakContract.sol", + "line_no": 18, + "src": "587:16" + }, + { + "contract_path": "src/KeccakContract.sol", + "line_no": 22, + "src": "734:16" + }, + { + "contract_path": "src/KeccakContract.sol", + "line_no": 26, + "src": "887:16" + } + ], + "title": "`abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()`" + }, { "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-deadline", @@ -304,12 +326,24 @@ } ], "title": "Arbitrary `from` passed to `transferFrom` (or `safeTransferFrom`)" + }, + { + "description": "Consider protecting the initializer functions with modifiers.", + "detector_name": "unprotected-initializer", + "instances": [ + { + "contract_path": "src/UnprotectedInitialize.sol", + "line_no": 35, + "src": "820:33" + } + ], + "title": "Unprotected initializer" } ] }, "issue_count": { - "high": 3, - "low": 22 + "high": 5, + "low": 20 }, "low_issues": { "issues": [ @@ -377,28 +411,6 @@ ], "title": "Solmate's SafeTransferLib does not check for token contract's existence" }, - { - "description": "Use `abi.encode()` instead which will pad items to 32 bytes, which will [prevent hash collisions](https://docs.soliditylang.org/en/v0.8.13/abi-spec.html#non-standard-packed-mode) (e.g. `abi.encodePacked(0x123,0x456)` => `0x123456` => `abi.encodePacked(0x1,0x23456)`, but `abi.encode(0x123,0x456)` => `0x0...1230...456`). Unless there is a compelling reason, `abi.encode` should be preferred. If there is only one argument to `abi.encodePacked()` it can often be cast to `bytes()` or `bytes32()` [instead](https://ethereum.stackexchange.com/questions/30912/how-to-compare-strings-in-solidity#answer-82739).\nIf all arguments are strings and or bytes, `bytes.concat()` should be used instead.", - "detector_name": "avoid-abi-encode-packed", - "instances": [ - { - "contract_path": "src/KeccakContract.sol", - "line_no": 18, - "src": "587:16" - }, - { - "contract_path": "src/KeccakContract.sol", - "line_no": 22, - "src": "734:16" - }, - { - "contract_path": "src/KeccakContract.sol", - "line_no": 26, - "src": "887:16" - } - ], - "title": "`abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()`" - }, { "description": "The `ecrecover` function is susceptible to signature malleability. This means that the same message can be signed in multiple ways, allowing an attacker to change the message signature without invalidating it. This can lead to unexpected behavior in smart contracts, such as the loss of funds or the ability to bypass access control. Consider using OpenZeppelin's ECDSA library instead of the built-in function.", "detector_name": "ecrecover", @@ -1179,18 +1191,6 @@ } ], "title": "Inconsistency in declaring uint256/uint (or) int256/int variables within a contract" - }, - { - "description": "Consider protecting the initializer functions with modifiers.", - "detector_name": "unprotected-initializer", - "instances": [ - { - "contract_path": "src/UnprotectedInitialize.sol", - "line_no": 35, - "src": "820:33" - } - ], - "title": "Unprotected initializer" } ] } diff --git a/report.md b/report.md index 273734b89..01c209081 100644 --- a/report.md +++ b/report.md @@ -9,31 +9,31 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [Issue Summary](#issue-summary) - [High Issues](#high-issues) - [H-1: Using `delegatecall` in loop](#h-1-using-delegatecall-in-loop) - - [H-2: Using `block.timestamp` for swap deadline offers no protection](#h-2-using-blocktimestamp-for-swap-deadline-offers-no-protection) - - [H-3: Arbitrary `from` passed to `transferFrom` (or `safeTransferFrom`)](#h-3-arbitrary-from-passed-to-transferfrom-or-safetransferfrom) + - [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) - [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) - - [L-3: `abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()`](#l-3-abiencodepacked-should-not-be-used-with-dynamic-types-when-passing-the-result-to-a-hash-function-such-as-keccak256) - - [L-4: `ecrecover` is susceptible to signature malleability](#l-4-ecrecover-is-susceptible-to-signature-malleability) - - [L-5: Deprecated OpenZeppelin functions should not be used](#l-5-deprecated-openzeppelin-functions-should-not-be-used) - - [L-6: Unsafe ERC20 Operations should not be used](#l-6-unsafe-erc20-operations-should-not-be-used) - - [L-7: Solidity pragma should be specific, not wide](#l-7-solidity-pragma-should-be-specific-not-wide) - - [L-8: Missing checks for `address(0)` when assigning values to address state variables](#l-8-missing-checks-for-address0-when-assigning-values-to-address-state-variables) - - [L-9: `public` functions not used internally could be marked `external`](#l-9-public-functions-not-used-internally-could-be-marked-external) - - [L-10: Define and use `constant` variables instead of using literals](#l-10-define-and-use-constant-variables-instead-of-using-literals) - - [L-11: Event is missing `indexed` fields](#l-11-event-is-missing-indexed-fields) - - [L-12: Empty `require()` / `revert()` statements](#l-12-empty-require--revert-statements) - - [L-13: The `nonReentrant` `modifier` should occur before all other modifiers](#l-13-the-nonreentrant-modifier-should-occur-before-all-other-modifiers) - - [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](#l-21-inconsistency-in-declaring-uint256uint-or-int256int-variables-within-a-contract) - - [L-22: Unprotected initializer](#l-22-unprotected-initializer) + - [L-3: `ecrecover` is susceptible to signature malleability](#l-3-ecrecover-is-susceptible-to-signature-malleability) + - [L-4: Deprecated OpenZeppelin functions should not be used](#l-4-deprecated-openzeppelin-functions-should-not-be-used) + - [L-5: Unsafe ERC20 Operations should not be used](#l-5-unsafe-erc20-operations-should-not-be-used) + - [L-6: Solidity pragma should be specific, not wide](#l-6-solidity-pragma-should-be-specific-not-wide) + - [L-7: Missing checks for `address(0)` when assigning values to address state variables](#l-7-missing-checks-for-address0-when-assigning-values-to-address-state-variables) + - [L-8: `public` functions not used internally could be marked `external`](#l-8-public-functions-not-used-internally-could-be-marked-external) + - [L-9: Define and use `constant` variables instead of using literals](#l-9-define-and-use-constant-variables-instead-of-using-literals) + - [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](#l-20-inconsistency-in-declaring-uint256uint-or-int256int-variables-within-a-contract) # Summary @@ -89,8 +89,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 3 | -| Low | 22 | +| High | 5 | +| Low | 20 | # High Issues @@ -107,7 +107,32 @@ When calling `delegatecall` the same `msg.value` amount will be accredited multi -## H-2: Using `block.timestamp` for swap deadline offers no protection +## H-2: `abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()` + +Use `abi.encode()` instead which will pad items to 32 bytes, which will [prevent hash collisions](https://docs.soliditylang.org/en/v0.8.13/abi-spec.html#non-standard-packed-mode) (e.g. `abi.encodePacked(0x123,0x456)` => `0x123456` => `abi.encodePacked(0x1,0x23456)`, but `abi.encode(0x123,0x456)` => `0x0...1230...456`). Unless there is a compelling reason, `abi.encode` should be preferred. If there is only one argument to `abi.encodePacked()` it can often be cast to `bytes()` or `bytes32()` [instead](https://ethereum.stackexchange.com/questions/30912/how-to-compare-strings-in-solidity#answer-82739). +If all arguments are strings and or bytes, `bytes.concat()` should be used instead. + +- Found in src/KeccakContract.sol [Line: 18](tests/contract-playground/src/KeccakContract.sol#L18) + + ```solidity + return keccak256(abi.encodePacked(a, b)); + ``` + +- Found in src/KeccakContract.sol [Line: 22](tests/contract-playground/src/KeccakContract.sol#L22) + + ```solidity + return keccak256(abi.encodePacked(a, b)); + ``` + +- Found in src/KeccakContract.sol [Line: 26](tests/contract-playground/src/KeccakContract.sol#L26) + + ```solidity + return keccak256(abi.encodePacked(a, b)); + ``` + + + +## 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. @@ -215,7 +240,7 @@ In the PoS model, proposers know well in advance if they will propose one or con -## H-3: Arbitrary `from` passed to `transferFrom` (or `safeTransferFrom`) +## H-4: 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. @@ -257,6 +282,18 @@ Passing an arbitrary `from` address to `transferFrom` (or `safeTransferFrom`) ca +## H-5: Unprotected initializer + +Consider protecting the initializer functions with modifiers. + +- Found in src/UnprotectedInitialize.sol [Line: 35](tests/contract-playground/src/UnprotectedInitialize.sol#L35) + + ```solidity + function initializeWithoutModifierOrRevert() external { + ``` + + + # Low Issues ## L-1: Centralization Risk for trusted owners @@ -334,32 +371,7 @@ https://github.com/transmissions11/solmate/blob/main/src/utils/SafeTransferLib.s -## L-3: `abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()` - -Use `abi.encode()` instead which will pad items to 32 bytes, which will [prevent hash collisions](https://docs.soliditylang.org/en/v0.8.13/abi-spec.html#non-standard-packed-mode) (e.g. `abi.encodePacked(0x123,0x456)` => `0x123456` => `abi.encodePacked(0x1,0x23456)`, but `abi.encode(0x123,0x456)` => `0x0...1230...456`). Unless there is a compelling reason, `abi.encode` should be preferred. If there is only one argument to `abi.encodePacked()` it can often be cast to `bytes()` or `bytes32()` [instead](https://ethereum.stackexchange.com/questions/30912/how-to-compare-strings-in-solidity#answer-82739). -If all arguments are strings and or bytes, `bytes.concat()` should be used instead. - -- Found in src/KeccakContract.sol [Line: 18](tests/contract-playground/src/KeccakContract.sol#L18) - - ```solidity - return keccak256(abi.encodePacked(a, b)); - ``` - -- Found in src/KeccakContract.sol [Line: 22](tests/contract-playground/src/KeccakContract.sol#L22) - - ```solidity - return keccak256(abi.encodePacked(a, b)); - ``` - -- Found in src/KeccakContract.sol [Line: 26](tests/contract-playground/src/KeccakContract.sol#L26) - - ```solidity - return keccak256(abi.encodePacked(a, b)); - ``` - - - -## L-4: `ecrecover` is susceptible to signature malleability +## L-3: `ecrecover` is susceptible to signature malleability The `ecrecover` function is susceptible to signature malleability. This means that the same message can be signed in multiple ways, allowing an attacker to change the message signature without invalidating it. This can lead to unexpected behavior in smart contracts, such as the loss of funds or the ability to bypass access control. Consider using OpenZeppelin's ECDSA library instead of the built-in function. @@ -371,7 +383,7 @@ The `ecrecover` function is susceptible to signature malleability. This means th -## L-5: Deprecated OpenZeppelin functions should not be used +## L-4: Deprecated OpenZeppelin functions should not be used Openzeppelin has deprecated several functions and replaced with newer versions. Please consult https://docs.openzeppelin.com/ @@ -389,7 +401,7 @@ Openzeppelin has deprecated several functions and replaced with newer versions. -## L-6: Unsafe ERC20 Operations should not be used +## L-5: Unsafe ERC20 Operations should not be used ERC20 functions may not behave as expected. For example: return values are not always meaningful. It is recommended to use OpenZeppelin's SafeERC20 library. @@ -443,7 +455,7 @@ ERC20 functions may not behave as expected. For example: return values are not a -## L-7: Solidity pragma should be specific, not wide +## L-6: Solidity pragma should be specific, not wide 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;` @@ -497,7 +509,7 @@ Consider using a specific version of Solidity in your contracts instead of a wid -## L-8: Missing checks for `address(0)` when assigning values to address state variables +## L-7: Missing checks for `address(0)` when assigning values to address state variables Check for `address(0)` when assigning values to address state variables. @@ -539,7 +551,7 @@ Check for `address(0)` when assigning values to address state variables. -## L-9: `public` functions not used internally could be marked `external` +## L-8: `public` functions not used internally could be marked `external` Instead of marking a function as `public`, consider marking it as `external` if it is not used internally. @@ -641,7 +653,7 @@ Instead of marking a function as `public`, consider marking it as `external` if -## L-10: Define and use `constant` variables instead of using literals +## L-9: Define and use `constant` variables instead of using literals If the same constant literal value is used multiple times, create a constant state variable and reference it throughout the contract. @@ -701,7 +713,7 @@ If the same constant literal value is used multiple times, create a constant sta -## L-11: Event is missing `indexed` fields +## L-10: Event is missing `indexed` fields 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. @@ -725,7 +737,7 @@ Index event fields make the field more quickly accessible to off-chain tools tha -## L-12: Empty `require()` / `revert()` statements +## L-11: Empty `require()` / `revert()` statements Use descriptive reason strings or custom errors for revert paths. @@ -785,7 +797,7 @@ Use descriptive reason strings or custom errors for revert paths. -## L-13: The `nonReentrant` `modifier` should occur before all other modifiers +## L-12: The `nonReentrant` `modifier` should occur before all other modifiers This is a best-practice to protect against reentrancy in other modifiers. @@ -803,7 +815,7 @@ This is a best-practice to protect against reentrancy in other modifiers. -## L-14: Using `ERC721::_mint()` can be dangerous +## L-13: 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. @@ -815,7 +827,7 @@ Using `ERC721::_mint()` can mint ERC721 tokens to addresses which don't support -## L-15: PUSH0 is not supported by all chains +## L-14: 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. @@ -953,7 +965,7 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai -## L-16: Modifiers invoked only once can be shoe-horned into the function +## L-15: Modifiers invoked only once can be shoe-horned into the function @@ -977,7 +989,7 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai -## L-17: Empty Block +## L-16: Empty Block Consider removing empty blocks. @@ -1073,7 +1085,7 @@ Consider removing empty blocks. -## L-18: 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 Use `e` notation, for example: `1e18`, instead of its full numeric value. @@ -1199,7 +1211,7 @@ Use `e` notation, for example: `1e18`, instead of its full numeric value. -## L-19: Internal functions called only once can be inlined +## L-18: 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. @@ -1211,7 +1223,7 @@ Instead of separating the logic into a separate function, consider inlining the -## L-20: Contract still has TODOs +## L-19: Contract still has TODOs Contract contains comments with TODOS @@ -1229,7 +1241,7 @@ Contract contains comments with TODOS -## L-21: Inconsistency in declaring uint256/uint (or) int256/int variables within a contract +## L-20: Inconsistency in declaring uint256/uint (or) int256/int variables within a contract Consider keeping the naming convention consistent in a given contract @@ -1247,15 +1259,3 @@ Consider keeping the naming convention consistent in a given contract -## L-22: Unprotected initializer - -Consider protecting the initializer functions with modifiers. - -- Found in src/UnprotectedInitialize.sol [Line: 35](tests/contract-playground/src/UnprotectedInitialize.sol#L35) - - ```solidity - function initializeWithoutModifierOrRevert() external { - ``` - - - diff --git a/report.sample_profile.md b/report.sample_profile.md index 273734b89..01c209081 100644 --- a/report.sample_profile.md +++ b/report.sample_profile.md @@ -9,31 +9,31 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [Issue Summary](#issue-summary) - [High Issues](#high-issues) - [H-1: Using `delegatecall` in loop](#h-1-using-delegatecall-in-loop) - - [H-2: Using `block.timestamp` for swap deadline offers no protection](#h-2-using-blocktimestamp-for-swap-deadline-offers-no-protection) - - [H-3: Arbitrary `from` passed to `transferFrom` (or `safeTransferFrom`)](#h-3-arbitrary-from-passed-to-transferfrom-or-safetransferfrom) + - [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) - [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) - - [L-3: `abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()`](#l-3-abiencodepacked-should-not-be-used-with-dynamic-types-when-passing-the-result-to-a-hash-function-such-as-keccak256) - - [L-4: `ecrecover` is susceptible to signature malleability](#l-4-ecrecover-is-susceptible-to-signature-malleability) - - [L-5: Deprecated OpenZeppelin functions should not be used](#l-5-deprecated-openzeppelin-functions-should-not-be-used) - - [L-6: Unsafe ERC20 Operations should not be used](#l-6-unsafe-erc20-operations-should-not-be-used) - - [L-7: Solidity pragma should be specific, not wide](#l-7-solidity-pragma-should-be-specific-not-wide) - - [L-8: Missing checks for `address(0)` when assigning values to address state variables](#l-8-missing-checks-for-address0-when-assigning-values-to-address-state-variables) - - [L-9: `public` functions not used internally could be marked `external`](#l-9-public-functions-not-used-internally-could-be-marked-external) - - [L-10: Define and use `constant` variables instead of using literals](#l-10-define-and-use-constant-variables-instead-of-using-literals) - - [L-11: Event is missing `indexed` fields](#l-11-event-is-missing-indexed-fields) - - [L-12: Empty `require()` / `revert()` statements](#l-12-empty-require--revert-statements) - - [L-13: The `nonReentrant` `modifier` should occur before all other modifiers](#l-13-the-nonreentrant-modifier-should-occur-before-all-other-modifiers) - - [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](#l-21-inconsistency-in-declaring-uint256uint-or-int256int-variables-within-a-contract) - - [L-22: Unprotected initializer](#l-22-unprotected-initializer) + - [L-3: `ecrecover` is susceptible to signature malleability](#l-3-ecrecover-is-susceptible-to-signature-malleability) + - [L-4: Deprecated OpenZeppelin functions should not be used](#l-4-deprecated-openzeppelin-functions-should-not-be-used) + - [L-5: Unsafe ERC20 Operations should not be used](#l-5-unsafe-erc20-operations-should-not-be-used) + - [L-6: Solidity pragma should be specific, not wide](#l-6-solidity-pragma-should-be-specific-not-wide) + - [L-7: Missing checks for `address(0)` when assigning values to address state variables](#l-7-missing-checks-for-address0-when-assigning-values-to-address-state-variables) + - [L-8: `public` functions not used internally could be marked `external`](#l-8-public-functions-not-used-internally-could-be-marked-external) + - [L-9: Define and use `constant` variables instead of using literals](#l-9-define-and-use-constant-variables-instead-of-using-literals) + - [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](#l-20-inconsistency-in-declaring-uint256uint-or-int256int-variables-within-a-contract) # Summary @@ -89,8 +89,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 3 | -| Low | 22 | +| High | 5 | +| Low | 20 | # High Issues @@ -107,7 +107,32 @@ When calling `delegatecall` the same `msg.value` amount will be accredited multi -## H-2: Using `block.timestamp` for swap deadline offers no protection +## H-2: `abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()` + +Use `abi.encode()` instead which will pad items to 32 bytes, which will [prevent hash collisions](https://docs.soliditylang.org/en/v0.8.13/abi-spec.html#non-standard-packed-mode) (e.g. `abi.encodePacked(0x123,0x456)` => `0x123456` => `abi.encodePacked(0x1,0x23456)`, but `abi.encode(0x123,0x456)` => `0x0...1230...456`). Unless there is a compelling reason, `abi.encode` should be preferred. If there is only one argument to `abi.encodePacked()` it can often be cast to `bytes()` or `bytes32()` [instead](https://ethereum.stackexchange.com/questions/30912/how-to-compare-strings-in-solidity#answer-82739). +If all arguments are strings and or bytes, `bytes.concat()` should be used instead. + +- Found in src/KeccakContract.sol [Line: 18](tests/contract-playground/src/KeccakContract.sol#L18) + + ```solidity + return keccak256(abi.encodePacked(a, b)); + ``` + +- Found in src/KeccakContract.sol [Line: 22](tests/contract-playground/src/KeccakContract.sol#L22) + + ```solidity + return keccak256(abi.encodePacked(a, b)); + ``` + +- Found in src/KeccakContract.sol [Line: 26](tests/contract-playground/src/KeccakContract.sol#L26) + + ```solidity + return keccak256(abi.encodePacked(a, b)); + ``` + + + +## 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. @@ -215,7 +240,7 @@ In the PoS model, proposers know well in advance if they will propose one or con -## H-3: Arbitrary `from` passed to `transferFrom` (or `safeTransferFrom`) +## H-4: 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. @@ -257,6 +282,18 @@ Passing an arbitrary `from` address to `transferFrom` (or `safeTransferFrom`) ca +## H-5: Unprotected initializer + +Consider protecting the initializer functions with modifiers. + +- Found in src/UnprotectedInitialize.sol [Line: 35](tests/contract-playground/src/UnprotectedInitialize.sol#L35) + + ```solidity + function initializeWithoutModifierOrRevert() external { + ``` + + + # Low Issues ## L-1: Centralization Risk for trusted owners @@ -334,32 +371,7 @@ https://github.com/transmissions11/solmate/blob/main/src/utils/SafeTransferLib.s -## L-3: `abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()` - -Use `abi.encode()` instead which will pad items to 32 bytes, which will [prevent hash collisions](https://docs.soliditylang.org/en/v0.8.13/abi-spec.html#non-standard-packed-mode) (e.g. `abi.encodePacked(0x123,0x456)` => `0x123456` => `abi.encodePacked(0x1,0x23456)`, but `abi.encode(0x123,0x456)` => `0x0...1230...456`). Unless there is a compelling reason, `abi.encode` should be preferred. If there is only one argument to `abi.encodePacked()` it can often be cast to `bytes()` or `bytes32()` [instead](https://ethereum.stackexchange.com/questions/30912/how-to-compare-strings-in-solidity#answer-82739). -If all arguments are strings and or bytes, `bytes.concat()` should be used instead. - -- Found in src/KeccakContract.sol [Line: 18](tests/contract-playground/src/KeccakContract.sol#L18) - - ```solidity - return keccak256(abi.encodePacked(a, b)); - ``` - -- Found in src/KeccakContract.sol [Line: 22](tests/contract-playground/src/KeccakContract.sol#L22) - - ```solidity - return keccak256(abi.encodePacked(a, b)); - ``` - -- Found in src/KeccakContract.sol [Line: 26](tests/contract-playground/src/KeccakContract.sol#L26) - - ```solidity - return keccak256(abi.encodePacked(a, b)); - ``` - - - -## L-4: `ecrecover` is susceptible to signature malleability +## L-3: `ecrecover` is susceptible to signature malleability The `ecrecover` function is susceptible to signature malleability. This means that the same message can be signed in multiple ways, allowing an attacker to change the message signature without invalidating it. This can lead to unexpected behavior in smart contracts, such as the loss of funds or the ability to bypass access control. Consider using OpenZeppelin's ECDSA library instead of the built-in function. @@ -371,7 +383,7 @@ The `ecrecover` function is susceptible to signature malleability. This means th -## L-5: Deprecated OpenZeppelin functions should not be used +## L-4: Deprecated OpenZeppelin functions should not be used Openzeppelin has deprecated several functions and replaced with newer versions. Please consult https://docs.openzeppelin.com/ @@ -389,7 +401,7 @@ Openzeppelin has deprecated several functions and replaced with newer versions. -## L-6: Unsafe ERC20 Operations should not be used +## L-5: Unsafe ERC20 Operations should not be used ERC20 functions may not behave as expected. For example: return values are not always meaningful. It is recommended to use OpenZeppelin's SafeERC20 library. @@ -443,7 +455,7 @@ ERC20 functions may not behave as expected. For example: return values are not a -## L-7: Solidity pragma should be specific, not wide +## L-6: Solidity pragma should be specific, not wide 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;` @@ -497,7 +509,7 @@ Consider using a specific version of Solidity in your contracts instead of a wid -## L-8: Missing checks for `address(0)` when assigning values to address state variables +## L-7: Missing checks for `address(0)` when assigning values to address state variables Check for `address(0)` when assigning values to address state variables. @@ -539,7 +551,7 @@ Check for `address(0)` when assigning values to address state variables. -## L-9: `public` functions not used internally could be marked `external` +## L-8: `public` functions not used internally could be marked `external` Instead of marking a function as `public`, consider marking it as `external` if it is not used internally. @@ -641,7 +653,7 @@ Instead of marking a function as `public`, consider marking it as `external` if -## L-10: Define and use `constant` variables instead of using literals +## L-9: Define and use `constant` variables instead of using literals If the same constant literal value is used multiple times, create a constant state variable and reference it throughout the contract. @@ -701,7 +713,7 @@ If the same constant literal value is used multiple times, create a constant sta -## L-11: Event is missing `indexed` fields +## L-10: Event is missing `indexed` fields 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. @@ -725,7 +737,7 @@ Index event fields make the field more quickly accessible to off-chain tools tha -## L-12: Empty `require()` / `revert()` statements +## L-11: Empty `require()` / `revert()` statements Use descriptive reason strings or custom errors for revert paths. @@ -785,7 +797,7 @@ Use descriptive reason strings or custom errors for revert paths. -## L-13: The `nonReentrant` `modifier` should occur before all other modifiers +## L-12: The `nonReentrant` `modifier` should occur before all other modifiers This is a best-practice to protect against reentrancy in other modifiers. @@ -803,7 +815,7 @@ This is a best-practice to protect against reentrancy in other modifiers. -## L-14: Using `ERC721::_mint()` can be dangerous +## L-13: 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. @@ -815,7 +827,7 @@ Using `ERC721::_mint()` can mint ERC721 tokens to addresses which don't support -## L-15: PUSH0 is not supported by all chains +## L-14: 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. @@ -953,7 +965,7 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai -## L-16: Modifiers invoked only once can be shoe-horned into the function +## L-15: Modifiers invoked only once can be shoe-horned into the function @@ -977,7 +989,7 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai -## L-17: Empty Block +## L-16: Empty Block Consider removing empty blocks. @@ -1073,7 +1085,7 @@ Consider removing empty blocks. -## L-18: 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 Use `e` notation, for example: `1e18`, instead of its full numeric value. @@ -1199,7 +1211,7 @@ Use `e` notation, for example: `1e18`, instead of its full numeric value. -## L-19: Internal functions called only once can be inlined +## L-18: 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. @@ -1211,7 +1223,7 @@ Instead of separating the logic into a separate function, consider inlining the -## L-20: Contract still has TODOs +## L-19: Contract still has TODOs Contract contains comments with TODOS @@ -1229,7 +1241,7 @@ Contract contains comments with TODOS -## L-21: Inconsistency in declaring uint256/uint (or) int256/int variables within a contract +## L-20: Inconsistency in declaring uint256/uint (or) int256/int variables within a contract Consider keeping the naming convention consistent in a given contract @@ -1247,15 +1259,3 @@ Consider keeping the naming convention consistent in a given contract -## L-22: Unprotected initializer - -Consider protecting the initializer functions with modifiers. - -- Found in src/UnprotectedInitialize.sol [Line: 35](tests/contract-playground/src/UnprotectedInitialize.sol#L35) - - ```solidity - function initializeWithoutModifierOrRevert() external { - ``` - - -