Closed c4-submissions closed 9 months ago
bytes032 marked the issue as high quality report
bytes032 marked the issue as primary issue
bytes032 marked the issue as duplicate of #152
jhsagd76 marked the issue as satisfactory
l think this issue should be selected for the report due to the proof of concept, which is missing in the other reports. @jhsagd76
Thank you for your excellent work. This report and POC are indeed outstanding. However, the current primay issue also clearly highlights the vulnerabilities and relevant code. I believe it would be unfair to switch to this directly.
However, additional incentive rules for POC are currently under discussion. I think you are doing the right thing, so please keep it up. Thank you very much for your excellent report!
Lines of code
https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/LeverageMacroBase.sol#L118-L211
Vulnerability details
Impact
On short explanation, the function cdpOfOwnerByIdx loops through the sorted list from the lowest NICR to the biggest NICR in order to find the correct Cdp position by the provided details:
This feature is implemented in LeverageMacroBase, so when new Cdp position is opened, the user is free to compare and check its new Cdp parameters. However in order to do that the macro does the following things:
First the system prepares for the post check, by looping through the sorted list and counting all the Cdps owned by the macro. Then returns the value of initialCdpIndex, which is the number of owned Cdps by the macro + the next Cdp which is about to be opened.
Second the post check happens, and with the help of the provided initialCdpIndex the system tries to find the correct Cdp position and compare its parameter details with the expected values.
This feature might work when opening the first Cdp, but won't work correctly when the macro already has few opened Cdp positions. The main problem occurs in the post check, when trying to find the correct Cdp position with the help of the function cdpOfOwnerByIndex, the system simply loops through the sorted list and gets the last Cdp position owned by the macro with the biggest NICR. So in order for the post check to work correctly, the system takes a guess that every newly opened Cdp by the macro will have a bigger NICR than the previous opened Cdp positions, which is unrealistic and won't be the case most of the times.
For the example and the provided POC, user opens two new Cdps and wants to check their position details:
Proof of Concept
Tools Used
Manual review.
Recommended Mitigation Steps
In the current system, there are tons of ways to figure out what will be the correct index for the newly opened Cdp. As an example with the view function getSyncedICR, the user can get the current ICR of his Cdp positions and figure out which will be the correct index to use for the post check when opening the new Cdp.
My recommendation would be to remove the setup for post call check and let the user provide his own initial index, which will be used for the post creation call check.
Assessed type
Other