Attacker can modify position (hedge or close) of any user

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/main/src/markets/gamma/GammaTradeMarketL2.sol#L81
https://github.com/code-423n4/2024-05-predy/blob/main/src/markets/gamma/GammaTradeMarket.sol#L425

Vulnerability details

modifyAutoHedgeAndClose function processes a trader's modify order to update auto close and auto hedge condition.
Only the owner of that particular position should be able to modify.
However in the current state the function allows anyone to change/close a user's position.

Proof of Concept

File: GammaTradeMarketL2.sol

    function modifyAutoHedgeAndClose(GammaModifyOrderL2 memory order, bytes memory sig) external {
        GammaModifyInfo memory modifyInfo =
            L2GammaDecoder.decodeGammaModifyInfo(order.param, order.lowerLimit, order.upperLimit, order.maximaDeviation);

        uint64 pairId = userPositions[order.positionId].pairId;

        _modifyAutoHedgeAndClose(
            GammaOrder(
                OrderInfo(address(this), order.trader, order.nonce, order.deadline),     /// @note - trader's address will of that position's owner
                pairId,
                order.positionId,
                _quoteTokenMap[pairId],
                0,
                0,
                0,
                0,
                0,
                0,
                modifyInfo
            ),
            sig
        );
    }

_modifyAutoHedgeAndClose() is called with the desired inputs & signature & after verifying the signature, the user's position is returned by calling the _getOrInitPosition().

File: GammaTradeMarket.sol

