Closed c4-bot-6 closed 7 months ago
"the error lies that if the number is greater than 128 bits then shifting it to the right by 48 bits(maximimSB(128) - 80) does not reduce precision but divides the number by 2 ** 48." -> What is the difference between removing the last 48 decimals and shifting the uints?
Picodes marked the issue as unsatisfactory: Invalid
Lines of code
https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolMath.sol#L161 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolMath.sol#L165
Vulnerability details
Impact
In the PoolMath contract, there exist a byte shift in order to reduce the decimal precision of input variables.
However instead of reducing the precision the logic allows any number that than 2 128 to effectively be divided by by 2 48.
The impact is that should any tokens have decimals that exceed these value, they will not lose precision but their actual value.
Proof of Concept
In the function _zapSwapAmount
The line
checks if any of the values reserve0, reserve1, zapAmount0 and zapAmount1 are greater than 2 ** 128.
If these are indeed greater then shift them to the right by 48 bits
However the error lies that if the number is greater than 128 bits then shifting it to the right by 48 bits(maximimSB(128) - 80) does not reduce precision but divides the number by 2 ** 48.
Tools Used
Manual Review
Recommended Mitigation Steps
I would recommend that the protocol rather divides by the number of decimals needed to be truncated then use explicit conversion, as opposed to shifting.
Assessed type
Math