Incorrect Error Handling Would Make Debugging Difficult

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-02-hydradx//blob/main/HydraDX-node/pallets/omnipool/src/traits.rs#L171

Vulnerability details

Proof of Concept

The code uses map_err(|_| ())? in several places to handle errors. This approach discards the original error and replaces it with (). While this is a simple way to handle errors, it can make debugging difficult if an error occurs, as there will be no information about what the original error was.

For Example

let oracle_price = ExternalOracle::get_price(asset_a, asset_b).map_err(|_| ())?;

Impact

In this line, if get_price returns an error, map_err converts it into (), and then the ? operator immediately returns this from the current function. This means that if get_price fails, we'll know that an error occurred, but we won't know why. and it would make it difficult to track errors while debugging

Tools Used

Manual

Recommended Mitigation Steps

A better approach would be to define a custom error type that can hold different kinds of errors. This way, we can convert the original error into our custom error type, preserving the original error information. Here's an example:

#[derive(Debug)]
enum MyError {
    OracleError(OracleError),
    // other kinds of errors...
}

let oracle_price = ExternalOracle::get_price(asset_a, asset_b).map_err(MyError::OracleError)?;

In this version, if get_price fails, its error is wrapped in the MyError::OracleError variant, and then this is returned from the current function. This preserves the original error information, making it easier to debug what went wrong.

Assessed type

Error

[0xRobocop]

Consider QA

[c4-pre-sort]

0xRobocop marked the issue as insufficient quality report

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)

[c4-judge]

OpenCoreCH marked the issue as grade-b