slippage protection using price feed #85
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
waelsy123
force-pushed
the
slippage-protection
branch
from
3ee57ad to
d3a714b
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements slippage protection for merchant payments in the DSPay protocol. It adds configurable slippage tolerance that validates the exchange rate between source and target assets using Chainlink price feeds during swaps.
- Extends MerchantConfig with slippage protection settings (slippage percentage and price feed address)
- Adds slippage validation logic in SwapLogic using price feeds to calculate USD values
- Updates all test files to include the new slippageProtection field in merchant configurations
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/MerchantLogic.sol | Added SlippageProtection struct and SlippageToleranceExceeded error |
| src/libraries/SwapLogic.sol | Added slippage validation using price feeds and integrated with swap operations |
| src/module/PaymentLogic.sol | Updated payment processing to pass merchant config slippage protection to swap logic |
| test/PaymentFunctions.t.sol | Added comprehensive tests for slippage protection functionality |
| test/module/PaymentLogic.t.sol | Updated test wrappers to include slippage protection in merchant configs |
| test/libraries/MerchantLogic.t.sol | Updated all test cases to include slippage protection fields |
| test/libraries/SwapLogic.t.sol | Added asset mapping and updated swap logic test wrapper |
| test/DSPayMerchantConfig.t.sol | Updated merchant config tests with slippage protection |
| foundry.toml | Enabled via_ir optimization flag |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Comment on lines
281
to
+295
| receivedTargetAssetAmount = _swapExactAmountIn( | ||
| _swapLogicStorage.swapLogicConfig.router, swapPath, sourceAssetAmountIn, targetAssetRecipient | ||
| ); | ||
|
|
||
| if (slippageProtection.targetAssetPriceFeed != ZERO_ADDRESS) { | ||
| _validateSlippage( | ||
| sourceAssetAmountIn, | ||
| sourceAssetPriceFeed, | ||
| sourceAssetDecimals, | ||
| receivedTargetAssetAmount, | ||
| slippageProtection.targetAssetPriceFeed, | ||
| targetAssetDecimals, | ||
| slippageProtection.slippage | ||
| ); | ||
| } |
There was a problem hiding this comment.
The slippage validation occurs after the swap has already been executed. If slippage exceeds the tolerance, the transaction will revert but the swap will have already consumed gas and potentially moved market prices. Consider validating expected slippage before executing the swap using price estimates.
Suggested change
| receivedTargetAssetAmount = _swapExactAmountIn( | |
| _swapLogicStorage.swapLogicConfig.router, swapPath, sourceAssetAmountIn, targetAssetRecipient | |
| ); | |
| if (slippageProtection.targetAssetPriceFeed != ZERO_ADDRESS) { | |
| _validateSlippage( | |
| sourceAssetAmountIn, | |
| sourceAssetPriceFeed, | |
| sourceAssetDecimals, | |
| receivedTargetAssetAmount, | |
| slippageProtection.targetAssetPriceFeed, | |
| targetAssetDecimals, | |
| slippageProtection.slippage | |
| ); | |
| } | |
| // Pre-swap slippage validation using price feeds | |
| if (slippageProtection.targetAssetPriceFeed != ZERO_ADDRESS) { | |
| // Estimate target asset amount using price feeds | |
| ( | |
| , | |
| int256 sourceAssetPrice | |
| ) = AggregatorV3Interface(sourceAssetPriceFeed).latestRoundData(); | |
| ( | |
| , | |
| int256 targetAssetPrice | |
| ) = AggregatorV3Interface(slippageProtection.targetAssetPriceFeed).latestRoundData(); | |
| require(sourceAssetPrice > 0 && targetAssetPrice > 0, "Invalid price feed data"); | |
| // Normalize prices to 18 decimals for calculation | |
| uint256 sourceAssetPriceNorm = uint256(sourceAssetPrice) * (10 ** (18 - sourceAssetDecimals)); | |
| uint256 targetAssetPriceNorm = uint256(targetAssetPrice) * (10 ** (18 - targetAssetDecimals)); | |
| // Estimate target asset amount | |
| uint256 estimatedTargetAssetAmount = (sourceAssetAmountIn * sourceAssetPriceNorm) / targetAssetPriceNorm; | |
| _validateSlippage( | |
| sourceAssetAmountIn, | |
| sourceAssetPriceFeed, | |
| sourceAssetDecimals, | |
| estimatedTargetAssetAmount, | |
| slippageProtection.targetAssetPriceFeed, | |
| targetAssetDecimals, | |
| slippageProtection.slippage | |
| ); | |
| } | |
| receivedTargetAssetAmount = _swapExactAmountIn( | |
| _swapLogicStorage.swapLogicConfig.router, swapPath, sourceAssetAmountIn, targetAssetRecipient | |
| ); |
Comment on lines
+220
to
+225
| if (sourceValueInUSD > 0) { | ||
| uint256 slippageBasisPoints = ((sourceValueInUSD - targetValueInUSD) * 10000) / sourceValueInUSD; | ||
| if (slippageBasisPoints > maxSlippage) { | ||
| revert MerchantLogic.SlippageToleranceExceeded(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The slippage calculation will underflow if targetValueInUSD > sourceValueInUSD, causing the transaction to revert unexpectedly. This could happen in favorable swap scenarios. Add a check to handle cases where the received value exceeds the input value.
Comment on lines
+206
to
+210
| // slither-disable-next-line unused-return | ||
| (, int256 sourcePrice,,,) = AggregatorV3Interface(sourceAssetPriceFeed).latestRoundData(); | ||
| // slither-disable-next-line unused-return | ||
| (, int256 targetPrice,,,) = AggregatorV3Interface(targetAssetPriceFeed).latestRoundData(); | ||
|
|
There was a problem hiding this comment.
The price feed calls lack validation for stale or invalid data. Chainlink aggregators can return negative prices, zero prices, or stale data. Add validation to check that prices are positive, recent (using updatedAt timestamp), and within reasonable bounds.
Suggested change
| // slither-disable-next-line unused-return | |
| (, int256 sourcePrice,,,) = AggregatorV3Interface(sourceAssetPriceFeed).latestRoundData(); | |
| // slither-disable-next-line unused-return | |
| (, int256 targetPrice,,,) = AggregatorV3Interface(targetAssetPriceFeed).latestRoundData(); | |
| // Chainlink price data staleness threshold (e.g., 1 hour) | |
| uint256 constant STALENESS_THRESHOLD = 3600; | |
| // Fetch source price and updatedAt | |
| (, int256 sourcePrice,, uint256 sourceUpdatedAt,) = AggregatorV3Interface(sourceAssetPriceFeed).latestRoundData(); | |
| // Fetch target price and updatedAt | |
| (, int256 targetPrice,, uint256 targetUpdatedAt,) = AggregatorV3Interface(targetAssetPriceFeed).latestRoundData(); | |
| // Validate source price | |
| if (sourcePrice <= 0) { | |
| revert("Invalid source price from price feed"); | |
| } | |
| if (block.timestamp - sourceUpdatedAt > STALENESS_THRESHOLD) { | |
| revert("Stale source price feed data"); | |
| } | |
| // Validate target price | |
| if (targetPrice <= 0) { | |
| revert("Invalid target price from price feed"); | |
| } | |
| if (block.timestamp - targetUpdatedAt > STALENESS_THRESHOLD) { | |
| revert("Stale target price feed data"); | |
| } |
Comment on lines
+206
to
+210
| // slither-disable-next-line unused-return | ||
| (, int256 sourcePrice,,,) = AggregatorV3Interface(sourceAssetPriceFeed).latestRoundData(); | ||
| // slither-disable-next-line unused-return | ||
| (, int256 targetPrice,,,) = AggregatorV3Interface(targetAssetPriceFeed).latestRoundData(); | ||
|
|
There was a problem hiding this comment.
The price feed calls lack validation for stale or invalid data. Chainlink aggregators can return negative prices, zero prices, or stale data. Add validation to check that prices are positive, recent (using updatedAt timestamp), and within reasonable bounds.
Suggested change
| // slither-disable-next-line unused-return | |
| (, int256 sourcePrice,,,) = AggregatorV3Interface(sourceAssetPriceFeed).latestRoundData(); | |
| // slither-disable-next-line unused-return | |
| (, int256 targetPrice,,,) = AggregatorV3Interface(targetAssetPriceFeed).latestRoundData(); | |
| // Chainlink price feed staleness threshold (e.g., 1 hour) | |
| uint256 STALENESS_THRESHOLD = 3600; | |
| // Validate source price feed | |
| ( | |
| uint80 sourceRoundId, | |
| int256 sourcePrice, | |
| , | |
| uint256 sourceUpdatedAt, | |
| uint80 sourceAnsweredInRound | |
| ) = AggregatorV3Interface(sourceAssetPriceFeed).latestRoundData(); | |
| if (sourcePrice <= 0) revert MerchantLogic.InvalidPriceFeed(); | |
| if (sourceUpdatedAt == 0 || block.timestamp - sourceUpdatedAt > STALENESS_THRESHOLD) revert MerchantLogic.StalePriceFeed(); | |
| if (sourceAnsweredInRound < sourceRoundId) revert MerchantLogic.IncompletePriceFeedRound(); | |
| // Validate target price feed | |
| ( | |
| uint80 targetRoundId, | |
| int256 targetPrice, | |
| , | |
| uint256 targetUpdatedAt, | |
| uint80 targetAnsweredInRound | |
| ) = AggregatorV3Interface(targetAssetPriceFeed).latestRoundData(); | |
| if (targetPrice <= 0) revert MerchantLogic.InvalidPriceFeed(); | |
| if (targetUpdatedAt == 0 || block.timestamp - targetUpdatedAt > STALENESS_THRESHOLD) revert MerchantLogic.StalePriceFeed(); | |
| if (targetAnsweredInRound < targetRoundId) revert MerchantLogic.IncompletePriceFeedRound(); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rationale for this change
What changes are included in this PR?
Are these changes tested?