Open howlbot-integration[bot] opened 1 month ago
This is correct but what would be the impact? It seems that totalAssets
would behave correctly so funds wouldn't be affected. However for example getPoolData
and maxWithdraw
would be affected so we could argue that functionality is broken but it isn't described by these reports.
I'll downgrade to QA
Picodes changed the severity to QA (Quality Assurance)
Picodes marked the issue as grade-b
Can confirm the PoC is valid, but we are also unable to find significant impact on our end (besides some potential confusion if frontends use the values returned by getPoolData
to display stats)
Agreed that the impact isn't correctly described in the report. Kudos to the judge @Picodes for pointing it out. However, the following problems still occur :
maxWithdraw()
will return the incorrect amount, which will lead to reverts for the withdraw()
function temporarily. This is a violation of ERC-4626
maxRedeem()
will also cause 'redeem' to revert, if the user's shares balance in assets is greater than the pool's token balance. This is, again, a violation of ERC-4626.
withdraw()
and redeem()
will hence be temporarily affected, and some users will also lose access to their funds temporarily. After the pool recovers its balance again through deposits, these functions will operate normally.
Again, as the judge pointed out, there are no funds directly at risk, which is why my initial conviction that this is a High severity issue is incorrect.
However, given broken functionality, temporary loss of access to funds, and violations of ERC-4626, I would like to urge the judge to re-assess and see if this should be a Medium
rather than a QA
I trust the judge to make the correct decision here.
Thanks!
@sammy-tm my view is that this issue could have been a Medium but when judging a report we're supposed to take into account only the impacts described by this report, and in this case there is none
Per direction from the judge, staff have marked as 1st place. Also noting there was a tie for 1st/2nd place.
Lines of code
https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/CollateralTracker.sol#L578
Vulnerability details
s_poolAssets
can underflow inCollateralTracker.sol
. This is because, in thewithdraw()
function, the assets that the user withdraws are deducted froms_poolAssets
; however, there is no check to ensures_poolAssets >= assets
. Moreover, the updation ofs_poolAssets
is handled in an unchecked block, which makes the underflow possible.s_poolAssets
can be less thanassets
, this is because when a short option is minted, assets are moved from the Panoptic pool to the Uniswap pool. i.e, assets are deducted froms_poolAssets
and incremented ins_inAMM
. So, the underflow is possible when a large share of the deposited liquidity is in the Uniswap pool.Impact
This breaks the functionality and accounting of the entire protocol. A number of attacks can be performed to drain the pool due to this vulnerability. An example would be :
Attacker mints a large number of short options
Attacker withdraws and causes underflow
Attacker can drain the pool by calling withdraw() again as assets are now highly undervalued relative to shares.
Proof of Concept
The following test demonstrates the underflow scenario :
To run the test:
Copy the code above into
CollateralTracker.t.sol
Run
forge test --match-test test_POC_Underflow
Tools Used
Foundry
Recommended Mitigation Steps
Remove the unchecked block. Alternatively, add this check in
withdraw()
:Assessed type
Under/Overflow