c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/LeverageMacroBase.sol#L173-L178

Vulnerability details

Impact

When creating a new cdp using LeverageMacroBase.doOperation() the call will revert if Operator.equal is used for comparison since the expected values are compared to the wrong cdp. Even if the call succeeds, the safety measure that compares the expected values for collateral and debt will be bypassed since the values are compared to the wrong cdp and will allow the creation of unexpected cdps.

Proof of Concept

When calling LeverageMacroBase.doOperation() and setting postCheckType == PostOperationCheck.openCdp the macro intends to open a cdp. To check if the cdp was opened with the correct values, at the end of the call sortedCdps.cdpOfOwnerByIndex(address(this), initialCdpIndex) is called to get the id of the newly created cdp. To assure that the new cdp was opened with the expected values, the values of the new cdpId are compared to the expected values. If the values of the newly created cdp do not meet the expected values, the function reverts to prevent the creation of an unexpected cdp.

In the beginning of the function the variable initialCdpIndex is determined by calling sortedCdps.cdpCountOf(address(this)). The returned value is the number of cdps already created by LeverageMacroBase. After that, a new cdp is created and the cdps owne by LeverageMacroBase is increased by 1. The problem is that sortedCdps.cdpOfOwnerByIndex() is used for determining the id of the new cdp. According to the comments sortedCdps.cdpOfOwnerByIndex() returns “… a specific CDP for a given owner, indexed by it's place in the linked list relative to other Cdps owned by the same address”. This means that the linked list is used, starting from the tail, and the id with the given index is returned. So if you call “cdpOfOwnerByIndex()” with an index matching the amount of cdps the owner has, it will returns the cdp with the highest NICR (the one that is the closes to the head of the linked list). Since one can create a new cdp with any debt resulting in any NICP the probability that it will be the cdp with the highest NICP owned by the caller is quite low.

Since the values of this cdp with the highest NICP are compared to the expected values when creating the new cdp, the call will fail nearly 100% of the time when using Operator.equal, even if the new cdp was created properly.(Except when creating the first cdp)

Even if the call succeeds, the safety measure that compares the expected values for collateral and debt will be bypassed since the expected values are compared to the wrong cdp. This will allow for the creation of unexpected cdps and render the checks useless.

Tools Used

Manual review

Recommended Mitigation Steps

To fix the issue described above, make a call to “SortedCdps .toCdpId()” to determine the id of the newly created cdp before creating it. For owner use the address of LeverageMacroBase, for lockHeight use the current block height and for nonce use the public value of the variable nextCdpNonce in the SortedCdps contract

Assessed type

Invalid Validation

c4-pre-sort commented 11 months ago

bytes032 marked the issue as sufficient quality report

c4-pre-sort commented 11 months ago

bytes032 marked the issue as duplicate of #152

c4-judge commented 11 months ago

jhsagd76 changed the severity to 2 (Med Risk)