211:      function _modifyAutoHedgeAndClose(GammaOrder memory gammaOrder, bytes memory sig) internal {
            ...
221:        GammaTradeMarketLib.UserPosition storage userPosition =
222:           _getOrInitPosition(gammaOrder.positionId, false, gammaOrder.info.trader, gammaOrder.pairId);
            ...

The above call leads to _getOrInitPosition() where inside the if statement it is checked if trader calling the original modify function is the owner of that position.

File: GammaTradeMarket.sol

408:  function _getOrInitPosition(uint256 positionId, bool isCreatedNew, address trader, uint64 pairId)
       ...
425:   if (userPosition.owner != trader) {
426:        revert SignerIsNotPositionOwner();
427:   }    
       ...

Scenario:

• Alice has a position in the gamma market.
• Alice has recently called modifyAutoHedgeAndClose() to change some of the settings.
• Later, Bob sees this change & calls modifyAutoHedgeAndClose() with the same signature Alice had used earlier to validate the transaction & with order.trader as Alice's address with manipulated inputs.
• The check passes because Bob has used Alice's as input.

    if (userPosition.owner != trader) {
            revert SignerIsNotPositionOwner();
        }

• Bob is able to change the parameters of userPosition like the expiration, lower/upper limit, hedge interval, etc. which would result in loss to Alice's position in the market.

Tools Used

Manual Review

Recommended Mitigation Steps

Use msg.sender instead of order.trader inside the modifyAutoHedgeAndClose() function. This way when the call goes to _getOrInitPosition(), since the caller is not the owner of the position, the call would revert.

File: GammaTradeMarketL2.sol

    function modifyAutoHedgeAndClose(GammaModifyOrderL2 memory order, bytes memory sig) external {
        GammaModifyInfo memory modifyInfo =
            L2GammaDecoder.decodeGammaModifyInfo(order.param, order.lowerLimit, order.upperLimit, order.maximaDeviation);

        uint64 pairId = userPositions[order.positionId].pairId;

        _modifyAutoHedgeAndClose(
            GammaOrder(
-                OrderInfo(address(this), order.trader, order.nonce, order.deadline),
+                OrderInfo(address(this), msg.sender, order.nonce, order.deadline),
        ...

Assessed type

Invalid Validation

[Shubh0412]

@alex-ppg
Please have a look at the above finding.

If a user has enabled modification of their position & has actually made a change by calling the modifyAutoHedgeAndClose() in the GammaTradeMarketL2 contract, it is easy for anyone to change that user's existing position.
The verification of the signature would not be a problem in this case because the attacker can copy the same signature of the original owner of the position from the transaction block.

In the check shown below, since the attacker has set trader address to be that of the original owner, the check simply passes.

File: GammaTradeMarket.sol

408:  function _getOrInitPosition(uint256 positionId, bool isCreatedNew, address trader, uint64 pairId)
       ...
425:   if (userPosition.owner != trader) {
426:        revert SignerIsNotPositionOwner();
427:   }    
       ...

The attacker can now change any of the values inside the saveUserPosition() without the user's consent which will lead to different limits, expiration and slippage tolerance for the user's position.

File: GammaTradeMarketLib

      function saveUserPosition(GammaTradeMarketLib.UserPosition storage userPosition, GammaModifyInfo memory modifyInfo)
      ....

          // auto close condition
        userPosition.expiration = modifyInfo.expiration;
        userPosition.lowerLimit = modifyInfo.lowerLimit;
        userPosition.upperLimit = modifyInfo.upperLimit;

        // auto hedge condition
        userPosition.maximaDeviation = modifyInfo.maximaDeviation;
        userPosition.hedgeInterval = modifyInfo.hedgeInterval;
        userPosition.sqrtPriceTrigger = modifyInfo.sqrtPriceTrigger;
        userPosition.auctionParams.minSlippageTolerance = modifyInfo.minSlippageTolerance;
        userPosition.auctionParams.maxSlippageTolerance = modifyInfo.maxSlippageTolerance;
        userPosition.auctionParams.auctionPeriod = modifyInfo.auctionPeriod;
        userPosition.auctionParams.auctionRange = modifyInfo.auctionRange;
        ...

[alex-ppg]

Hey @Shubh0412, thanks for contributing to the PJQA process! This represents a validation repository finding and as such was not evaluated by me directly.

I do not believe the issue is relevant as the interaction described would consume the signature via a Permit 2 transfer preventing its replayability.

[Shubh0412]

Hello @alex-ppg,
Please allow & consider the below reply.

According to the official permit2 docs

Universal Router protects against this by checking that the msg.sender from inside the routing contract is the supposed spender by passing msg.sender in as the owner param in any permit calls and by passing in msg.sender as the from param in any transfer calls.

& also here

Similar to the security considerations outlined in SignatureTransfer, integrating contracts need to perform valid safety checks on the caller and pass in correct addresses for the from argument in any transfer calls.

The above statements are verified in _getOrInitPosition().

File: GammaTradeMarket.sol

408:  function _getOrInitPosition(uint256 positionId, bool isCreatedNew, address trader, uint64 pairId)
       ...
425:   if (userPosition.owner != trader) {
426:        revert SignerIsNotPositionOwner();
427:   }    
       ...

But the trader parameter is never msg.sender but manually entered in modifyAutoHedgeAndClose().

File: GammaTradeMarketL2.sol
81:    OrderInfo(address(this), order.trader, order.nonce, order.deadline),

As per your comment

I do not believe the issue is relevant as the interaction described would consume the signature via a Permit 2 transfer preventing its replayability.

File: GammaTradeMarket.sol

     function _verifyOrder(ResolvedOrder memory order) internal {
        order.validate();

        _permit2.permitWitnessTransferFrom(
            order.toPermit(),
            order.transferDetails(address(this)),
            order.info.trader,
            order.hash,
            GammaOrderLib.PERMIT2_ORDER_TYPE,
            order.sig
        );
    }

When verifying the order, all the parameters are being obtained from ResolvedOrder memory order which originated from below. Both gammaOrder & sig can be easily obtained once the original owner of the position has called modifyAutoHedgeAndClose().

File: GammaTradeMarket.sol

156:    ResolvedOrder memory resolvedOrder = GammaOrderLib.resolve(gammaOrder, sig);

The attacker only needs to change the modifyInfo part of the input to manipulate that user's position. All the remaining parameters of order & sig can remain the same which are used to verify the authenticity of the call.

File: GammaTradeMarketL2.sol

    function modifyAutoHedgeAndClose(GammaModifyOrderL2 memory order, bytes memory sig) external {
        GammaModifyInfo memory modifyInfo =
            L2GammaDecoder.decodeGammaModifyInfo(order.param, order.lowerLimit, order.upperLimit, order.maximaDeviation);

        uint64 pairId = userPositions[order.positionId].pairId;

        _modifyAutoHedgeAndClose(
            GammaOrder(
                OrderInfo(address(this), order.trader, order.nonce, order.deadline),     /// @note - trader's address will of that position's owner
                pairId,
                order.positionId,
                _quoteTokenMap[pairId],
                0,
                0,
                0,
                0,
                0,
                0,
                modifyInfo
            ),
            sig
        );
    }

Using msg.sender instead of order.trader above will fix the issue because when verifying only the original owner of the position will be able to modify & not anyone else.

[alex-ppg]

Hey @Shubh0412, thank you for your contribution. We can observe here that the nonce is actively consumed at the Permit2 level, so a new signature would need to be generated and no replay can occur.

[Shubh0412]

Thank you @alex-ppg for making it clear.
My bad for not going all the way through. Apologies for the trouble caused.