Closed sherlock-admin3 closed 1 month ago
sherlock-admin2
commented
2 months ago 2 comment(s) were left on this issue during the judging contest.
santipu_ commented:
Invalid - UniversalSigValidator is a contract that doesn't make calls to update the Pyth price in the current state of the code.
takarez commented:
the pyth oracle is trusted provide an accurate data in this case.
opass
commented
2 months ago The attack is unable to take effect.
In the worst case, the hacker can use UniversalSigValidator to make an arbitrary code execution and update the price.
The attack flow is as follow
// File: perp-contract-v3/src/orderGatewayV2/OrderGatewayV2.sol
161: function settleOrder(SettleOrderParam[] calldata settleOrderParams) external onlyRelayer(_sender()) nonReentrant {
162: uint256 settleOrderParamsLength = settleOrderParams.length;
163:
164: // NOTE: only support 1 <-> 1 order matching for now
165: if (settleOrderParamsLength != 1) {
166: revert LibError.SettleOrderParamsLengthError();
167: }
168:
169: IAddressManager addressManager = getAddressManager();
170: IClearingHouse clearingHouse = addressManager.getClearingHouse();
171: InternalContext memory context;
172: // we cache some storage reads in "context" since we will use them multiple times in other internal functions
173: context.vault = addressManager.getVault();
174: context.config = addressManager.getConfig();
175: context.pythOracleAdapter = addressManager.getPythOracleAdapter();
176: context.collateralTokenDecimals = IERC20Metadata(_getAsset(context.vault)).decimals();
177:
178: for (uint256 i = 0; i < settleOrderParamsLength; i++) {
179: SettleOrderParam memory settleOrderParam = settleOrderParams[i];
180: uint256 marketId = settleOrderParam.signedOrder.order.marketId;
181: context.imRatio = context.config.getInitialMarginRatio(marketId);
// get PriceA as context.oraclePrice
182: context.oraclePrice = _getPrice(context.pythOracleAdapter, context.config, marketId);
183:
// in _fillTakerOrder, verifyOrderSignature may be updated to PriceB
184: (InternalWithdrawMarginParam memory takerWithdrawMarginParam, uint256 takerRelayFee) = _fillTakerOrder(
185: context,
186: settleOrderParam
187: );
188:
// in _fillMakerOrder, verifyOrderSignature may be updated to PriceC
189: (
190: bytes memory makerData,
191: InternalWithdrawMarginParam memory makerWithdrawMarginParam,
192: uint256 makerRelayFee
193: ) = _fillMakerOrder(context, settleOrderParam);
194:
// in _openPosition, may use PriceB or PriceC to check margin requirement
195: _openPosition(
196: InternalOpenPositionParams({
197: clearingHouse: clearingHouse,
198: settleOrderParam: settleOrderParam,
199: makerData: makerData,
200: takerRelayFee: takerRelayFee,
201: makerRelayFee: makerRelayFee
202: })
203: );
204:
205: // withdraw margin for taker reduce order
// in _withdrawMargin,
206: if (takerWithdrawMarginParam.trader != address(0)) {
207: _withdrawMargin(
208: context,
209: marketId,
210: takerWithdrawMarginParam.trader,
211: takerWithdrawMarginParam.requiredMarginRatio
212: );
213: }
214:
215: // withdraw margin for maker reduce order
216: if (makerWithdrawMarginParam.trader != address(0)) {
217: _withdrawMargin(
218: context,
219: marketId,
220: makerWithdrawMarginParam.trader,
221: makerWithdrawMarginParam.requiredMarginRatio
222: );
223: }
224: }
225: }
226:// File: perp-contract-v3/src/orderGatewayV2/OrderGatewayV2.sol
462: function _withdrawMargin(
463: InternalContext memory context,
464: uint256 marketId,
465: address trader,
466: uint256 requiredMarginRatio
467: ) internal {
// requiredMarginRatio is calculated from PriceA
468: uint256 withdrawableMargin = _getWithdrawableMargin(context, marketId, trader, requiredMarginRatio);
469: if (withdrawableMargin > 0) {
470: context.vault.transferMarginToFund(
471: marketId,
472: trader,
473: withdrawableMargin.formatDecimals(INTERNAL_DECIMALS, context.collateralTokenDecimals)
474: );
475: }
476: }// File: perp-contract-v3/src/vault/Vault.sol
406: function _transferMarginToFund(
407: uint256 marketId,
408: address trader,
409: uint256 amountXCD
410: ) internal marketExistsAndActive(marketId) {
411: if (amountXCD == 0) {
412: revert LibError.ZeroAmount();
413: }
414:
415: // check free collateral is enough for withdraw
// will use PriceB or PriceC to check, which is the same as OpenPosition
416: uint256 price = _getPrice(marketId);
417: uint256 freeCollateral = getFreeCollateral(marketId, trader, price);
418:
419: // convert margin from collateral decimals to INTERNAL_DECIMALS for comparison
420: IERC20Metadata collateralToken = IERC20Metadata(_getVaultStorage().collateralToken);
421: uint256 amount = amountXCD.formatDecimals(collateralToken.decimals(), INTERNAL_DECIMALS);
422: if (freeCollateral < amount) {
423: revert LibError.NotEnoughFreeCollateral(marketId, trader);
424: }
425:
426: // repay from margin first if there's any unsettled loss before
427: _settlePnl(marketId, trader, 0);
428:
429: // update accounting
430: _formatAndUpdateMargin(marketId, trader, -amountXCD.toInt256());
431:
432: _updateFund(trader, amountXCD.toInt256());
433: }
IllIllI000
commented
2 months ago @opass I agree that _transferMarginToFund() checks using the new price, so the second case I list where the attacker can take more than the mm requirements should let them is invalid (not a High), but the first case I list is still valid (a Medium). A legitimate user may do a ReduceOnly and calculate that they'll have plenty of a margin buffer by reducing by a specific amount, but the attacker can change the price such that the original user's buffer is a lot smaller than it should have been, including to right at the maintenance margin requirement (if the price moves enough), making the user's risk reduction ineffective.
opass
commented
2 months ago Hi @IllIllI000 , thanks for your reply. I checked the code and your argument statement again.
Let me rephrase your point of view:
The taker can propose a withdrawableMargin. After this withdrawableMargin be withdrawn (transfer to fund), the taker's margin will be below imRatio. So it makes our imRatio check useless. (imRatio is initial margin ratio) and it's vulnerable.
462: function _withdrawMargin(
463: InternalContext memory context,
464: uint256 marketId,
465: address trader,
466: uint256 requiredMarginRatio
467: ) internal {
468: uint256 withdrawableMargin = _getWithdrawableMargin(context, marketId, trader, requiredMarginRatio);
469: if (withdrawableMargin > 0) {
470: context.vault.transferMarginToFund(
471: marketId,
472: trader,
473: withdrawableMargin.formatDecimals(INTERNAL_DECIMALS, context.collateralTokenDecimals)
474: );
475: }
476: }
But inside vault.transferMarginToFund, the proposed withdraw amount must be less or equal than freeCollateral.
406: function _transferMarginToFund(
407: uint256 marketId,
408: address trader,
409: uint256 amountXCD
410: ) internal marketExistsAndActive(marketId) {
411: if (amountXCD == 0) {
412: revert LibError.ZeroAmount();
413: }
414:
415: // check free collateral is enough for withdraw
416: uint256 price = _getPrice(marketId);
417: uint256 freeCollateral = getFreeCollateral(marketId, trader, price);
418:
419: // convert margin from collateral decimals to INTERNAL_DECIMALS for comparison
420: IERC20Metadata collateralToken = IERC20Metadata(_getVaultStorage().collateralToken);
421: uint256 amount = amountXCD.formatDecimals(collateralToken.decimals(), INTERNAL_DECIMALS);
// ---here---
422: if (freeCollateral < amount) {
423: revert LibError.NotEnoughFreeCollateral(marketId, trader);
424: }
425:
426: // repay from margin first if there's any unsettled loss before
427: _settlePnl(marketId, trader, 0);
428:
429: // update accounting
430: _formatAndUpdateMargin(marketId, trader, -amountXCD.toInt256());
431:
432: _updateFund(trader, amountXCD.toInt256());
433: }
Inside getFreeCollateral calculation process, the freeCollateral is calculated based on imRatio and PriceB/C. So the taker cannot withdraw more freeCollateral than his imRatio.
// MarginProfile.sol
28: /// @inheritdoc IMarginProfile
29: function getFreeCollateral(uint256 marketId, address trader, uint256 price) public view returns (uint256) {
30: int256 accountValue = getAccountValue(marketId, trader, price);
31: if (accountValue <= 0) {
32: return 0;
33: }
34:
35: uint256 freeMargin = getFreeMargin(marketId, trader);
36: uint256 initialMarginRequirement = getMarginRequirement(marketId, trader, MarginRequirementType.INITIAL);
37: uint256 minOfFreeMarginAndAccountValue = FixedPointMathLib.min(freeMargin, accountValue.toUint256());
38: if (initialMarginRequirement >= minOfFreeMarginAndAccountValue) {
39: return 0;
40: }
41: return minOfFreeMarginAndAccountValue - initialMarginRequirement;
42: }
I agree that enhancing the code to consistently pass the price throughout can lead to improvements, making it easier to identify errors. However, even in the current version of the code, the vulnerability claim is not valid since the imRatio check remains effective.
IllIllI000
commented
2 months ago @opass After this withdrawableMargin be withdrawn (transfer to fund), the trader's margin will be below imRatio I'm saying something different. The trader is trying to reduce their position, which means keeping the margin ratio at exactly the same level as before, but reducing the amount of margin that may be taken during liquidation. The attacker is able to change the margin ratio, such that the amount of margin is larger than the trader expects. In other words, the trader sets some value, and the attacker is able to introduce some slippage into that value. I'm not saying anything about whether that affects the imRatio or mmRatio - I'm saying that the trader-supplied preference of reduceonly is being bypassed. The user still has enough margin to satisfy the protocol, but the fact remains that what they asked for, did not occur.
opass
commented
2 months ago Hi @IllIllI000, If I understand right, you're suggesting that users expect their margin ratio to remain consistent, both before and after a settleOrder, and this is unrelated to the imRatio.
For example, a trader aims to maintain a margin ratio of 30% after reducing their position. If we calculate using the old price, the margin ratio remains at 30% even after the position reduction. if recalculated with the new price, the margin ratio might deviate from 30%, which contradicts the trader's original intent.
In the following two cases, the risk of trader is the same.
Since oracle price changes all the time, the difference is already on-chain or not yet. Any one can update oracle price on-chain to new price and the taker's margin ratio will be changed based on new price.
The system has ensured the user can keep the same margin ratio in calculation at old price. We think it's acceptable for following reasons.
IllIllI000
commented
2 months ago @opass I agree that if only one trade is happening for the user, then there's no extra risk, but it's possible that the user is closing multiple parts of the position with multiple makers, and if the margin for one of the trades is reduced more than expected, it could cause the second part of the transaction with the second maker to revert, which it wouldn't have otherwise. The time to figure out what happened and retry could be enough time for the price to move and for the position to get liquidated.
opass
commented
2 months ago @IllIllI000 If a price update from a previous transaction causes the next user's transaction to fail, it means the system is more secure.
This is because the price has already been updated, and allowing users to successfully initiate trades with outdated prices could expose them to greater risks. (in next block, their margin will be calculated in latest price)
In such cases, I believe the inconvenience caused is just the price of safety.
IllIllI000
commented
2 months ago If a price can be proven to be stale, then user actions are allowed to fail. If the user expected to use a more recent price than the attacker, then it's a user error to not have updated to that price. If the relayer didn't use the most recent price, that's out of scope due to the readme. @nevillehuang as long as this logic applies to other submissions, I'm comfortable with closing this one
nevillehuang
commented
1 month ago @IllIllI000 Which other submissions are you referring to?
IllIllI000
commented
1 month ago @nevillehuang I don't have any particular issue in mind, it was more a general statement in case there's an escalation that happens to fall along similar lines
IllIllI
high
Signature validation callbacks can be used to make margin withdrawal calculations invalid
Summary
The Perpetual protocol uses ERC-6492 signatures for order validation, and this signature scheme allows the user to execute arbitrary code, including updates to the Pyth price oracle.
Vulnerability Detail
The code that calculates the ReduceOnly margin ratio, is done using a price that was fetched prior to the order validation callbacks being triggered, which means the prices and therefore margin calculations done previously will be invalid if either the maker or the taker update the Pyth oracle with a price that is different from the one used by the gateway. With crypto's fast-paced markets, price changes can be very large.
The actual settling of the order by the ClearingHouse verifies that the current margin requirements are fulfilled based on the current Pyth price, but the code in the OrderGatewayV2 that tries to maintain the same margin ratio before and after the order, does not re-check the ratio with the new price.
Impact
A user on the border of liquidation, who properly reduces their position to within the limits, could end up being liquidated in the next block, because too much margin was withdrawn. Alternatively, a user that knows they'll be liquidated soon, can withdraw more margin than they should be able to, leading to higher risk of bad debt for the exchange.
Code Snippet
The price that the Relayer set is fetched and saved to
context.oraclePrice, and then used in_fillTakerOrder()/_fillMakerOrder()to calculate therequiredMarginRatio, but by the time_openPosition()is called, the price it uses to fill the order may be different:https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/orderGatewayV2/OrderGatewayV2.sol#L191-L211
Tool used
Manual Review
Recommendation
Pass through the stored price through every call that requires it, rather than having each contract fetch it from Pyth. It can be a Relayer-specific code path