Skip to content

Commit

Permalink
Update felt252 detector (#44)
Browse files Browse the repository at this point in the history 
* update name/difficulty for felt252 detector

* update readme

* update insta snapshot

* fixed clippy errors
  • Loading branch information
@technovision99
technovision99 authored Sep 18, 2023
1 parent 8dc26ae commit 87d455a
Show file tree
Hide file tree
Showing 7 changed files with 67 additions and 70 deletions.
2 changes: 1 addition & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 8 additions & 11 deletions src/compilation/utils/felt252_serde.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,17 +127,14 @@ impl Felt252Serde for StatementIdx {

/// A set of all the supported long generic ids.
static SERDE_SUPPORTED_LONG_IDS: Lazy<OrderedHashSet<&'static str>> = Lazy::new(|| {
OrderedHashSet::from_iter(
[
StorageAddressFromBaseAndOffsetLibfunc::STR_ID,
ContractAddressTryFromFelt252Libfunc::STR_ID,
StorageBaseAddressFromFelt252Libfunc::STR_ID,
StorageAddressTryFromFelt252Trait::STR_ID,
Secp256GetPointFromXLibfunc::<Secp256k1>::STR_ID,
Secp256GetPointFromXLibfunc::<Secp256r1>::STR_ID,
]
.into_iter(),
)
OrderedHashSet::from_iter([
StorageAddressFromBaseAndOffsetLibfunc::STR_ID,
ContractAddressTryFromFelt252Libfunc::STR_ID,
StorageBaseAddressFromFelt252Libfunc::STR_ID,
StorageAddressTryFromFelt252Trait::STR_ID,
Secp256GetPointFromXLibfunc::<Secp256k1>::STR_ID,
Secp256GetPointFromXLibfunc::<Secp256r1>::STR_ID,
])
});
/// A mapping of all the long names when fixing them from the hashed keccak representation.
static LONG_NAME_FIX: Lazy<UnorderedHashMap<BigUint, &'static str>> = Lazy::new(|| {
Expand Down
10 changes: 5 additions & 5 deletions src/detectors/felt252_overflow.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,19 @@ 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 {
Confidence::Medium
}

fn impact(&self) -> Impact {
Impact::High
Impact::Medium
}

fn run(&self, core: &CoreUnit) -> Vec<Result> {
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand Down
12 changes: 6 additions & 6 deletions tests/snapshots/integration_tests__detectors@dead_code.cairo.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
60 changes: 30 additions & 30 deletions tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
28 changes: 14 additions & 14 deletions tests/snapshots/integration_tests__detectors@unchecked_l1_handler_from.cairo.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
]
6 changes: 3 additions & 3 deletions tests/snapshots/integration_tests__detectors@unused_return.cairo.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 87d455a

Please sign in to comment.