Foil Vault and Prediction Market

PredictionMarket.mint() is called by the maker and accepts a takerSignature to verify the specified taker has approved the prediction. The signed message doesn't include any type of nonce. This allows the same signature to be used as many times as possible until either:

• the allowance of the taker runs out
• the collateral balance of the taker runs out
• the signature expires due to its deadline

A malicious maker can drain the taker balance since most likely the take will have approved a large amount of tokens to the PredictionMarket contract.

This poses a danger for the vault. If it signed a message for a prediction that's to be soon resolved and the signature still hasn't expired, the maker can drain all of its funds by repeatedly calling mint().

Add a replay protection mechanism, for example nonce.

Users can artificially inflate the value of their shares by transferring losing predictions into the system.

For example:

• Eve enters the PassiveLiquidityVault
• Eve mints a prediction with the following stats to herself (takes on both the maker and taker side):
  • the prediction will settle in the next days
  • the prediction will extremely unlikely resolve to yes (something like an alien invasion will start tomorrow)
  • the yes side holds a huge amount of liquidity and the no side almost nothing
• Eve transfers the yes side to the PassiveLiquidityVault and therefore increases userCollateralDeposits and therefore it's share value drastically
• Eve exits the vault and gains from the increased share price
• Soon after that the prediction resolves to No and Eve gets her funds back
• Repeat

Another impact of this issue is causing high utilization rates. For instance:r

• the vault holds 2000 assets and 200 of them are deployed (10% utilization)
• the attacker inflates the collateral of the vault with 50_000 assets => utilizationRate = 50 200 / 52 200 = >96%
• the rest of the funds of the vault cannot be used anymore.

If there is going to be only 1 vault ever, you can hardcode its address in the PredictionMarket and forbid transfers to it. Othwerise:

1. Introduce the following mappings to the PredictionMarket contract

    mapping(uint256 => address) private initialNftMaker;
    mapping(uint256 => address) private initialNftTaker;
    mapping(address => uint256) private vaultProvidedCollateral;

1. Record the initial owners of the NFTs in _createPrediction() and also increase the vaultProvidedCollateral for the maker and the taker

        initialNftMaker[makerNftTokenId] = maker;
        initialNftTaker[takerNftTokenId] = taker;

        vaultProvidedCollateral[maker] += makerCollateral;
        vaultProvidedCollateral[taker] += takerCollateral;

1. In the burn() function check if the owner of the NFT is the initial owner and if yes, decrease the vaultProvidedCollateral

        if (ownerOf(prediction.makerNftTokenId) == initialNftMaker[prediction.makerNftTokenId]) {
            vaultProvidedCollateral[initialNftMaker[prediction.makerNftTokenId]] -= prediction.makerCollateral;
        }
        if (ownerOf(prediction.takerNftTokenId) == initialNftTaker[prediction.takerNftTokenId]) {
            vaultProvidedCollateral[initialNftTaker[prediction.takerNftTokenId]] -= prediction.takerCollateral;
        }

1. Add a public getter function getVaultProvidedCollateral to PredictionMarket.sol and IPredictionMarket.sol

    function getVaultProvidedCollateral(
        address user
    ) external view returns (uint256) {
        return vaultProvidedCollateral[user];
    }

1. Change the PassiveLiquidityVault._deployedLiquidity() to use the newly created view function

    function _deployedLiquidity() internal view returns (uint256) {
        // get vault's owned NFTs and sum the collateral of each for each NFT
        uint256 totalDeployedAmount = 0;
        address[] memory protocols = activeProtocols.values();
        for (
            uint256 protocolIndex = 0;
            protocolIndex < protocols.length;
            protocolIndex++
        ) {
            address protocol = protocols[protocolIndex];
            IPredictionMarket pm = IPredictionMarket(protocol);
            uint256 userCollateralDeposits = pm.getVaultProvidedCollateral(
                address(this)
            );
            totalDeployedAmount += userCollateralDeposits;
        }
        return totalDeployedAmount;
    }

By doing this even if someone transfers tokens to the vault, they will not be counted as deployed liquidity.

Implemented in the fuzzing suite - a Guardian proof of concept

The PredictionMarket inherits the ERC721 standard as every maker and taker position is an NFT.

However the advantage of using NFTs as positions (the transferability) is not used. It is possible to transfer the NFTs to other accounts however this account will not become the new maker or taker on transfer. Therefore it is not possible to transfer or sell positions.

