Part 2
Zaros
PerpetualsDEXFoundrySolidity
70,000 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

FullFill Swap will Fail due to minAmountOut wrong calculation

TheKhans

Summary

while swapping the usdToken of vault with assets the user will calls getAmountOfAssetOut to find the minAmount user will receive after swap it does not deduct Fee from amountOut but in case of actual swap we first deduct swapFee and protocol Fee and than Check the minAmountOut for slippage , this inconsistency will in most of cases result in revert.

Vulnerability Details

Let have a look at getAmountOfAssetOut which user will calls before initiateSwap :

/src/market-making/branches/StabilityBranch.sol:100
100: function getAmountOfAssetOut(
101: uint128 vaultId,
102: UD60x18 usdAmountInX18,
103: UD60x18 indexPriceX18
104: )
105: public
106: view
107: returns (UD60x18 amountOutX18)
108: {
109: // fetch the vault's storage pointer
110: Vault.Data storage vault = Vault.load(vaultId);
111:
112: // fetch the vault's total assets in USD; if the vault is empty
113: // revert here to prevent panic from subsequent divide by zero
114: UD60x18 vaultAssetsUsdX18 = ud60x18(IERC4626(vault.indexToken).totalAssets()).mul(indexPriceX18);
115: if (vaultAssetsUsdX18.isZero()) revert Errors.InsufficientVaultBalance(vaultId, 0, 0);
116:
117: // we use the vault's net sum of all debt types coming from its connected markets to determine the swap rate
118: SD59x18 vaultDebtUsdX18 = vault.getTotalDebt(); // this is zero but why ?
119:
120: // calculate the premium or discount that may be applied to the vault asset's index price
121: // note: if no premium or discount needs to be applied, the premiumDiscountFactorX18 will be
122: // 1e18 (UD60x18 one value)
123: UD60x18 premiumDiscountFactorX18 =
124: UsdTokenSwapConfig.load().getPremiumDiscountFactor(vaultAssetsUsdX18, vaultDebtUsdX18);
125:
126: // get amounts out taking into consideration the CL price and the premium/discount
127: amountOutX18 = usdAmountInX18.div(indexPriceX18).mul(premiumDiscountFactorX18);
128: }
129:

From the code we can see that it does not check for any fee , it return the amount of assets which user will receive after swap . but in case of swap we check this slippage after deducting the Fee:

/src/market-making/branches/StabilityBranch.sol:330
330: function fulfillSwap(
331: address user,
332: uint128 requestId,
333: bytes calldata priceData,
334: address engine
335: )
336: external
337: onlyRegisteredSystemKeepers
338: {
339: // load request for user by id
340: UsdTokenSwapConfig.SwapRequest storage request = UsdTokenSwapConfig.load().swapRequests[user][requestId];
341:
342: // revert if already processed
343: if (request.processed) {
344: revert Errors.RequestAlreadyProcessed(user, requestId);
345: }
346:
347: // working data
348: FulfillSwapContext memory ctx;
349:
350: // if request dealine expired revert
351: ctx.deadline = request.deadline;
352: if (ctx.deadline < block.timestamp) {
353: revert Errors.SwapRequestExpired(user, requestId, ctx.deadline);
354: }
355:
356: // set request processed to true
357: request.processed = true;
358:
359: // load market making engine config
360: MarketMakingEngineConfiguration.Data storage marketMakingEngineConfiguration =
361: MarketMakingEngineConfiguration.load();
362:
363: // load vault data
364: ctx.vaultId = request.vaultId;
365: Vault.Data storage vault = Vault.loadLive(ctx.vaultId);
366:
367: // get usd token of engine
368: ctx.usdToken = UsdToken(marketMakingEngineConfiguration.usdTokenOfEngine[engine]);
369:
370: // load Stability configuration data
371: StabilityConfiguration.Data storage stabilityConfiguration = StabilityConfiguration.load();
372:
373: // get price from report in 18 dec
374: ctx.priceX18 = stabilityConfiguration.verifyOffchainPrice(priceData);
375:
376: // get amount out asset
377: ctx.amountIn = request.amountIn;
378: ctx.amountOutBeforeFeesX18 = getAmountOfAssetOut(ctx.vaultId, ud60x18(ctx.amountIn), ctx.priceX18);
379:
380: // gets the base fee and swap fee for the given amount out before fees
381: (ctx.baseFeeX18, ctx.swapFeeX18) = getFeesForAssetsAmountOut(ctx.amountOutBeforeFeesX18, ctx.priceX18);
382:
383: // cache the collateral asset address
384: ctx.asset = vault.collateral.asset;
385:
386: // load the collateral configuration storage pointer
387: Collateral.Data storage collateral = Collateral.load(ctx.asset);
388: // subtract the fees and convert the UD60x18 value to the collateral's decimals value
389: ctx.amountOut =
390: collateral.convertUd60x18ToTokenAmount(ctx.amountOutBeforeFeesX18.sub(ctx.baseFeeX18.add(ctx.swapFeeX18)));
391:
392: // slippage check
393: // @audit we need to check mintOut after fee deduction
394: ctx.minAmountOut = request.minAmountOut;
395: if (ctx.amountOut < ctx.minAmountOut) {
396: revert Errors.SlippageCheckFailed(ctx.minAmountOut, ctx.amountOut);
397: }
398:
399: // calculates the protocol's share of the swap fee by multiplying the total swap fee by the protocol's fee
...
437: }
438:

