Summary
The sqrt() function incorrectly compares bit values, which will result in an incorrect calculation for certain input values.
Vulnerability Details
The function compares whether the shifted value is less than 16777002, which corresponds to 1111_1111_1111_1111_111111_0010_1010, but the correct value would be 16777215, or 1111_111111_111111_111111_111111_1111_111111_1111. This will cause incorrect calculations with certain input values.
let r := shl(7, lt(87112285931760246646623899502532662132735, x))
r := or(r, shl(6, lt(4722366482869645213695, shr(r, x))))
r := or(r, shl(5, lt(1099511627775, shr(r, x))))
@> r := or(r, shl(4, lt(16777002, shr(r, x))))
z := shl(shr(1, r), z)
z := shr(18, mul(z, add(shr(r, x), 65536)))
Impact
The function will return incorrect calculations, which may lead to loss of funds if used in monetary transactions.
Tools Used
Foundry, Manual review
Recommended Mitigation
Correct the comparison value.
// This segment is to get a reasonable initial estimate for the Babylonian method. With a bad
// start, the correct # of bits increases ~linearly each iteration instead of ~quadratically.
let r := shl(7, lt(87112285931760246646623899502532662132735, x))
r := or(r, shl(6, lt(4722366482869645213695, shr(r, x))))
r := or(r, shl(5, lt(1099511627775, shr(r, x))))
// Correct: 16777215 0xffffff
- r := or(r, shl(4, lt(16777002, shr(r, x))))
+ r := or(r, shl(4, lt(16777215, shr(r, x))))
z := shl(shr(1, r), z)
// There is no overflow risk here since `y < 2**136` after the first branch above.
z := shr(18, mul(z, add(shr(r, x), 65536))) // A `mul()` is saved from starting `z` at 181.```