Consider to implement logic that allows to transfer positions.

When users requests deposits, their assets are transferred to the vault contract and the unconfirmedAssets variable is increased with the transferred amount. Several different issues happen because of that:

1. The availableAssets and _getAvailableAssets functions do not subtract the unconfirmedAssets from the balance of the contract.

This leads to issues in multiple parts of the codebase:

• It is possible that the manager uses unconfirmed assets in predictions as the approveFundsUsage function includes the unconfirmedAssets in the available one
• The calculated utilization ratio is wrong if there are any unconfirmedAssets in the contract

1. There is a potential exploit when the emergencyMode is active:

PassiveLiquidityVault.emergencyMode() can be executed when emergencyMode == true.

This allows the users to redeem their shares atomically, without any requests. The used price ratio is the following

        uint256 vaultBalance = _getAvailableAssets();
        if (vaultBalance == 0) revert InsufficientAvailableAssets(shares, 0);

        uint256 withdrawAmount = Math.mulDiv(
            shares,
            vaultBalance,
            totalShares,
            Math.Rounding.Floor
        );

The _getAvailableAssets() returns the raw balance of the contract, including unconfirmedAssets. This is problematic since the price ratio can be manipulated by depositing a large amount via requestDeposit() (which won't be settled by the manager) and later calling cancelDeposit() to get back the funds.

The formula for the additional assets needed to drain the whole vault is

Δassets = totalAssets * (totalShares / attackerShares - 1)

However, the attacker will still need to wait for expirationTime to pass, unless their large requestDeposit() was executed in the past.

Furthermore, even if the function is used normally, the depositors of the unconfirmedAssets don't own shares, which means their assets are distributed to the rest of the share holders.

1. Because the NAV of the vault depends on its balance + the deployed assets, some withdrawals may have the correct price ratio, but not enough assets in the vault backing them. Right now, unconfirmedAssets are sitting in the contract and can be consumed by these withdrawals, leaving users unable to cancel their requests.

For example:

- There are 5000 assets in the vault and 5000 shares (1:1)
- 80% of the funds (4000) are deployed to protocols
- The contract is left with `balance = 1000` and `deployedLiquidity = 4000`.
- Alice requests a deposit of `1000 assets`  ⇒ `unconfirmedAssets = 1000` and `asset.balanceOf(vault) = 2000`
- Now Bob can withdraw 2000 of their shares for 2000 assets and leave the vault with 0 assets
- If Alice request expires, she won’t be able to cancel it because there are not enough assets in the contract to refund her.
- Now Alice cannot perform any operations in the vault because of its one request per time design. She has to wait until new assets enter the vault or supply them herself, leading to potential losses.

Instead of storing the unconfirmed assets in the vault, introduce a new contract, UnconfirmedAssetsHolder and transfer the assets to it. You will allow the vault contract to spend the tokens on behalf of UnconfirmedAssetsHolder and the vault will:

• transfer the funds back to the user when cancelDeposit is executed
• transfer the funds to itself when processDeposit is executed

To deploy liquidity to the prediction markets, the manager of the vault will periodically call approveFundsUsage() and will sign messages which will turn the PassiveLiuqidityVault into a taker when PredictionMarket.mint() is called.

The PredictionMarket uses _safeMint() to mint NFTs to both the maker and the taker. However, the taker (PassiveLiquidityVault) doesn't implement the onERC721Received() function. Because of that the transaction reverts, the vault is unable to receive the NFT and its liquidity can never be deployed.

Implement the onERC721Received() in the PassiveLiquidityVault().

As stated out in docs, the lastUserInteractionTimestamp is used to add a delay between user request to prevent rapid-fire interactions.

However if the user's request was not processed and canceled instead after 10 minutes (for example because of price changes), this timestamp is not reset. Therefore if a user is unlucky and prices fluctuate a lot as many predictions are settled it may take the user days just to enter the vault.

Consider to reset the lastUserInteractionTimestamp on cancel.

PredictionMarket.consolidatePrediction() allows the owner of both the maker and taker NFT for a given prediction to receive all of the collateral in exchange of both the tokens, successfully exiting the prediction market.

However, there is no access control applied to the function, so anyone is allowed to call it. Users on the market who hold both maker and taker tokens will always be in danger of being consolidated. They will still receive their collateral, but won't be able to sell the tokens anymore. Moreover, if the NFTs are used in a market contract for users to buy them, an attacker can just call consolidatePrediction() to burn the NFTs, leading to unexpected behavior.

Allow only the owner of the tokens to call the function.

        if (prediction.maker != prediction.taker)
            revert MakerAndTakerAreDifferent();
+       require (msg.sender == prediction.maker, <<SomeError>>);

The following edge case scenario will break the PassiveLiquidityVault:

• X amount of funds are deposited into the vault
• The manager opens positions with X*UtilRatio amount
• Users withdraw X - X*UtilRatio from the vault
• Utilization Ratio is 100% now
• The vault loses all open predictions
• Now the vault owns 0 assets but has a totalSupply of shares > 0
• A new user enters the vault with $Y assets and gets Y shares in return (1:1 ratio as there are no assets in the vault yet)
• X > Y which means another user who owns >= Y Shares can steal all funds from the new user

Make sure to pause the vault forever and redeploy the system if this edge case scenario should ever happen.

When a prediction is created, the encodedPredictedOutcomes are checked by the PredictionMarketUmaResolver.validatePredictionMarkets() function. The validation performed there requires the marketId for each outcome to be different than 0.

The prediction itself is a parley and the outcomes are treated as its legs, meaning if even one of the outcomes predicted is not correct, the whole prediction should resolve in favor of the taker.

The logic for this is implemented in PredictionUmaResolver.resolvePrediction().

            if (market.marketId != marketId) {
                isValid = false;
                error = Error.INVALID_MARKET;
                break;
            }

            if (!market.settled) {
                // Do not immediately invalidate; record and continue.
                hasUnsettledMarkets = true;
                continue;
            }

            bool marketOutcome = market.resolvedToYes;

            if (predictedOutcomes[i].prediction != marketOutcome) {
                makerWon = false;
                // Decisive loss on a settled market: return valid with no error
                return (true, Error.NO_ERROR, makerWon);
            }

If any of the markets is not settled yet, the function doesn't return yet, but checks for a losing leg and if it finds one, the parlay is resolved with makerWon = false.

However, if market.marketId != marketId, the parlay is treated as invalid. Because marketId can be any value besides 0, it's possible to create parlays that contain marketId that will never be wrapped.

At the time of creating the prediction, the taker will see all the legs, but since he joins a parlay, he will assume that even if one of the outcomes is wrong, he will win everything, so the taker will be incentivized to join. Later, even when the markets are settled, the bet will always resolve as invalid and the collateral will remain stuck in the PredictionMarket. If the manager doesn't check that before signing, it's possible to even trap the PassiveLiquidityVault.

Apply the same logic for the market.marketId != marketId case like you do for !market.settled - if any of the other outcomes are already resolved in the opposite direction, resolve with makerWon = false, otherwise return invalid.

Depositing into or withdrawing from the vault works in a 2-step flow:

1. The user makes a request with a given amount of funds to deposit or withdraw together with the shares to receive or burn.
2. The manager bot executes the request if the result of it would not lead to stealing from current users.

For the deposit flow the user should not provide the shares to mint based on the current share price but a bit less instead as slippage could happen when the vault makes profit.

However the vault could also make a loss and in that case the slippage check is insufficient, for example:

• The vault is empty
• Bob enters the with vault with $100 assets and receives 100 shares in return (share price = $1)
• Alice creates a deposit request with $100 assets and expects 98 shares in return (2% slippage)
• A big bet is lost and the vault loses 25% of it’s assets
  • Now the vault has 75 assets and 100 shares
  • Therefore the share price is 1 * 100 / 75 = $1.33 now
  • Bob’s shares are now worth: 100 * 75 / 100 = $75
• Alice request is processed and she receives 98 Shares with a worth of: 98 * 175 / 198 = $86.61
• Bob’s shares are now worth: 100 * 175 / 198 = $88.38
• Therefore Alice accidentally gifted a big amount of her deposit to Bob

Consider adding additional sharesToMint parameter to processDeposit which is provided by the manager and uses the actual price and pass it to _mint().

Then make sure the value is not lower than what the user specified

require(sharesToMint >= request.shares)

Apply the same fix for processWithdrawal() as well.

Both PredictionMarket.fillOrder() and PredictionMarket.mint() allows users to match their prediction against an opposite one.

In the case of fillOrder(), the taker matches an order placed by the maker and in the case of mint(), the maker matches an order signed by the taker. The orders in both functions have deadlines that make them invalid after expiration, but there is no way to cancel them earlier. If the market outcome becomes known prematurely and before the deadline expires, these orders can be used to create 100% winning predictions and steal money from their creators.

For example, a maker places NO order for Will PEPE hit $1 until the end of 2025 market with deadline of 1 day. However, PEPE hits $1 after 2 hours. Takers are incentivized to fill that order since it’s a sure win for them, while makers cannot do anything do stop them.

1. Consider to revert in fillOrder if a market is already settled.
2. Allow canceling orders at any point in time

function cancelOrder(uint256 orderId) external nonReentrant {
    IPredictionStructs.LimitOrderData storage order = unfilledOrders[
        orderId
    ];
    if (order.orderId != orderId) revert OrderNotFound();
  - if (block.timestamp < order.orderDeadline) revert OrderNotExpired();
    if (order.maker != msg.sender) revert MakerIsNotCaller();

3. Add a per user nonce towards the taker signatures used in mint() and expose a function that allows the user to invalidate nonces. Signing contracts, including PassiveLiquidityVault, must have a way to call that function as well.

PredictionMarketUmaResolver.assertionDisputedCallback() will be called by the UMA oracle when someone disputes an assertion. The resolver doesn't perform any state changes on dispute, it only emits the AssertionDisputed() event. However, the function is not properly guarded and anyone can call it, resulting in incorrect event emissions, making offchain listeners think the assertion was disputed when it was not.

Allow only the oracle to call the function

    function assertionDisputedCallback(bytes32 assertionId) external {
+       if (msg.sender != address(config.optimisticOracleV3)) {
+           revert OnlyOptimisticOracleV3CanCall();
+       }
        bytes32 marketId = umaSettlements[assertionId].marketId;

        // do nothing on disputes, just emit the event. We wait for the assertion to be resolved (truthfully or not) to close the loop.
        emit AssertionDisputed(marketId, assertionId, block.timestamp);
    }

PassiveLiquidityVault.approveFundsUsage() adds prediction markets to the activeProtocols set, but there is no way to remove a protocol from it.

Whenever _deployedLiquidity() is called, it will loop through all of the prediction markets, even if they are already not in use, increasing the gas usage and the risk of encountering an OOG exception.

Furthermore, if pm.getUserCollateralDeposits() reverts for any of these markets, all flows using deployedLiquidity() will experience DOS.

Consider adding a way to remove protocols from the set.

The PassiveLiquidityVault inherits the ERC4626 standard but does not follow it's rules.

Consider to follow the rules of the ERC4626 standard or remove the inheritance.

The normal requestWithdrawal function is still allowed if the emergencyMode is active. Depending on the emergency situation this may result in unexpected behavior or may even allow an exploit.

Consider to add the notEmergency modifier to the requestWithdrawal function.

The cancelDeposit and cancelWithdrawal functions do not have a whenNotPaused modifier and therefore it is not possible to stop them.

This could turn out bad, especially if there is a vulnerability in the cancelDeposit as it transfers funds to the user.

Consider to add whenNotPaused modifiers to these functions.

The notProcessingRequests does the same as the nonReentrant modifier and is therefore redundant.

Consider to remove the notProcessingRequests modifier

The onlyManager is not used and instead it's logic is written out in the process functions.

Consider to use the onlyManager modifier in the process functions.

Users can set any value as orderDeadline. If a user is not aware that the order can not be canceled before this timestamp passed and just uses the maximum uint all funds are permanently stuck if the order will not be matched.

Consider to validate the orderDeadline parameter against a maximum.

The PassiveLiquidityVault tries to overwrite and revert in all ERC4626 functions, but forgot about the convert functions:

• convertToShares
• convertToAssets

Overwrite the convert functions as they will DoS anyway because the totalAssets function reverts.

The PassiveLiquidityVault does not perform normal share price calculations on chain and instead a bot decides if the user's wished assets to shares ratio is allowed or not.

This allows anyone to DoS deposits with a front run, for example:

• The vault is empty
• Eve requests to deposit 100 assets and receive 100 shares in return
• The manager processes eve’s transaction
  • share price is now 100 / 100 = $1 per share
• Bob requests to deposit 100 assets and receive 100 shares in return (so in the current ratio)
• Eve front runs bob’s transaction and transfers 1 asset into the contract
  • Share price is now 101/100 = $1.01 per share
• As bob tries to buy shares in a 1:1 ratio (cheaper than they currently are) the manager will not accept bob’s order

The same can happen accidentally when the vault receives profit from a settled prediction.

Consider to make users aware of that so they always calculate with a realistic slippage value.

The toggleEmergencyMode function performs a critical state change but does not emit an event.

Consider to emit an event to follow best practices.

There is not function to update the approvedAsserters mapping after the deployment of the PredictionMarketUmaResolver.

Consider to add a function to be able to update it if needed.

The UMA oracle accepts a bond payment only if the given token is whitelisted.

if (cachedCurrencies[currency].isWhitelisted) return true;

PredictionMarketUmaResolver uses config.bondCurrency as bond token.

IERC20 bondCurrency = IERC20(config.bondCurrency);

The config is set only once during deployment. If the UMA oracle removes the asset from the whitelist, the resolver contract becomes unusable.

Add ownership feature and a function to change the config

The manager of the PassiveLiquidityVault is able to rug the PassiveLiquidityVault in multiple ways. For example by creating a request to withdraw all assets of the vault for 1 share and processing it, or by using approveFundsUsage to get all funds out of the contract.

This is a centralization risk that could have very bad consequences if the private key behind this EOA falls into the wrong hands.

Be aware of this risk and consider to add more checks in the process functions to limit the power of the manager.

For example by calculating the estimated share value with on chain conditions and comparing it with the deposited or withdrawn asset amount to make sure they are not completely off.

Or for example by limiting the amount of funds that can withdrawn in a given time span.

Limiting the power of the manager to not rug the vault in one transaction gives the owner a bit of time to pause the contract in an emergency situation.

In the case that a prediction can't be settled, for example as it's a bet on which sports team will win but the game is canceled. The prediction can't be burned and the funds of the users are permanently frozen.

Consider to add a deadline to predictions at which it can be canceled so that both parties get their funds back.

All comments talk about a default expiration time of 10 minutes, however it is set to 2 minutes instead, which may be too less.

Consider to update it to 10 minutes.

The MIN_DEPOSIT constant in PassiveLiquidityVault is hardcoded to 100e18. This is not sufficient for supporting wide range of tokens as different tokens have different value and decimals.

Consider having this variable configurable.

The PassiveLiquidityVault.requestWithdrawal() function requires the withdrawn shares to be more than MIN_DEPOSIT unless the users performs a full withdrawal

        if (shares < balanceOf(msg.sender) && expectedAssets < MIN_DEPOSIT)
            revert AmountTooSmall(expectedAssets, MIN_DEPOSIT);

An attacker can leverage this check to DOS honest user withdrawals. For example, if share:asset = 1 and Alice tries to withdraw 20e18 shares, she should receive 20e18 assets. Now Bob can frontrun her transaction with a 1 wei donation and make the transaction fail because Alice now has 20e18 + 1 shares.

Consider adding a function which always performs a full withdraw request and also accepts maxShares parameter to ensure the redeemed shares are not exceeding a given amount specified by the user.

During requestWithdrawal(), the shares being withdrawn are not burned or taken out from the account of the user. This allows the holder to transfer them after the request is created. By transferring the shares to different accounts, users can sort of bypass the InsufficientBalance() check and create a lot of pending requests.

When processWithdrawal() is invoked, the transaction will revert if the user balance is not sufficient. The users who know the shares can be transferred have an edge over the other participants in case a sudden price spike happens from the time the request was submitted until it's executed. They can just transfer the shares out of their account and leave the rest of the users bear a larger loss.

Furthermore, if the manager batches several calls to processWithdrawal() together such reverts can temporarily DOS the execution of other withdrawals.

Consider if shares must be locked when requestWithdrawal() is executed.

The MIN_DEPOSIT check during withdrawals can be bypassed by using different accounts. Users can transfer 1 wei of shares to many different accounts and request withdrawal from them. Because 1 wei is their whole balance, the check will be successfully bypassed and they will be able to spam the withdrawal queue.

Reconsider if the current check is good enough for your goals.

In approveFundsUsage() there is validation performed to ensure the vault utilization after funds are approved doesn't surpass the maxUtilizationRate

        uint256 newUtilization = ((deployedLiquidity + amount) * BASIS_POINTS) /
            totalAssetsValue;
        if (newUtilization > maxUtilizationRate)
            revert ExceedsMaxUtilization(newUtilization, maxUtilizationRate);

However, the real utilization increases only after funds are actually pulled from the contract. If a second approval to a different protocol is executed before the previous one pulled the funds, it's possible to use up all the funds. For example:

• Vault has 1000 assets
• approveForFunds() gives 800 approval to protocol A (80%)
• approveForFunds() gives 200 approval to protocol B (20%)

If there are no funds pulled in between the two calls, both of them will succeed and the utilization will end up being 100%, leaving the vault with no assets.

Furthermore, the arbitrary protocol parameter gives a lot of power to the manager as they can technically provide a malicious contract and drain the whole vault, which can be catastrophic if the manager account becomes compromised.

1. Include the allowance of each protocol towards the deployedLiquidity value as they are funds that can be pulled at any time.
2. Consider having either immutable protocols or at least make sure the caller of approveFundsUsage() is a proper multisig.

When setInteractionDelay() and setExpirationTime() functions of the PassiveLiquidityVault are executed, the storage variables interactionDelay and expirationTime are being changes.

When a request is created, the block.timestamp is saved in pendingRequests[user].timestamp and lastUserInteractionTimestamp[user]. On further interactions, expirationTime and interactionDelay are added towards these timestamp to ensure the needed delay has passed.

Modifying one of the two variables will affect all pending requests.

Consider adding the expirationTime and interactionDelay when creating the request or performing an interaction and then only check if the timestamp has passed.

When UMA resolves an assertion, the PredictionMarketUmaResolver updates the WrappedMarket as settled but does not set umaSettlements[assertionId].settled to true. Although on-chain logic currently does not use this flag, the stored struct can mislead off-chain indexers or future logic that might rely on it, creating inconsistent state.

if (assertedTruthfully) {
    market.settled = true;
    market.resolvedToYes = umaSettlements[assertionId].resolvedToYes;
    // Missing: umaSettlements[assertionId].settled = true;
}

Set umaSettlements[assertionId].settled = true in assertionResolvedCallback() when assertedTruthfully is true.

The maker collateral amount is checked to be >= minCollateral but the taker collateral is not.

Consider to check both the maker and taker collateral to be at least the minCollateral.

The system works with USDe as collateral but doesn't stake any of it and therefore misses a lot of yield.

Consider to stake USDe to be more capital efficient.

The manager in PassiveLiquidityVault will sign messages to deploy the vault liquidity to prediction markets. Changing the manager via setManager() will cause:

• all signed, but not yet executed messages to become invalid
• all previously signed messages by the new manager to become valid

Keep that in mind before using setManager()

There is unused code in multiple parts of the system.

Imports:

• Ownable and ISapienceStructs in the PredictionMarket contract
• IPredictionStructs in the PassiveLiquidityVault contract

Errors:

• PassiveLiquidityVault
  • OnlyOwner
  • InvalidIndex
  • ProcessingInProgress
  • InvalidCaller
• PredictionMarketUmaResolver
  • MarketNotDisputed
  • MarketNotOpen
  • InvalidCaller
  • MarketAlreadyWrapped
  • MarketNotSettled

Consider to use or remove unused code.

The PassiveLiquidityVault acts as something similar to a market maker in the sense that it takes on the opposite side of a bet.

As the quotes for the vault (market price) will always be a bit better than for the user and as the vault has open positions on both sides of many different predictions. It should sum up in the end and the vault should make profit under normal conditions.

However if the vault freely allows users to open bets with any liquidity amount the vault has available than there is a big risk of having too much exposure on a single bet or just a few bets.

For example if the vault owns $150k and someone creates a big bet that locks up$100k and the bet is lost than the vault lost a significant share of it's liquidity.

Be aware of this risk and make sure that the amount of liquidity the PassiveLiquidityVault provides to to a single bet is limited based on the amount of liquidity the vault owns.

Exposure always needs to be distributed enough or otherwise the vault could quickly lose a lot of it's funds with just a bit of bad luck.

During the kickoff call, it was mentioned that the vault is expected to work with any ERC20 token, but the current implementation doesn't support:

• tokens with decimals != 18 because of MIN_DEPOSIT
• tokens with fee on transfer features

Keep that in mind before deploying.

The MIN_DEPOSIT constant in PassiveLiquidityVault is a minimum amount of tokens that has to be requested during deposits and withdrawals. Currently the name suggests it will be only used on deposits.

Consider changing the constant to MIN_REQUEST_AMOUNT

Currently the utilization rate is represented in BASIS_POINTS = 1e4. In result, there is a precision loss of up to 0.01% happening.

You can use WAD instead of BASIS_POINTS for the utilization calculation to reduce the precision loss experienced.

At the end of PredictionMarketUmaResolver.resolvePrediction() it's checked if there are any unsettled markets.

        if (isValid && hasUnsettledMarkets && makerWon) {
            // No decisive loss found, but at least one market is unsettled
            isValid = false;
            error = Error.MARKET_NOT_SETTLED;
        }

The makerWon is unnecessary - it's always gonna be true because the only code path that sets it to false returns afterwards.

You can remove makerWon from the if condition.

The PassiveLiquidityVault.requestWithdrawal() function and PassiveLiquidityVault.requestDeposit() read the same storage-backed values multiple times within a single execution path. Specifically, pendingRequests[msg.sender] is accessed twice to read fields in both functions and balanceOf(msg.sender) is used three times in requestWithdrawal(). Each access incurs an SLOAD. Since these values do not change within the function prior to use, caching them in local variables would reduce gas usage.

Cache storage reads into local variables and reuse them. For example:

• uint256 bal = balanceOf(msg.sender); (for requestWithdrawal)
• PendingRequest storage req = pendingRequests[msg.sender]; (for both functions)

Then use bal and req consistently throughout the function.

Several state variables are initialized with default values at declaration and then set again in the constructor to identical values. This causes unnecessary SSTOREs and gas usage without changing behavior.

uint256 public maxUtilizationRate = 8000; // 80%
uint256 public interactionDelay = 1 days;
...
constructor(...) {
    ...
    manager = _manager;
    maxUtilizationRate = DEFAULT_MAX_UTILIZATION_RATE; // same as 8000
    interactionDelay = DEFAULT_INTERACTION_DELAY;      // same as 1 days
}

Remove the constructor assignments for maxUtilizationRate and interactionDelay if they already equal the DEFAULT_* values.

The custom error OnlyOwner(address caller, address owner) is declared in PassiveLiquidityVault, but never used in any revert paths. Unused custom errors add noise and bloat the bytecode.

Remove the unused error.

The PredictionMarket.sol file imports OpenZeppelin's Ownable, but the contract does not inherit from it nor reference any symbols from that file. Unused imports increase code noise and can mislead reviewers into assuming ownership controls exist.

Remove the unused import.

PredictionMarket._createPrediction() can be executed with an arbitrary resolver. This allows the usage of malicious resolvers which may revert on resolution or control who wins the prediction.

Make sure the takers, especially the vault, check the resolver before taking an order.

PredictionMarketUmaResolver.validatePredictionMarkets() doesn't check if the given market of an outcome has already been settled. This allows the creation of parleys with an already known outcome.

Consider validating if the market is settled

The manager has to pay gas to process requests but is not compensated to do so.

Consider to let users pay a gas fee to the manager.

The approveFundsUsage function tries to check if the utilization ratio after this approval exceeds the configured maxUtilizationRate.

However the calculation is only based on the total approvals to markets and excludes the already deployed liquidity (getUserCollateralDeposits).

Include the funds from the getUserCollateralDeposits calls in this calculation.

Approvals of the vault to the given prediction markets needs to be configured manually with the approveFundsUsage. As it is not adjusted automatically when users exit the system it is possible that unconfirmed assets are utilized.

For example:

• eve requests a deposits of 1000 assets
• the deposit is processed
• 80% so 800 assets are approved to the prediction market
• alice requests a deposit of 500 assets
• eve front runs the transaction and requests a withdraw of 999 assets
• alice deposit request goes through
• eve’s withdraw request is processed
• eve creates a prediction and 501 assets are pulled from the vault (possible as the approval still exists from when eve had 1000 assets in the vault)
• now all of alice funds are used but she has no shares in the vault which means if her deposit fails all of her funds were stolen / belong to eve now as she owns 1 share in the system

Consider to automatically decrease the utilization rate after a withdrawal if the available assets < the max utilization rate.

When the PredictionMarket NFTs are transferred between accounts, the _update() function calls to.supportsInterface() if the to address has code.

        if (addr.code.length == 0) {
            return false;
        }

        // Use ERC-165 standard interface detection
        try IERC165(addr).supportsInterface(type(IPassiveLiquidityVault).interfaceId) returns (bool supported) {
            return supported;
        } catch {
            return false;
        }

This allows any account to reject transfers, even if they were executed via transferFrom() and not safeTransferFrom(). There is a check for the size of the code of the account, however since EIP-7702, EOAs can have code associated with them to be executed when they are called. Therefore, any EOA or a contract can reject a transfer.

The supportsInterface() also gives the control flow to the to address even when transferFrom() is used, which may be unexpected, since most NFTs are not doing it.

Furthermore, _verifyTransfer() is executed before all changes in PredictionMarket._update() are applied. This can lead to loss of funds for users in some very specific edge cases. For example:

• Alice and Bob have 5000 collateral each
• they are respectively a maker and taker for 1000 collateral
• Prediction is settled and taker wins
• Alice transfers the maker NFT to Bob
• Bob uses the hook to call burn()
• burn() will reduce both of their collaterals with 1000
• the rest of the _update() function will reduce Alice's collateral with 1000 more and will add it to Bob

In result Alice lost her collateral twice.

Follow the CEI pattern in the _update() function and be very explicit in the documentation that transferFrom() gives the control flow to the recipient, so third party integrators are prepared for any related risks. As an additional safety measure, you can have the gas forwarded for the supportsInterface call capped.

There are unused errors in the PredictionMarketUmaResolver:

• MarketNotDisputed
• MarketNotOpen
• InvalidCaller
• MarketAlreadyWrapped
• MarketNotSettled

Consider to remove the unused errors.

The CEI pattern is not followed in the PredictionMarket.mint() function. It first checks the nonce, then calls isValidSignature() and finally updates the maker's nonce. While this should be generally safe, because isValidSignature() is called on the taker, it increases the risk of signature replays if some taker has non-standard implementation.

Consider updating the nonce immediately after checking it.

-       if (mintPredictionRequestData.makerNonce != nonces[mintPredictionRequestData.maker]) {
+       if (mintPredictionRequestData.makerNonce != nonces[mintPredictionRequestData.maker]++) {
            revert InvalidMakerNonce();
        }

As a fix for M-06 orders are now cancellable at any point in time, not only after they expire.

While this protects users from having their orders executed unexpectedly, the mint() function is still susceptible to that.

Acknowledge or add a fix for mint() as well.

When PassiveLiquidityVault.supportsInterface() is called for the IERC721Receiver interface, the result will be false because that interface is not included in the function, even though the vault implements it.

Consider including the interface in the supportsInterface() function.

The comment above PassiveLiquidityVault.decimals() says that an offset is added to the underlying asset's decimals, but that is not true as the function returns _underlyingDecimals unchanged.

     * @dev Decimals are computed by adding the decimal offset on top of the underlying asset's decimals. This
     * "original" value is cached during construction of the vault contract. If this read operation fails (e.g., the
     * asset has not been created yet), a default of 18 is used to represent the underlying asset's decimals.
     *
     * See {IERC20Metadata-decimals}.
     */
    function decimals() public view virtual override returns (uint8) {
        return _underlyingDecimals;
    }

Consider changing that part of the comment.

The code was updated in a way that when a deposit or withdrawal request is performed in the PassiveLiquidityVault, each field of pendingRequest is updated separately.

        request.user = msg.sender;
        request.isDeposit = true;
        request.shares = expectedShares;
        request.assets = assets;
        request.timestamp = block.timestamp;
        request.processed = false;

This version of the code uses 5 SSTORE compared to using only 1 with the previous one.

Consider storing everything at once.

The _update() function in PassiveLiquidityVault was overriden to prevent transfers of locked shares. However, there is a way for the user balance to drop below these locked shares during emergencies. For example:

• User has 1000 shares
• They request a withdrawal of 500 shares
• Emergency mode activates and they withdraw their whole balance
• Now their balance is 0, but the locked shares are still 500

The user would have to clear their withdrawal request manually when the contract is not paused.

No code changes needed, but would be good if it's documented.

The UtilizationRateUpdated event is emitted when setMaxUtilizationRate changes the maximum utilization or when approveFundsUsage() is called. These are two different use cases, but the event emitted is the same.

For example:

• Admin sets the utilization to 80%, therefore UtilizationRateUpdated(0, 80e18) is emitted
• 600 of 1000 funds are approved, so UtilizationRateUpdated(0, 60e18) is emitted

Notice that for the last emission, the second parameter is the projected utilization, not the actual one.

Consider separating the two use cases to emit different events.