Panics as error-handling

[code423n4]

Handle

nascent

Vulnerability details

[H-04] Panics as error-handling

Severity: High
Likelihood: Medium

The use of .unwrap(), expect(), and assert!() should be limited to tests, compile-time assertions (e.g. consts), 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(), and assert!() with a Result::Err(). Where necessary, change function signatures to return a Result<> and handle error cases at the highest level of execution, even if this means intentionally throwing away a result:

let _res : Result<_, _> = func_that_can_fail();

or, ideally, logging any error condition

if let Err(e) = func_that_canfail() {
    log!("error");
}

[jkilpatr]

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.

[albertchon]

Yeah this needs a specific example, otherwise it's not relevant

[loudoguno]

reopening as per judges assessment as "primary issue" on findings sheet