Closed c4-bot-3 closed 3 months ago
Worth checking
GalloDaSballo marked the issue as sufficient quality report
GalloDaSballo marked the issue as high quality report
GalloDaSballo marked the issue as primary issue
previewmintshares cant sync supply because its a view and views dont write to storage. Everyone can query the mintfee beforehand. There is no swapping so there is no extra fee. Way overblown anyway. No need to change anything. Dismissed
maximum Q/A but overblown and should be disqualified. User can always query impact and fee and expected value, there are view functions for that, these functions used on UI (https://app.wiselending.com/) hence no misinformation or misalignment is present in current implementation, expectations are set through views to be queried.
trust1995 marked the issue as unsatisfactory: Invalid
@trust1995 The sponsor has commented on the preview
functions but the lack of slippage protection on depositExactAmount()
, withdrawExactAmount()
, and withdrawExactShares()
hasn't been discussed.
There's a similar finding from last year that you judged as a valid medium: https://github.com/code-423n4/2023-05-maia-findings/issues/901
In the recent PoolTogether contest, a similar slippage issue was reported and again judged as a medium: https://github.com/code-423n4/2024-03-pooltogether-findings/issues/274
Given that Mainnet is in scope and there's no explicit mention of an external periphery within the protocol that handles this, I believe it makes sense that this should be considered valid given that risk of griefing and unwanted loss of funds for the user is present.
Without the preview()
aspect of the report, we are left with insufficient quality to for the briefly described slippage issue.
Lines of code
https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarmController/PendlePowerFarmToken.sol#L334-L345 https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarmController/PendlePowerFarmToken.sol#L443-L492
Vulnerability details
Impact
PendlePowerFarmToken
does not implement slippage control for the deposit/withdraw functions. Other projection functions related to those likepreviewMintShares()
also lack a supply sync, which can mislead the depositor in the case where there was a prior reward distribution.Proof of Concept
PendlePowerFarmToken
is vulnerable to griefing through slippage (either intentional or unintentional). There are functions that allow the vault share holder to project the outcome of deposit and withdraw interactions. Those are:previewMintShares()
previewAmountWithdrawShares()
previewBurnShares()
The problem is that none performs a supply sync, so the amount the user will get from those may not correspond to the amount they will get after calling:
depositExactAmount()
,withdrawExactAmount()
, orwithdrawExactShares()
.The vulnerability arises only when a reward distribution has happened before the victim's call. The supply sync is handled from the following function:
This is handled properly for the interaction functions but not for the preview ones. The supply should be synced in both places so the user can be more certain of the projected amount.
Here's, for instance, how the
depositExactAmount()
function looks:The
syncSupply
modifier it uses already performs this and it is done before the function's body is entered, so the amount returned frompreviewMintShares()
here will be correct:Coded POC (
PendlePowerFarmControllerBase.t.sol
):Logs' output:
Tools Used
Manual Review
Recommended Mitigation Steps,
The preview functions should all sync the supply so users can get a proper projection of the outcome of the particular interaction. I also recommend introducing optional slippage control to
depositExactAmount()
,withdrawExactShares()
, andwithdrawExactAmount()
. This is an extra layer of safety that will ensure the user never gets less than what they initially anticipated.Assessed type
Token-Transfer