Skip to content

Commit

Permalink
fix unused return bug (#53)
Browse files Browse the repository at this point in the history 
* check struct_deconstruct has fn ret values as input

* use bundled compiler to get CI to pass

* Fix test

---------

Co-authored-by: Simone <simone.monica@trailofbits.com>
  • Loading branch information
@technovision99 @smonicas
technovision99 and smonicas authored Jan 24, 2024
1 parent b0be7d8 commit b6100dd
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 15 deletions.
28 changes: 16 additions & 12 deletions src/detectors/unused_return.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ impl Detector for UnusedReturn {
for f in compilation_unit.functions_user_defined() {
for (i, stmt) in f.get_statements().iter().enumerate() {
if let SierraStatement::Invocation(invoc) = stmt {
// Get the return values from the function
let ret_vars = &invoc.branches[0].results;
// Get the concrete libfunc called
let libfunc = compilation_unit
.registry()
Expand Down Expand Up @@ -83,7 +85,8 @@ impl Detector for UnusedReturn {
.registry()
.get_libfunc(&invoc.libfunc_id)
.expect("Library function not found in the registry");

// Get the parameters to the instruction for the struct_deconstruct case
let args = &invoc.args;
// Immediate Drop instruction
if let CoreConcreteLibfunc::Drop(drop_libfunc) = libfunc {
let ty_dropped = compilation_unit
Expand Down Expand Up @@ -117,23 +120,24 @@ impl Detector for UnusedReturn {
.registry()
.get_libfunc(&invoc.libfunc_id)
.expect("Library function not found in the registry");

self.iterate_struct_deconstruct(
compilation_unit,
&mut results,
libfunc,
stmt_to_check,
stmt,
&f.name(),
return_variables,
);
// We want to make sure the struct_deconstruct corresponds to the function's return values, and not any misc. struct cleanup
if ret_vars.contains(&args[0]) {
self.iterate_struct_deconstruct(
compilation_unit,
&mut results,
libfunc,
stmt_to_check,
stmt,
&f.name(),
return_variables,
);
}
}
} else if let CoreConcreteLibfunc::Enum(
EnumConcreteLibfunc::Match(_),
) = libfunc
{
let return_variables = invoc.branches[0].results.len();

// Jump one statement which is a branch_align and the next one will be a struct_deconstruct
let stmt_to_check = &following_stmts[2..];
if let SierraStatement::Invocation(invoc) = &stmt_to_check[0] {
Expand Down
45 changes: 42 additions & 3 deletions tests/detectors/unused_return.cairo
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
use option::OptionTrait;

use serde::Serde;
#[starknet::contract]
mod UnusedReturn {
#[storage]
struct Storage {
value: felt252,
}
#[derive(Copy,Drop,Serde)]
struct TestStruct {
val1: felt252,
val2: felt252,
val3: felt252
}

#[external(v0)]
fn unused_return_1(ref self: ContractState, amount: felt252) {
Expand All @@ -31,7 +37,17 @@ mod UnusedReturn {

#[external(v0)]
fn unused_return_5(ref self: ContractState) {
let _a = f_5(ref self);
let _a = f_5(ref self);
}

#[external(v0)]
fn unused_return_6(ref self: ContractState, s: TestStruct) {
let _a = f_6(ref self, s.val1,s.val2);
}

#[external(v0)]
fn unused_return_7(ref self: ContractState, s: TestStruct) {
let _a = f_7(ref self, s);
}

#[external(v0)]
Expand All @@ -50,6 +66,18 @@ mod UnusedReturn {
f_5(ref self)
}

#[external(v0)]
fn no_report4(ref self: ContractState, s: TestStruct) -> felt252 {
let a = f_6(ref self, s.val1,s.val2);
s.val3 + a
}

#[external(v0)]
fn no_report5(ref self: ContractState, s: TestStruct) -> felt252 {
let a = f_6(ref self, s.val1,s.val2);
a
}

fn f_1(ref self: ContractState, amount: felt252) -> felt252 {
self.value.write(amount);
23
Expand All @@ -72,4 +100,15 @@ mod UnusedReturn {
a * 2
}

}
fn f_6(ref self:ContractState, amount1: felt252, amount2: felt252) -> felt252 {
let a = self.value.read();
let ret = amount1 * amount2;
ret + a
}

fn f_7(ref self:ContractState, s:TestStruct) -> felt252 {
let a = self.value.read();
s.val1 * s.val2 * s.val3 + a
}

}
48 changes: 48 additions & 0 deletions tests/snapshots/integration_tests__detectors@unused_return.cairo.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,42 @@ input_file: tests/detectors/unused_return.cairo
confidence: Medium,
message: "The function unused_return::unused_return::UnusedReturn::f_5 uses the felt252 operation felt252_mul([11], [12]) -> ([13]), which is not overflow/underflow safe",
},
Result {
impact: Medium,
name: "felt252-unsafe-arithmetic",
confidence: Medium,
message: "The function unused_return::unused_return::UnusedReturn::f_6 uses the felt252 operation felt252_add([14], [13]) -> ([15]) with the user-controlled parameters: [14], which is not overflow/underflow safe",
},
Result {
impact: Medium,
name: "felt252-unsafe-arithmetic",
confidence: Medium,
message: "The function unused_return::unused_return::UnusedReturn::f_6 uses the felt252 operation felt252_mul([3], [4]) -> ([14]) with the user-controlled parameters: [3],[4], which is not overflow/underflow safe",
},
Result {
impact: Medium,
name: "felt252-unsafe-arithmetic",
confidence: Medium,
message: "The function unused_return::unused_return::UnusedReturn::f_7 uses the felt252 operation felt252_add([17], [12]) -> ([18]) with the user-controlled parameters: [17], which is not overflow/underflow safe",
},
Result {
impact: Medium,
name: "felt252-unsafe-arithmetic",
confidence: Medium,
message: "The function unused_return::unused_return::UnusedReturn::f_7 uses the felt252 operation felt252_mul([13], [14]) -> ([16]) with the user-controlled parameters: [13],[14], which is not overflow/underflow safe",
},
Result {
impact: Medium,
name: "felt252-unsafe-arithmetic",
confidence: Medium,
message: "The function unused_return::unused_return::UnusedReturn::f_7 uses the felt252 operation felt252_mul([16], [15]) -> ([17]) with the user-controlled parameters: [16],[15], which is not overflow/underflow safe",
},
Result {
impact: Medium,
name: "felt252-unsafe-arithmetic",
confidence: Medium,
message: "The function unused_return::unused_return::UnusedReturn::no_report4 uses the felt252 operation felt252_add([6], [13]) -> ([14]) with the user-controlled parameters: [6],[13], which is not overflow/underflow safe",
},
Result {
impact: Medium,
name: "unused-return",
Expand Down Expand Up @@ -40,4 +76,16 @@ input_file: tests/detectors/unused_return.cairo
confidence: Medium,
message: "Return value unused for the function call function_call<user@unused_return::unused_return::UnusedReturn::f_5>([0], [1], [2]) -> ([3], [4], [5]) in unused_return::unused_return::UnusedReturn::unused_return_5",
},
Result {
impact: Medium,
name: "unused-return",
confidence: Medium,
message: "Return value unused for the function call function_call<user@unused_return::unused_return::UnusedReturn::f_6>([0], [1], [2], [4], [5]) -> ([7], [8], [9]) in unused_return::unused_return::UnusedReturn::unused_return_6",
},
Result {
impact: Medium,
name: "unused-return",
confidence: Medium,
message: "Return value unused for the function call function_call<user@unused_return::unused_return::UnusedReturn::f_7>([0], [1], [2], [3]) -> ([4], [5], [6]) in unused_return::unused_return::UnusedReturn::unused_return_7",
},
]

0 comments on commit b6100dd

Please sign in to comment.