Part 2

Zaros
PerpetualsDEXFoundrySolidity
70,000 USDC
View results
Submission Details
Severity: high
Invalid

Funds accumulate incorrectly in `StabilityBranch.fulfillSwap` where `protocolReward` and `amountOut` won't match `amountOutBeforeFees`

Summary

There's an issue in StabilityBranch.fulfillSwap resulting in a portion of the total expected amount out being lost while splitting the user funds and the protocol rewards.

Vulnerability Details

The fees in fulfillSwap will be distributed as ctx.protocolReward for the protocolFeeRecipients and ctx.amountOut for the user.

However, ctx.protocolReward summed with ctx.amountOut will be not be equal to ctx.amountOutBeforeFeesX18.

The issue occurs because of the following calculations:

ctx.amountOut = collateral.convertUd60x18ToTokenAmount(ctx.amountOutBeforeFeesX18.sub(ctx.baseFeeX18.add(ctx.swapFeeX18)));
ctx.protocolSwapFeeX18 = ctx.swapFeeX18.mul(ud60x18(marketMakingEngineConfiguration.totalFeeRecipientsShares));
ctx.protocolReward = collateral.convertUd60x18ToTokenAmount(ctx.baseFeeX18.add(ctx.protocolSwapFeeX18));

As we can see, the amountOut is the amountOutBeforeFees subtracted with the sum of baseFee and swapFee, while the protocolReward is the baseFee summed with the multiplication of the swapFee with the totalFeeRecipientsShare.

This will result in some value being lost between amountOutBeforeFees and the values effectively being distributed via token transfers: ctx.protocolReward and ctx.amountOut.

We can verify this with the following snippet computations below.

Please note swapFee and baseFee are resulting in very small values in the existing tests, e.g. it's resulting in 0 or 1 wei, therefore we'll use some fee values greather than 1 wei to demonstrate this accumulation error and to highlight the value being lost. We are assuming that baseFee and swapFee should have non negligible values in some cases, otherwise there wouldn't be a reason for these fees to be defined.

We will also be using the values with 18 decimals as uint256 instead of ud60 for ease of demonstration.

amountOutBeforeFees = 1000e18
collateralDecimals = 18;
baseFee = 30e18 (for demonstration)
swapFee = 70e18 (for demonstration)
totalFeeRecipientsShare = 0.1e18 (from the tests cases)
ctx.amountOut = collateral.convertUd60x18ToTokenAmount(ctx.amountOutBeforeFeesX18.sub(ctx.baseFeeX18.add(ctx.swapFeeX18)));
ctx.amountOut = 1000e18 - (30e18 + 70e18)
ctx.amountOut = 900e18
ctx.protocolSwapFeeX18 = ctx.swapFeeX18.mul(ud60x18(marketMakingEngineConfiguration.totalFeeRecipientsShares));
ctx.protocolSwapFeeX18 = (70e18 * 0.1e18) / 1e18
ctx.protocolSwapFeeX18 = 7e18
ctx.protocolReward = collateral.convertUd60x18ToTokenAmount(ctx.baseFeeX18.add(ctx.protocolSwapFeeX18));
ctx.protocolReward = 30e18 + 7e18
ctx.protocolReward = 37e18

With these values, the protocolFeeRecipients will receive 37e18 and the user will receive 900e18, but 63e18 from amountOutBeforeFees would be lost since 1000e18 - 900e18 - 37e18 = 63e18.

Impact

This will cause expected values not being distributed while fulfilling a swap from usd to a collateral from the vault.

Although there's a slippage protection, the impact is considerable as it results in assets not being delivered correctly to the user. The bigger the fees the bigger the discrepancy can be.

Likelihood is high as it can happen at every swap being that gets fulfilled - assuming swapFee and baseFee have substantial values greater than 1 wei.

Tools Used

Manual review.

Recommendations

Calculate the protocolReward first and then calculate the amountOut as amountOutBeforeFees decremented by the protocolReward.

Updates

Lead Judging Commences

inallhonesty Lead Judge
10 months ago
inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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

Give us feedback!