Platform: Code4rena
Start Date: 24/10/2023
Pot Size: $149,725 USDC
Total HM: 7
Participants: 52
Period: 21 days
Judge: ronnyx2017
Total Solo HM: 2
Id: 300
League: ETH
Rank: 32/52
Findings: 1
Award: $117.51
π Selected for report: 0
π Solo Findings: 0
π Selected for report: SpicyMeatball
Also found by: 0xBeirao, 7ashraf, LokiThe5th, OMEN, TrungOre, alexzoid, alpha, bdmcbri, ether_sky, fatherOfBlocks, ge6a, hihen, hunter_w3b, jasonxiale, ladboy233, lsaudit, niroh, nobody2018, nonseodion, peanuts, prapandey031, shaka, twcctop, twicek, wangxx2026
117.508 USDC - $117.51
https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/LeverageMacroBase.sol#L452 https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/LeverageMacroBase.sol#L291-L310 https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/LeverageMacroBase.sol#L398-L431 https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/BorrowerOperations.sol#L963-L981 https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/BorrowerOperations.sol#L553-L594
Borrower authorization and then opencdp cannot be completed in one transaction
In the _handleOperation method, the authorization required before executing _openCdpCallback, _closeCdpCallback, and _adjustCdpCallback will be completed in _doSwaps. The specific code is as follows:
function _handleOperation(LeverageMacroOperation memory operation) internal { uint256 beforeSwapsLength = operation.swapsBefore.length; if (beforeSwapsLength > 0) { _doSwaps(operation.swapsBefore); // Authorize } // Based on the type we do stuff if (operation.operationType == OperationType.OpenCdpOperation) { _openCdpCallback(operation.OperationData); } else if (operation.operationType == OperationType.CloseCdpOperation) { _closeCdpCallback(operation.OperationData); } else if (operation.operationType == OperationType.AdjustCdpOperation) { _adjustCdpCallback(operation.OperationData); } uint256 afterSwapsLength = operation.swapsAfter.length; if (afterSwapsLength > 0) { _doSwaps(operation.swapsAfter); } }
Since access to borrowerOperations is restricted in _ensureNotSystem, the code is as follows:
function _doSwap(SwapOperation memory swapData) internal { _ensureNotSystem(swapData.addressForSwap); ... (bool success, ) = excessivelySafeCall( swapData.addressForSwap, gasleft(), 0, 0, swapData.calldataForSwap ); require(success, "Call has failed"); ... } function _ensureNotSystem(address addy) internal { require(addy != address(borrowerOperations)); // Restrict access to borrowerOperations ... }
This will result in the inability to access the borrowerOperations.permitPositionManagerApproval method for authorization, and the borrowerOperations.closeCdp and borrowerOperations.adjustCdpWithColl methods called in _closeCdpCallback and _adjustCdpCallback can be delegated to others to call. The code is as follows:
function closeCdp(bytes32 _cdpId) external override { address _borrower = sortedCdps.getOwnerAddress(_cdpId); _requireBorrowerOrPositionManagerAndUpdateManagerApproval(_borrower); // Check if there is authorization ... }
For example, I have a CDP and now I want to entrust SC Wallet to close my CDP. Since I cannot call permitPositionManagerApproval for authorization, I cannot complete this operation in a transaction.
VSCode
In the _ensureNotSystem method, remove restrictions on borrowerOperations access.
In fact, I think that the restrictions in _ensureNotSystem can be removed. This is a wallet. Any developer can develop a similar wallet according to his own wishes. The restrictions here have little significance for security.
Access Control
#0 - c4-pre-sort
2023-11-15T15:47:35Z
bytes032 marked the issue as insufficient quality report
#1 - bytes032
2023-11-15T15:47:36Z
Over inflated
#2 - GalloDaSballo
2023-11-20T09:48:40Z
Missed the important surplus, somewhat valid as QA
#3 - c4-sponsor
2023-11-20T09:48:44Z
GalloDaSballo (sponsor) acknowledged
#4 - c4-sponsor
2023-11-20T09:48:49Z
GalloDaSballo marked the issue as disagree with severity
#5 - GalloDaSballo
2023-11-20T09:49:00Z
I think we wil remove the safety check and add a way to reset allowances
#6 - jhsagd76
2023-11-27T06:55:15Z
The impact sounds insufficient to make me keep med.
#7 - c4-judge
2023-11-27T06:55:24Z
jhsagd76 changed the severity to QA (Quality Assurance)
#8 - c4-judge
2023-11-27T06:55:35Z
jhsagd76 marked the issue as grade-a
#9 - c4-judge
2023-11-27T06:56:41Z
jhsagd76 marked the issue as selected for report
#10 - c4-judge
2023-11-28T10:31:46Z
jhsagd76 marked the issue as not selected for report