PredyPool::trade() if statement is using the wrong address

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/PredyPool.sol#L265
https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/markets/perp/PerpMarketV1.sol#L159

Vulnerability details

Impact

The trade() function uses msg.sender when checking if the trader is allowed when allowlistEnabled is set to true, which is wrong because the filler is the one executing the orders of the traders and the PredyPool::trade() function is called in the market contracts so the msg.sender is the market contract, not the trader.

Proof of Concept

Consider the scenario when a trader wants to open a Perp position

The filler executes the trade order by calling executeOrderV3L2() from the PerpMarket.sol contracts which then calls the internal executeOrder function. As we can see here the PredyPool::trade() is called
https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/markets/perp/PerpMarketV1.sol#L159

 function _executeOrderV3(
        PerpOrderV3 memory perpOrder,
        bytes memory sig,
        SettlementParamsV3 memory settlementParams,
        uint64 orderId
    ) internal returns (IPredyPool.TradeResult memory tradeResult) {
        
        ...

>>      tradeResult = _predyPool.trade(
            IPredyPool.TradeParams(
                perpOrder.pairId,
                userPosition.vaultId,
                tradeAmount,
                0,
                abi.encode(
                    CallbackData(
                        CallbackSource.TRADE3, perpOrder.info.trader, 0, perpOrder.leverage, resolvedOrder, orderId
                    )
                )
            ),
            _getSettlementDataFromV3(settlementParams, msg.sender)
        );

        
        ...
    }

When called like this the msg.sender in PredyPool.sol will be the PerpMarketV1.sol address
So here in PredyPool::trade() the if statement is wrong because it will not check if the address of the trader is allowed, because the trader is not msg.sender
https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/PredyPool.sol#L265

   function trade(TradeParams memory tradeParams, bytes memory settlementData)
        external
        nonReentrant
        returns (TradeResult memory tradeResult)
    {
        globalData.validate(tradeParams.pairId);

>>      if (globalData.pairs[tradeParams.pairId].allowlistEnabled && !allowedTraders[msg.sender][tradeParams.pairId]) {
            revert TraderNotAllowed();
        }

        tradeParams.vaultId = globalData.createOrGetVault(tradeParams.vaultId, tradeParams.pairId);

        return TradeLogic.trade(globalData, tradeParams, settlementData);
    }

Tools Used

Manual Analysis

Recommended Mitigation Steps

Use the trader's address from the tradeParams that is passed from the market contract

tradeResult = _predyPool.trade(
            IPredyPool.TradeParams(
                perpOrder.pairId,
                userPosition.vaultId,
                tradeAmount,
                0,
                abi.encode(
                    CallbackData(
>>                  CallbackSource.TRADE3, perpOrder.info.trader, 0, perpOrder.leverage, resolvedOrder, orderId
                    )
                )
            ),
            _getSettlementDataFromV3(settlementParams, msg.sender)
        );

Assessed type

Other

[YordanVuchev]

@alex-ppg
Hey, can you review it once again?
They are using msg.sender in the trade() function to see if the trader is whitelisted, but the trades are not executed by the traders, they are executed from the filler. The filler calls the execute from the market which then calls the trade() from the pool, so msg.sender is not the trader it is the address of the market contract which makes the whitelist functionality not working at all

[alex-ppg]

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

The whitelist is meant to guard direct callers rather than signatories to permit system components to integrate with the system properly.

[YordanVuchev]

Hey, @alex-ppg
I believe your statement is not correct.
According to the Predy documentation about the Filler role:
https://docs.predy.finance/predy-v6/dev/architecture#filler
The Filler is responsible for actually submitting and executing the Trader's orders on the blockchain.

This means that the filler is always the one executing the orders

This is about the whitelist according to the docs:
https://docs.predy.finance/predy-v6/dev/architecture#registerpair
When whitelistEnabled is set to true, only specific addresses can trade in this pool

This means that if there is whitelist and someone tries to trade, when the filler executes his order it should revert because he is not whitelisted. This means that this pair is for only certain people that are whitelisted and not everyone can trade in it, so this makes the whitelist not a guard as you explained, but a list of certain addresses that will be able to use this pair

[syuhei176]

I apologize for the confusion. allowlistEnabled and allowedTraders are mapping variables used for access control of market contracts that can access the pair.