Badger eBTC Audit + Certora Formal Verification Competition - nobody2018's results

Lines of code

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

Vulnerability details

Impact

After LeverageMacroBase.doOperation is used to open a new CDP, there will be a POST CALL CHECK, which is used to check the new CDP. However, the current implementation incorrectly assumes that the index of the new CDP is the last one. In this case, there may be two impacts:

1. The check that should be passed cannot be passed, causing tx revert and waste of gas.
2. The check that should not have been passed was passed, which may result in funds losses from this contract or caller.

Proof of Concept

doOperation is used to open a new CDP, and there are roughly three steps: setup for POST CALL CHECK, openCdp, POST CALL CHECK for the created CDP.

File: packages\contracts\contracts\LeverageMacroBase.sol
118:     function doOperation(
119:         FlashLoanType flType,
120:         uint256 borrowAmount,
121:         LeverageMacroOperation calldata operation,
122:         PostOperationCheck postCheckType,
123:         PostCheckParams calldata checkParams
124:     ) external {
......
139:         uint256 initialCdpIndex;
140:         if (postCheckType == PostOperationCheck.openCdp) {
141:             // How to get owner
142:             // sortedCdps.existCdpOwners(_cdpId);
143:->           initialCdpIndex = sortedCdps.cdpCountOf(address(this));
144:         }

......//do the operation

169:         if (postCheckType == PostOperationCheck.openCdp) {
170:             // How to get owner
171:             // sortedCdps.existCdpOwners(_cdpId);
172:             // initialCdpIndex is initialCdpIndex + 1
173:->           bytes32 cdpId = sortedCdps.cdpOfOwnerByIndex(address(this), initialCdpIndex);.
174: 
175:             // Check for param details
176:             ICdpManagerData.Cdp memory cdpInfo = cdpManager.Cdps(cdpId);
177:->           _doCheckValueType(checkParams.expectedDebt, cdpInfo.debt);
178:->           _doCheckValueType(checkParams.expectedCollateral, cdpInfo.coll);
179:             require(
180:                 cdpInfo.status == checkParams.expectedStatus,
181:                 "!LeverageMacroReference: openCDP status check"
182:             );
183:         }
......
211:     }

L143, initialCdpIndex = sortedCdps.cdpCountOf(address(this)), which is used to count the number of CDPs already owned by this.

L145-168, Creats a CDP, the code here is not helpful in describing the issue and is therefore omitted.

L173, sortedCdps.cdpOfOwnerByIndex(address(this), initialCdpIndex) is used to get the id of the initialCdpIndex-th CDP owned by this. That's the problem. Because initialCdpIndex is the number of CDPs owned before the new CDP was created (from L143), that is to say, the index of the created CDP is considered to be the last one.

Let's look at the code of cdpOfOwnerByIndex:

File: packages\contracts\contracts\SortedCdps.sol
140:     function cdpOfOwnerByIndex(
141:         address owner,
142:         uint256 index
143:     ) external view override returns (bytes32) {
144:->       (bytes32 _cdpId, ) = _cdpOfOwnerByIndex(owner, index, dummyId, 0);
145:         return _cdpId;
146:     }
......
173:     function _cdpOfOwnerByIndex(
174:         address owner,
175:         uint256 index,
176:         bytes32 startNodeId,
177:         uint maxNodes
178:     ) internal view returns (bytes32, bool) {
179:         // walk the list, until we get to the indexed CDP
180:         // start at the given node or from the tail of list
181:->       bytes32 _currentCdpId = (startNodeId == dummyId ? data.tail : startNodeId);
182:->       uint _currentIndex = 0;
183:         uint i;
184: 
185:         while (_currentCdpId != dummyId) {
186:             // if the current CDP is owned by specified owner
187:             if (getOwnerAddress(_currentCdpId) == owner) {
188:                 // if the current index of the owner CDP matches specified index
189:->               if (_currentIndex == index) {
190:->                   return (_currentCdpId, true);
191:                 } else {
192:                     // if not, increment the owner index as we've seen a CDP owned by them
193:->                   _currentIndex = _currentIndex + 1;
194:                 }
195:             }
196:             ++i;
197: 
198:             // move to the next CDP in the list
199:             _currentCdpId = data.nodes[_currentCdpId].prevId;
200: 
201:             // cut the run if we exceed expected iterations through the loop
202:             if (maxNodes > 0 && i >= maxNodes) {
203:                 break;
204:             }
205:         }
206:         // if we reach maximum iteration or end of list
207:         // without seeing the specified index for the owner
208:         // then maybe a new pagination is needed
209:         return (_currentCdpId, false);
210:     }

L144, _cdpOfOwnerByIndex(owner, index, dummyId, 0) is called.

L181, _currentCdpId = data.tail due to startNodeId = dummyId.

L189-194, If _currentIndex == index, the target cdpId is found and the loop exits. Otherwise, _currentIndex is increased by 1.

To better describe the problem, consider the following scenario:

this already has 1 CDP: A, and the entire linked list currently has 10 CDPs. Its topology is as follows:

head                      tail  
cdp1->A->cdp2->...->cdp8->cdp9

New CDP is created via doOperation:

1. initialCdpIndex = 1, which is from sortedCdps.cdpCountOf(address(this)).

2. open a new Cdp: B.

3. The current linked list has 11 CDPs, and its topology is as follows:

   head                         tail  
   cdp1->A->cdp2->cdp3->B->...->cdp9

4. cdpId = sortedCdps.cdpOfOwnerByIndex(address(this), 1). cdpId was expected to be B's id, but A's id was returned.

