Phoenix-Protocol-Group / phoenix-contracts

Source code of the smart contracts of the Phoenix DeFi hub DEX protocol
GNU General Public License v3.0
10 stars 6 forks source link

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

Closed gangov closed 8 months ago

gangov commented 8 months ago

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).