Platform: Code4rena
Start Date: 05/07/2023
Pot Size: $390,000 USDC
Total HM: 136
Participants: 132
Period: about 1 month
Judge: LSDan
Total Solo HM: 56
Id: 261
League: ETH
Rank: 66/132
Findings: 2
Award: $256.59
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: peakbolt
Also found by: Ack, Nyx, carrotsmuggler, n1punp, rvierdiiev
254.4518 USDC - $254.45
Liquidators can lose rewards.
function liquidate( address[] calldata users, uint256[] calldata maxBorrowParts, ISwapper swapper, bytes calldata collateralToAssetSwapData ) external notPaused {
The liquidate() function can be used to liquidate multiple users at once. And there is a swap inside this process, so there is a slippage parameter that is collateralToAssetSwapData. It is used inside the _liquidateUser() function as we can see in the below.
uint256 minAssetMount = 0; if (_dexData.length > 0) { minAssetMount = abi.decode(_dexData, (uint256)); }
The problem is when the liquidator liquidates more than one user, there is only one slippage parameter(minAssetAmount) and it's used for all liquidations. And that slippage amount can be low for the second liquidation or third. The liquidator can lose his rewards due to slippage being too low.
Manual Review
A separate slippage parameter needs to be used in every liquidation.
Other
#0 - c4-pre-sort
2023-08-05T12:54:34Z
minhquanym marked the issue as duplicate of #122
#1 - c4-judge
2023-09-21T12:34:05Z
dmvt marked the issue as satisfactory
🌟 Selected for report: 0xWaitress
Also found by: Ack, BPZ, Breeje, LosPollosHermanos, Madalad, Nyx, Ruhum, SaeedAlipoor01988, ayeslick, c7e7eff, carrotsmuggler, cergyk, dirk_y, hack3r-0m, iglyx, kaden, kodyvim, kutugu, ladboy233, ltyu, mojito_auditor, n1punp, rvierdiiev, unsafesol, zzzitron
2.1417 USDC - $2.14
Malicious users can harm protocol by using the withdrawAllMarketFees() function with no slippage parameter.
/// @notice Loop through the master contracts and call `_depositFeesToYieldBox()` to each one of their clones. /// @dev `swappers_` can have one element that'll be used for all clones. Or one swapper per MasterContract. /// @dev Fees are withdrawn in TAP and sent to the FeeDistributor contract /// @param markets_ Singularity &/ BigBang markets array /// @param swappers_ one or more swappers to convert the asset to TAP. /// @param swapData_ swap data for each swapper function withdrawAllMarketFees( IMarket[] calldata markets_, ISwapper[] calldata swappers_, IPenrose.SwapData[] calldata swapData_ ) public notPaused { require( markets_.length == swappers_.length && swappers_.length == swapData_.length, "Penrose: length mismatch" ); require(address(swappers_[0]) != address(0), "Penrose: zero address"); require(address(markets_[0]) != address(0), "Penrose: zero address"); _withdrawAllProtocolFees(swappers_, swapData_, markets_); emit ProtocolWithdrawal(markets_, block.timestamp); }
Anyone can call the withdrawAllMarketFees() function and send fees to yieldbox but the problem is, there is a swap inside the _depositFeesToYieldBox, as we can see below.
ISwapper.SwapData memory swapData = swapper.buildSwapData( assetId, wethAssetId, 0, feeShares, true, true ); (amount, ) = swapper.swap( swapData, dexData.minAssetAmount, //@audit user input swapData ! feeTo, "" );
If a malicious user calls the function with no slippage parameter(swapData), the protocol can lose fees due to slippage.
Manual Review
Access control on the withdrawAllMarketFees() function will solve the problem.
Access Control
#0 - c4-pre-sort
2023-08-06T06:06:22Z
minhquanym marked the issue as duplicate of #266
#1 - c4-judge
2023-09-19T11:43:43Z
dmvt marked the issue as duplicate of #245
#2 - c4-judge
2023-09-22T22:16:22Z
dmvt marked the issue as satisfactory