The root cause of this case is that the linked list is in descending order of NICR, and the NICR of the newly created CDP may be lower than the NICR of the already created CDP. In this case, the wrong cdpId will be returned.

Tools Used

Manual Review

Recommended Mitigation Steps

In _openCdpCallback, borrowerOperations.openCdp returns the created cdpId, and we should specify a slot to store this value. Afterwards, read this slot directly in POST CALL CHECK and reset it to byte32(0). The slot can be obtained by using a method similar to keccak256("Badger.LeverageMacroBase.OpenCdpId").

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/LeverageMacroBase.sol#L143

Vulnerability details

Impact

When LeverageMacroBase.doOperation is used to open a CDP and set postCheckType to PostOperationCheck.openCdp, the code here will count all CDPs owned by this via sortedCdps.cdpCountOf(address(this)), which will traverse the entire doubly linked list. If the number of all items in this linked list are huge, the gas consumed by traversing the entire linked list may exceed the block limit. This situation is very likely to exist in a large protocol, where a large number of users open a large number of CDPs. In this case, callers who use LeverageMacroBase to open an CDP will not only suffer DOS but also gas losses (gas is expensive on the mainnet).

Proof of Concept

When doOperation is to open a new CDP, the caller can set its argument postCheckType to PostOperationCheck.openCdp, which means that some necessary checks need to be done after opening a CDP. doOperation will setup for post call check before executing the operation, where it will count the number of CDPs that this has opened:

File: packages\contracts\contracts\LeverageMacroBase.sol
118:     function doOperation(
119:         FlashLoanType flType,
120:         uint256 borrowAmount,
121:         LeverageMacroOperation calldata operation,
122:         PostOperationCheck postCheckType,
123:         PostCheckParams calldata checkParams
124:     ) external {
......
139:         uint256 initialCdpIndex;
140:         if (postCheckType == PostOperationCheck.openCdp) {
141:             // How to get owner
142:             // sortedCdps.existCdpOwners(_cdpId);
143:->           initialCdpIndex = sortedCdps.cdpCountOf(address(this));
144:         }

If postCheckType == PostOperationCheck.openCdp at L140, L143's sortedCdps.cdpCountOf(address(this)) will be called.

File: packages\contracts\contracts\SortedCdps.sol
216:     function cdpCountOf(address owner) external view override returns (uint256) {
217:->       (uint256 _cnt, ) = _cdpCountOf(owner, dummyId, 0);
218:         return _cnt;
219:     }
......
237:     function _cdpCountOf(
238:         address owner,
239:         bytes32 startNodeId,
240:         uint maxNodes
241:     ) internal view returns (uint256, bytes32) {
242:         // walk the list, until we get to the count
243:         // start at the given node or from the tail of list
244:         bytes32 _currentCdpId = (startNodeId == dummyId ? data.tail : startNodeId);
245:         uint _ownedCount = 0;
246:         uint i = 0;
247: 
248:         while (_currentCdpId != dummyId) {
249:             // if the current CDP is owned by specified owner
250:             if (getOwnerAddress(_currentCdpId) == owner) {
251:                 _ownedCount = _ownedCount + 1;
252:             }
253:             ++i;
254: 
255:             // move to the next CDP in the list
256:             _currentCdpId = data.nodes[_currentCdpId].prevId;
257: 
258:             // cut the run if we exceed expected iterations through the loop
259:             if (maxNodes > 0 && i >= maxNodes) {
260:                 break;
261:             }
262:         }
263:         return (_ownedCount, _currentCdpId);
264:     }

L217, call _cdpCountOf with parameters: startNodeId=dummyId, maxNodes=0.

L244, _currentCdpId will be assigned to data.tail because startNodeId is dummyId.

L248-262, traverse the linked list in reverse direction. The two conditions for exiting the while loop are:

1. _currentCdpId == dummyId means that head has been traversed.
2. Whether expected iterations exceeds maxNodes. If maxNodes is 0, then this check is skipped (L259).

As can be seen from the above analysis, the cdpCountOf function will traverse the entire linked list. For a large protocol with many users, the number of CDPs must be in the thousands. It is very likely that DOS opens CDP's tx via  doOperation.

The above analysis is only the first traversal, and there will be another traversal in a special case, which makes the problem more serious.

File: packages\contracts\contracts\LeverageMacroBase.sol
169:         if (postCheckType == PostOperationCheck.openCdp) {
170:             // How to get owner
171:             // sortedCdps.existCdpOwners(_cdpId);
172:             // initialCdpIndex is initialCdpIndex + 1
173:->           bytes32 cdpId = sortedCdps.cdpOfOwnerByIndex(address(this), initialCdpIndex);
......
183:         }

The logic of traversing the linked list in sortedCdps.cdpOfOwnerByIndex is similar to cdpCountOf.

Give an example to express this issue better:

Assume that there are 1000 CDPs in the linked list, and the current calling contract (this) has 1 CDP: A. The current topology of the linked list is as follows:

head                            tail  
cdp1->cdp2->....->A->cdp500...->cdp999

Call doOperation to open a new CDP, the flow is as follows:

1. After traversing 1000 items for the first time, we got initialCdpIndex = 1(L143).

2. Execute borrowerOperations.openCdp to get the new CDP(B).

3. There are currently 2 CDPs: index0 = A, index1 = B. The new topology of the linked list is as follows:

   head                                tail
   B->cdp1->cdp2->....->A->cdp500...->cdp999

4. Then, at L173 sortedCdps.cdpOfOwnerByIndex(address(this), 1) will traverse the entire linked list again.

Tools Used

Manual Review

Recommended Mitigation Steps

Should not traverse the entire linked list on-chain.

Assessed type

DoS