From 87d455af25a94eb58fb84fd163b82cae7537dcfe Mon Sep 17 00:00:00 2001 From: technovision99 <25871300+technovision99@users.noreply.github.com> Date: Mon, 18 Sep 2023 02:02:05 -0700 Subject: [PATCH] Update felt252 detector (#44) * update name/difficulty for felt252 detector * update readme * update insta snapshot * fixed clippy errors --- README.md | 2 +- src/compilation/utils/felt252_serde.rs | 19 +++--- src/detectors/felt252_overflow.rs | 10 ++-- ...tion_tests__detectors@dead_code.cairo.snap | 12 ++-- ...sts__detectors@felt252_overflow.cairo.snap | 60 +++++++++---------- ...ctors@unchecked_l1_handler_from.cairo.snap | 28 ++++----- ..._tests__detectors@unused_return.cairo.snap | 6 +- 7 files changed, 67 insertions(+), 70 deletions(-) diff --git a/README.md b/README.md index 25aeb8c..62e2d6b 100644 --- a/README.md +++ b/README.md @@ -70,7 +70,7 @@ Num | Detector | What it Detects | Impact | Confidence | Cairo --- | --- | --- | --- | --- | --- 1 | `controlled-library-call` | Library calls with a user controlled class hash | High | Medium | 1 & 2 2 | `unchecked-l1-handler-from` | Detect L1 handlers without from address check | High | Medium | 1 & 2 -3 | `felt252-overflow` | Detect user controlled operations with felt252 type, which is not overflow safe | High | Medium | 1 & 2 +3 | `felt252-unsafe-arithmetic` | Detect user controlled operations with felt252 type, which is not overflow/underflow safe | Medium | Medium | 1 & 2 4 | `reentrancy` | Detect when a storage variable is read before an external call and written after | Medium | Medium | 1 & 2 5 | `read-only-reentrancy` | Detect when a view function read a storage variable written after an external call | Medium | Medium | 1 & 2 6 | `unused-events` | Events defined but not emitted | Medium | Medium | 1 & 2 diff --git a/src/compilation/utils/felt252_serde.rs b/src/compilation/utils/felt252_serde.rs index 632e595..caf2e29 100644 --- a/src/compilation/utils/felt252_serde.rs +++ b/src/compilation/utils/felt252_serde.rs @@ -127,17 +127,14 @@ impl Felt252Serde for StatementIdx { /// A set of all the supported long generic ids. static SERDE_SUPPORTED_LONG_IDS: Lazy> = Lazy::new(|| { - OrderedHashSet::from_iter( - [ - StorageAddressFromBaseAndOffsetLibfunc::STR_ID, - ContractAddressTryFromFelt252Libfunc::STR_ID, - StorageBaseAddressFromFelt252Libfunc::STR_ID, - StorageAddressTryFromFelt252Trait::STR_ID, - Secp256GetPointFromXLibfunc::::STR_ID, - Secp256GetPointFromXLibfunc::::STR_ID, - ] - .into_iter(), - ) + OrderedHashSet::from_iter([ + StorageAddressFromBaseAndOffsetLibfunc::STR_ID, + ContractAddressTryFromFelt252Libfunc::STR_ID, + StorageBaseAddressFromFelt252Libfunc::STR_ID, + StorageAddressTryFromFelt252Trait::STR_ID, + Secp256GetPointFromXLibfunc::::STR_ID, + Secp256GetPointFromXLibfunc::::STR_ID, + ]) }); /// A mapping of all the long names when fixing them from the hashed keccak representation. static LONG_NAME_FIX: Lazy> = Lazy::new(|| { diff --git a/src/detectors/felt252_overflow.rs b/src/detectors/felt252_overflow.rs index 666b353..a61c1f5 100644 --- a/src/detectors/felt252_overflow.rs +++ b/src/detectors/felt252_overflow.rs @@ -15,11 +15,11 @@ pub struct Felt252Overflow {} impl Detector for Felt252Overflow { fn name(&self) -> &str { - "felt252-overflow" + "felt252-unsafe-arithmetic" } fn description(&self) -> &str { - "Detect felt252 arithmetic overflow with user-controlled params" + "Detect felt252 arithmetic overflow/underflow with user-controlled params" } fn confidence(&self) -> Confidence { @@ -27,7 +27,7 @@ impl Detector for Felt252Overflow { } fn impact(&self) -> Impact { - Impact::High + Impact::Medium } fn run(&self, core: &CoreUnit) -> Vec { @@ -116,7 +116,7 @@ impl Felt252Overflow { // Not tainted by any parameter, but still uses felt252 type if tainted_by.is_empty() { let msg = format!( - "The function {} uses the felt252 operation {}, which is not overflow safe", + "The function {} uses the felt252 operation {}, which is not overflow/underflow safe", &name, libfunc ); results.push(Result { @@ -127,7 +127,7 @@ impl Felt252Overflow { }); } else { let msg = format!( - "The function {} uses the felt252 operation {} with the user-controlled parameters: {}, which is not overflow safe", + "The function {} uses the felt252 operation {} with the user-controlled parameters: {}, which is not overflow/underflow safe", &name, libfunc, taints diff --git a/tests/snapshots/integration_tests__detectors@dead_code.cairo.snap b/tests/snapshots/integration_tests__detectors@dead_code.cairo.snap index 06e90ef..9372330 100644 --- a/tests/snapshots/integration_tests__detectors@dead_code.cairo.snap +++ b/tests/snapshots/integration_tests__detectors@dead_code.cairo.snap @@ -5,16 +5,16 @@ input_file: tests/detectors/dead_code.cairo --- [ Result { - impact: High, - name: "felt252-overflow", + impact: Medium, + name: "felt252-unsafe-arithmetic", confidence: Medium, - message: "The function dead_code::dead_code::DeadCode::add_1 uses the felt252 operation felt252_add([0], [1]) -> ([2]), which is not overflow safe", + message: "The function dead_code::dead_code::DeadCode::add_1 uses the felt252 operation felt252_add([0], [1]) -> ([2]), which is not overflow/underflow safe", }, Result { - impact: High, - name: "felt252-overflow", + impact: Medium, + name: "felt252-unsafe-arithmetic", confidence: Medium, - message: "The function dead_code::dead_code::DeadCode::add_2 uses the felt252 operation felt252_add([0], [1]) -> ([2]), which is not overflow safe", + message: "The function dead_code::dead_code::DeadCode::add_2 uses the felt252 operation felt252_add([0], [1]) -> ([2]), which is not overflow/underflow safe", }, Result { impact: Low, diff --git a/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap b/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap index 11911a6..a717ff6 100644 --- a/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap +++ b/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap @@ -5,64 +5,64 @@ input_file: tests/detectors/felt252_overflow.cairo --- [ Result { - impact: High, - name: "felt252-overflow", + impact: Medium, + name: "felt252-unsafe-arithmetic", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add uses the felt252 operation felt252_add([0], [1]) -> ([6]) with the user-controlled parameters: [0],[1], which is not overflow safe", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add uses the felt252 operation felt252_add([0], [1]) -> ([6]) with the user-controlled parameters: [0],[1], which is not overflow/underflow safe", }, Result { - impact: High, - name: "felt252-overflow", + impact: Medium, + name: "felt252-unsafe-arithmetic", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt252 operation felt252_add([22], [3]) -> ([23]) with the user-controlled parameters: [3], which is not overflow safe", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt252 operation felt252_add([22], [3]) -> ([23]) with the user-controlled parameters: [3], which is not overflow/underflow safe", }, Result { - impact: High, - name: "felt252-overflow", + impact: Medium, + name: "felt252-unsafe-arithmetic", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt252 operation felt252_add([23], [4]) -> ([24]) with the user-controlled parameters: [23],[4], which is not overflow safe", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt252 operation felt252_add([23], [4]) -> ([24]) with the user-controlled parameters: [23],[4], which is not overflow/underflow safe", }, Result { - impact: High, - name: "felt252-overflow", + impact: Medium, + name: "felt252-unsafe-arithmetic", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_uncontrolled uses the felt252 operation felt252_sub([1], [2]) -> ([3]), which is not overflow safe", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_uncontrolled uses the felt252 operation felt252_sub([1], [2]) -> ([3]), which is not overflow/underflow safe", }, Result { - impact: High, - name: "felt252-overflow", + impact: Medium, + name: "felt252-unsafe-arithmetic", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt252 operation felt252_mul([6], [5]) -> ([10]) with the user-controlled parameters: [6],[5], which is not overflow safe", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt252 operation felt252_mul([6], [5]) -> ([10]) with the user-controlled parameters: [6],[5], which is not overflow/underflow safe", }, Result { - impact: High, - name: "felt252-overflow", + impact: Medium, + name: "felt252-unsafe-arithmetic", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt252 operation felt252_mul([7], [4]) -> ([6]) with the user-controlled parameters: [7],[4], which is not overflow safe", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt252 operation felt252_mul([7], [4]) -> ([6]) with the user-controlled parameters: [7],[4], which is not overflow/underflow safe", }, Result { - impact: High, - name: "felt252-overflow", + impact: Medium, + name: "felt252-unsafe-arithmetic", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt252 operation felt252_mul([9], [3]) -> ([8]) with the user-controlled parameters: [9],[3], which is not overflow safe", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt252 operation felt252_mul([9], [3]) -> ([8]) with the user-controlled parameters: [9],[3], which is not overflow/underflow safe", }, Result { - impact: High, - name: "felt252-overflow", + impact: Medium, + name: "felt252-unsafe-arithmetic", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_sub_controlled uses the felt252 operation felt252_sub([16], [3]) -> ([17]) with the user-controlled parameters: [3], which is not overflow safe", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_sub_controlled uses the felt252 operation felt252_sub([16], [3]) -> ([17]) with the user-controlled parameters: [3], which is not overflow/underflow safe", }, Result { - impact: High, - name: "felt252-overflow", + impact: Medium, + name: "felt252-unsafe-arithmetic", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_sub_uncontrolled uses the felt252 operation felt252_sub([3], [4]) -> ([5]), which is not overflow safe", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_sub_uncontrolled uses the felt252 operation felt252_sub([3], [4]) -> ([5]), which is not overflow/underflow safe", }, Result { - impact: High, - name: "felt252-overflow", + impact: Medium, + name: "felt252-unsafe-arithmetic", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::test_sub_assert uses the felt252 operation felt252_sub([1], [12]) -> ([13]) with the user-controlled parameters: [1], which is not overflow safe", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::test_sub_assert uses the felt252 operation felt252_sub([1], [12]) -> ([13]) with the user-controlled parameters: [1], which is not overflow/underflow safe", }, Result { impact: Medium, diff --git a/tests/snapshots/integration_tests__detectors@unchecked_l1_handler_from.cairo.snap b/tests/snapshots/integration_tests__detectors@unchecked_l1_handler_from.cairo.snap index 2506fd2..83e6d8d 100644 --- a/tests/snapshots/integration_tests__detectors@unchecked_l1_handler_from.cairo.snap +++ b/tests/snapshots/integration_tests__detectors@unchecked_l1_handler_from.cairo.snap @@ -6,32 +6,32 @@ input_file: tests/detectors/unchecked_l1_handler_from.cairo [ Result { impact: High, - name: "felt252-overflow", + name: "unchecked-l1-handler-from", confidence: Medium, - message: "The function unchecked_l1_handler_from::unchecked_l1_handler_from::UncheckedL1HandlerFrom::bad uses the felt252 operation felt252_add([1], [2]) -> ([3]) with the user-controlled parameters: [1], which is not overflow safe", + message: "The L1 handler function unchecked_l1_handler_from::unchecked_l1_handler_from::UncheckedL1HandlerFrom::bad does not check the L1 from address", }, Result { - impact: High, - name: "felt252-overflow", + impact: Medium, + name: "felt252-unsafe-arithmetic", confidence: Medium, - message: "The function unchecked_l1_handler_from::unchecked_l1_handler_from::UncheckedL1HandlerFrom::check_recursive uses the felt252 operation felt252_add([3], [50]) -> ([51]), which is not overflow safe", + message: "The function unchecked_l1_handler_from::unchecked_l1_handler_from::UncheckedL1HandlerFrom::bad uses the felt252 operation felt252_add([1], [2]) -> ([3]) with the user-controlled parameters: [1], which is not overflow/underflow safe", }, Result { - impact: High, - name: "felt252-overflow", + impact: Medium, + name: "felt252-unsafe-arithmetic", confidence: Medium, - message: "The function unchecked_l1_handler_from::unchecked_l1_handler_from::UncheckedL1HandlerFrom::check_recursive uses the felt252 operation felt252_sub([11], [9]) -> ([10]), which is not overflow safe", + message: "The function unchecked_l1_handler_from::unchecked_l1_handler_from::UncheckedL1HandlerFrom::check_recursive uses the felt252 operation felt252_add([3], [50]) -> ([51]), which is not overflow/underflow safe", }, Result { - impact: High, - name: "felt252-overflow", + impact: Medium, + name: "felt252-unsafe-arithmetic", confidence: Medium, - message: "The function unchecked_l1_handler_from::unchecked_l1_handler_from::UncheckedL1HandlerFrom::good3 uses the felt252 operation felt252_add([14], [15]) -> ([16]) with the user-controlled parameters: [14], which is not overflow safe", + message: "The function unchecked_l1_handler_from::unchecked_l1_handler_from::UncheckedL1HandlerFrom::check_recursive uses the felt252 operation felt252_sub([11], [9]) -> ([10]), which is not overflow/underflow safe", }, Result { - impact: High, - name: "unchecked-l1-handler-from", + impact: Medium, + name: "felt252-unsafe-arithmetic", confidence: Medium, - message: "The L1 handler function unchecked_l1_handler_from::unchecked_l1_handler_from::UncheckedL1HandlerFrom::bad does not check the L1 from address", + message: "The function unchecked_l1_handler_from::unchecked_l1_handler_from::UncheckedL1HandlerFrom::good3 uses the felt252 operation felt252_add([14], [15]) -> ([16]) with the user-controlled parameters: [14], which is not overflow/underflow safe", }, ] diff --git a/tests/snapshots/integration_tests__detectors@unused_return.cairo.snap b/tests/snapshots/integration_tests__detectors@unused_return.cairo.snap index 7f41c0c..86f83dd 100644 --- a/tests/snapshots/integration_tests__detectors@unused_return.cairo.snap +++ b/tests/snapshots/integration_tests__detectors@unused_return.cairo.snap @@ -5,10 +5,10 @@ input_file: tests/detectors/unused_return.cairo --- [ Result { - impact: High, - name: "felt252-overflow", + impact: Medium, + name: "felt252-unsafe-arithmetic", confidence: Medium, - message: "The function unused_return::unused_return::UnusedReturn::f_5 uses the felt252 operation felt252_mul([14], [15]) -> ([16]), which is not overflow safe", + message: "The function unused_return::unused_return::UnusedReturn::f_5 uses the felt252 operation felt252_mul([14], [15]) -> ([16]), which is not overflow/underflow safe", }, Result { impact: Medium,