Here At Line 390 we can see that we subtract the fee amount from amountOutBeforeFeesX18 and store the result in amountOut and at line 395 than we check for slippage check , so in most of cases it will revert.
Note it is exactly same issue as the vault one mention in known issue.

POC

function testFuzz_WhenSlippageCheckPassesAndThePremiumOrDiscountIsNotZero( // @audit POC
uint256 vaultId,
uint256 marketId,
uint256 vaultAssetsBalance,
uint256 swapAmount,
uint256 vaultDebtAbsUsd,
bool useCredit
)
external
whenCallerIsKeeper
whenRequestWasNotYetProcessed
whenSwapRequestNotExpired
{
// working data
TestFuzz_WhenSlippageCheckPassesAndThePremiumOrDiscountIsNotZero_Context memory ctx;
​
ctx.fuzzVaultConfig = getFuzzVaultConfig(vaultId);
​
ctx.oneAsset = 10 ** ctx.fuzzVaultConfig.decimals;
​
changePrank({ msgSender: users.owner.account });
marketMakingEngine.configureUsdTokenSwapConfig(1, 30, type(uint96).max);
​
changePrank({ msgSender: users.naruto.account });
​
// bound the vault assets balance to be between 1 asset unit and the deposit cap
vaultAssetsBalance = bound({ x: vaultAssetsBalance, min: ctx.oneAsset, max: ctx.fuzzVaultConfig.depositCap });
​
// bound the vault's total credit or debt
vaultDebtAbsUsd = bound({ x: vaultDebtAbsUsd, min: ctx.oneAsset / 2, max: vaultAssetsBalance });
​
deal({
token: address(ctx.fuzzVaultConfig.asset),
to: ctx.fuzzVaultConfig.indexToken,
give: vaultAssetsBalance
});
​
swapAmount = vaultAssetsBalance / 1e9;
deal({ token: address(usdToken), to: users.naruto.account, give: swapAmount });
​
ctx.fuzzPerpMarketCreditConfig = getFuzzPerpMarketCreditConfig(marketId);
ctx.engine = IMockEngine(perpMarketsCreditConfig[ctx.fuzzPerpMarketCreditConfig.marketId].engine);
// we update the mock engine's unrealized debt in order to update the vault's total debt state
ctx.engine.setUnrealizedDebt(useCredit ? -int256(vaultDebtAbsUsd) : int256(vaultDebtAbsUsd));
​
ctx.minAmountOut = 0;
UD60x18 priceUsdX18 = IPriceAdapter(vaultsConfig[ctx.fuzzVaultConfig.vaultId].priceAdapter).getPrice();
​
ctx.priceData = getMockedSignedReport(ctx.fuzzVaultConfig.streamId, priceUsdX18.intoUint256());
ctx.usdTokenSwapKeeper = usdTokenSwapKeepers[ctx.fuzzVaultConfig.asset];
​
ctx.amountOut =
marketMakingEngine.getAmountOfAssetOut(ctx.fuzzVaultConfig.vaultId, ud60x18(swapAmount), priceUsdX18);
​
vm.assume(ctx.amountOut.intoUint256() > 0);
ctx.minAmountOut = ctx.amountOut.intoUint128();
initiateUsdSwap(uint128(ctx.fuzzVaultConfig.vaultId), swapAmount, ctx.minAmountOut);
​
(ctx.baseFeeX18, ctx.swapFeeX18) = marketMakingEngine.getFeesForAssetsAmountOut(ctx.amountOut, priceUsdX18);
​
ctx.amountOutAfterFee = convertUd60x18ToTokenAmount(
ctx.fuzzVaultConfig.asset, ctx.amountOut.sub(ctx.baseFeeX18.add(ctx.swapFeeX18))
);
​
changePrank({ msgSender: ctx.usdTokenSwapKeeper });
​
ctx.protocolSwapFee = ctx.swapFeeX18.mul(ud60x18(marketMakingEngine.exposed_getTotalFeeRecipientsShares()));
ctx.protocolReward =
convertUd60x18ToTokenAmount(ctx.fuzzVaultConfig.asset, ctx.baseFeeX18.add(ctx.protocolSwapFee));
​
ctx.requestId = 1;
​
marketMakingEngine.fulfillSwap(
users.naruto.account, ctx.requestId, ctx.priceData, address(marketMakingEngine)
);
​
// it should transfer assets to user
}
​

Add this test case to FulFillSwap.t.sol file and run with command :forge test --mt testFuzz_WhenSlippageCheckPassesAndThePremiumOrDiscountIsNotZero -vvv

Impact

Due to Fee Deduction and than checking for slippage will always result in revert.

Tools Used

Manual Review

Recommendations

Either add it to known issue list or check for slippage before deducting fee.

Updates

Lead Judging Commences

inallhonesty Lead Judge 6 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Missing basefee and settlement fee deduction before slippage check in the Stablitybranch contract.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.