Open code423n4 opened 2 years ago
Without a clear exploit path this is a style issue. Noting that panic halts rust programs is hardly a useful report, building a program with no use of panic will not ensure it works.
Yeah this needs a specific example, otherwise it's not relevant
reopening as per judges assessment as "primary issue" on findings sheet
Handle
nascent
Vulnerability details
[H-04] Panics as error-handling
Severity: High Likelihood: Medium
The use of
.unwrap()
,expect()
, andassert!()
should be limited to tests, compile-time assertions (e.g.const
s), and configuration checks. Panicks are at the thread level, so stopping one thread unexpectedly could cause undefined behavior in others. This can become a system-wide vulnerability when the panic can occur from reading the state of a contract due to it affecting all running daemons. Additionally, some of these unwraps, expects and asserts occur based off contract log output. Given how resyncing works, its possible for these panics to persist across process lifetimes (i.e. spin-up, crash, spin-up, crash...) resulting in a patch being required before the bridge returns to an operational state.Recommendation
Wherever possible, replace instances of
unwrap()
,expect()
, andassert!()
with aResult::Err()
. Where necessary, change function signatures to return aResult<>
and handle error cases at the highest level of execution, even if this means intentionally throwing away a result:or, ideally, logging any error condition