Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[V-PHX-VUL-022] Incorrect decimals assertion #184

Closed
gangov opened this issue Jan 16, 2024 · 0 comments · Fixed by #197
Closed

[V-PHX-VUL-022] Incorrect decimals assertion #184

gangov opened this issue Jan 16, 2024 · 0 comments · Fixed by #197
Assignees
Labels
audit bug Something isn't working

Comments

@gangov
Copy link
Collaborator

gangov commented Jan 16, 2024

The function from_str in the DECIMAL crate is used to convert a string to a decimal type. To handle the fractional part of the number, the function has the following logic:

Code snippet from the from_str function. It handles the fractional part of a string number.

if let Some(fractional_part) = parts_iter.next() {
   let fractional: i128 = fractional_part.parse().expect("Error parsing fractional");
   let exp = Self::DECIMAL_PLACES - fractional_part.len() as i32;
   assert!(exp <= Self::DECIMAL_PLACES, "Too many fractional digits");
   let fractional_factor = 10i128.pow(exp as u32);
   atomics += fractional * fractional_factor;
}

As it can be noted, the assert! is a tautology since exp will always be greater or equal than DECIMAL_PLACES given that fractional_part.len() is a positive number. Given the this, then it is possible for a negative exp to pass the assert! which later will be casted to a u32 number.

Impact

At present, there is no immediate consequence to this situation. The limitation stems from the fact that the largest string permissible as input can only consist of 36 digits for its fractional part. Should this limit be exceeded, invoking fractional_part.parse() would result in a panic. Conversely, the smallest accepted value for exp is -18, which, upon being casted to u32, transforms into a substantial number making the next line with pow to panic due to an overflow.

Recommendation

The assert should be changed to assert!(exp >= 0).

@gangov gangov added the bug Something isn't working label Jan 16, 2024
@gangov gangov self-assigned this Jan 17, 2024
@gangov gangov linked a pull request Jan 17, 2024 that will close this issue
@gangov gangov added the audit label Feb 6, 2024
@gangov gangov changed the title Audit: Incorrect decimals assertion [V-PHX-VUL-022] Incorrect decimals assertion Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant