Open code423n4 opened 1 year ago
berndartmueller marked the issue as primary issue
kyscott18 marked the issue as sponsor confirmed
We agree with the issue and implemented the same fix
berndartmueller changed the severity to QA (Quality Assurance)
This previously downgraded issue has been upgraded by berndartmueller
berndartmueller marked the issue as selected for report
Lines of code
https://github.com/code-423n4/2023-01-numoen/blob/2ad9a73d793ea23a25a381faadc86ae0c8cb5913/src/core/Pair.sol#L56
Vulnerability details
Impact
An attacker can steal the funds without affecting the invariant.
Proof of Concept
We can say the function
Pair.invariant()
is the heart of the protocol. All the malicious trades should be prevented by this function.The problem is there is a precision loss in the L56 and L57. The precision loss can result in the wrong invariant check result. Let's say the
token0
has 6 decimals and liquidity has more than 24 decimals. Then the firstFullMath.mulDiv
will cause significant rounding before it's converted to D18. To clarify the difference I wrote a custom functioninvariant()
to see the actual value ofa+b-c-d
.Put these two functions in the
LiquidityManagerTest.t.sol
and run the case. The result is as below and it shows that while the reserve0 amount changes to 150%, the actual valuea+b-c-d
does not change.So what does this mean? We know that if
a+b-c-d
is positive, it means anyone can callswap()
to withdraw the excess value. The above test shows that the significant change in the token0 reserve amount did not change the valuea+b-c-d
. Based on this, I wrote an attack case where dennis pulls 0.5*10**6 token0 without cost while the invariant stays at zero. Although the benefit is only 0.5 USDC for this test case, this shows a possibility drawing value without affecting the invariant for pools with low decimals.Tools Used
Foundry
Recommended Mitigation Steps
Make sure to multiply first before division to prevent precision loss.