Foil Updates

Proof of concept: PoC

Fee collectors can create under-collateralized positions and collateralize them using depositCollateral.

However, when calling increaseLiquidityPosition or decreaseLiquidityPosition, the zero collateral requirement for fee collectors causes updateCollateral to mistakenly remove and transfer all collateral back to the fee collector.

This allows fee collectors to withdraw collateral after depositing, potentially avoiding any loss at the end of the epoch. This is against protocol spec that the fee collector should never be able to back out of provided collateral, even if adjusting positions.

updateCollateral should not be triggered for fee collectors when modifying a position or position modification should be restricted during the epoch.

Foil Team: The issue was resolved in PR#155.

Proof of concept: PoC

When modifying a trade position, the output of a swap is used to calculate tradeRatio, which serves as a proxy price to determine the value of vGas. This ratio is essential for calculating PnL and setting the borrowed amounts for the new position.

However, if a small (dust) amount of vGas is swapped, amountIn or amountOut for vETH may round to zero due to Uniswap’s rounding behavior, causing tradeRatio to also be zero. This allows for potential exploitation: in a long position, borrowedVEth becomes zero, effectively wiping the position's debt and creating bad debt in the system.

Attack Scenario: 1. Alice opens a long position. 2. Alice decreases the position by 1 wei, setting tradeRatio to zero, which is below minPrice, creating bad debt by wiping all borrowedVEth. 3. Alice closes the position, recovering all previously deposited collateral plus additional funds, effectively stealing from the system.

If the trade price is below or above the min or max price for a pool, then revert.

Foil Team: The issue was resolved in PR#161.

Proof of concept: PoC

In LiquidityModule._closeLiquidityPosition, collected amounts are rounded up by adding 1 wei to offset Uniswap’s rounding when opening a position.

However, borrowed amounts may be zero (e.g., when adding liquidity outside the current price tick), and collected amounts can also be zero, depending on the price tick.

By adding 1 wei, users may withdraw more collateral than they initially deposited. Over time, this leads to the last user in an epoch being unable to withdraw due to insufficient collateral.

This behavior can also be exploited by malicious users with the following steps:

• Provide liquidity above the current price tick, so only vGas is borrowed and no vETH.
• Immediately decrease liquidity, collecting all borrowed vGas plus 1 wei of vETH.
• The 1 wei of vETH is added to the user's deposited collateral and then withdrawn.

1. If collected amount is zero, do not add the 1 wei adjustment.
2. Modify settlePosition to allow payouts of the contract's remaining balance when the exact

collateral amount is insufficient, preventing the last withdrawal from reverting if the balance is short by a few wei.

Foil Team: The issue was resolved in PR#150.

Proof of concept: PoC

When a trader is closing out a position, if the loss exceeds collateral deposited then this case is entered. As bad debt has been incurred, the collateral should be reduced to zero but currently depositedCollateralAmount remains unchanged.

The extraCollateralRequired would cover the losses, but however it is only taken into account if the trader is re-opening a new position.

So, If the trader was closing the position (i.e. size = 0), then all deposited collateral is returned to the trader implying losses are borne by the protocol/other LPs and traders.

Change the logic to:

if (collateralLoss > params.oldPosition.depositedCollateralAmount)
     output.position.depositedCollateralAmount = 0;
     extraCollateralRequired = collateralLoss - params.oldPosition.depositedCollateralAmount;

Foil Team: The issue was resolved in PR#164.

Proof of concept: PoC

When _quoteOrTrade is called and PnL is calculated, the tradeRatio experiences precision loss because of rounding down when performing divDecimal. While this is fine for longs, it's not for shorts.

That's because the tradeRatio is a fill price and if the fill price is lower, shorts will have made a profit. In result, when the PnL for shorts is calculated the trader will experience a smaller loss, leaving the system with fewer funds available than it should have in order to operate.

This can be most visible if a position has only borrowedVGas (short) and makes a trade to close the position. Because the entirety of the debt is being paid off, it would be expected that the vEthToZero would at least match the runtime.tradedVEth.

However, the vEthToZero would be slightly less due to the tradeRatioD18 rounding, and less collateral being held in the Foil contract. Later, when LPs try to close or settle their position, they will not be able to do so.

The contract will try to send them the amount they have earned, but this amount is not fully backed by the losses of the traders and the transaction will revert with ERC20: transfer amount exceeds balance.

In case of a short position, round the tradeRatio up to provide a worse fill price when going towards the long direction.

Foil Team: The issue was resolved in PR#168.

Proof of concept: PoC

When settling a liquidity position, getCurrentPositionTokenAmounts is called to retrieve the corresponding vGas and vETH token amounts of the position, which are then later rebalanced during position.settle.

The rebalancing process converts everything to ETH, adding all value to depositedCollateral and subtracting all debt from depositedCollateral.

However, the calculation during getCurrentPositionTokenAmounts rounds down, which can cause the total value of the position (including the collateral) to be less than the total debt in some cases.

This results in the settlement reverting due to an underflow in the following line: self.depositedCollateralAmount = self.borrowedVEth.

Rounding should be accounted for when calculating the required collateral. Consider adjusting loanAmount0 and loanAmount1 up by 1 wei during calculation.

Foil Team: The issue was resolved in PR#174.

Fee collectors can create under-collateralized positions and collateralize them using depositCollateral.

Currently, there are no restrictions preventing a fee collector from creating an oversized liquidity position, which can monopolize all available liquidity and hoard fees, preventing other fee collectors from benefiting.

Impose limits on the size of liquidity positions that fee collectors can create to ensure fair distribution of fees.

Foil Team: Acknowledged.

While creating a position in the liquidity or trading modules, _checkOnERC721Received function is called and then the position NFT is minted.

However, _checkOnERC721Received does not revert on failure but only returns false. Return value is not checked and positions can be minted to contracts that can't hold NFTs

Check the return value of the function before continuing.

Foil Team: The issue was resolved in PR#149.

Users provide deltaCollateralLimit when modifying their trade positions. While a positive deltaCollateralLimit indicates the maximum amount a user wants to provide to the protocol, a negative deltaCollateralLimit represents the minimum collateral amount a user wishes to receive from the protocol when decreasing or closing a position.

However, the negative case in _checkDeltaCollateralLimit is incorrect and behaves oppositely. It reverts when deltaCollateralLimit < 0 && deltaCollateral < deltaCollateralLimit. The user-provided value functions as a maximum limit instead of a minimum limit, resulting in the user receiving less than intended all the time.

Change deltaCollateral < deltaCollateralLimit to deltaCollateral > deltaCollateralLimit.

Foil Team: The issue was resolved in PR#159.

Currently, there is no mechanism that checks whether there is already an ongoing dispute or not while submitting a price. Asserter can submit a new price after an initial incorrect submission without waiting a dispute to resolve in 48-96 hours.

This would cause disputer to lose their bonds since the settlement will fail at this line as the assertionIds won't match.

Do not allow submitting new price if there is already an ongoing dispute.

Foil Team: The issue was resolved in PR#169.

Whenever a position is modified in position.updateValidLp, the required collateral for that position is calculated and compared against the current available collateral.

The collateral is calculated by using two values - debitEth and creditEth (debit is taken from the user and credit is given to them).

When removing a small amount of liquidity, it's possible that the decrease in creditEth is larger than the decrease in debitEth, which would lead to increased collateral requirements.

Since additionalCollateral is 0 when decreasing a position, the transaction will revert with InsufficientCollateral().

Allow the user to supply additional collateral when decreasing their position.

Foil Team: Acknowledged.

Proof of concept: PoC

Instead of opening a long position in the TradeModule to gain exposure to vGas, traders can use the LiquidityModule for a more efficient strategy.

By adding liquidity below the current price with a lower tick set to their desired entry price, traders can effectively create a limit order.

When the price reaches this minimum tick, the LP position converts fully to vGas, which can then be closed and transitioned into a Trade position.

Since LP positions have reduced collateral requirements (no swap fees nor price impact on entry), this approach allows for the same vGas position with less collateral.

Consider if this behavior should be prevented from a protocol perspective. One possible solution would be to fully close an LP's position in _closeLiquidityPosition instead of the transition to a Trade position, although low liquidity environments would have to be taken into consideration, and slippage protection would have to be appropriately handled.

Foil Team: Acknowledged.

If settlement.settlementPriceD18 in assertionResolvedCallback() is outside the acceptable price range for the given epoch, the new price for the epoch will be capped to either min or max with function setSettlementPriceInRange.

However, resolutionCallback() is still called with the original settlement.settlementPriceD18 and not the newly set price of the epoch. Whenever the settlement price is outside the acceptable range, the callback will receive an incorrect price.

The protocol team plans to create new epochs with that price which will lead to an epoch starting with prices outside the valid range.

Pass epoch.settlementPriceD18 instead of settlement.settlementPriceD18 to assertionResolvedCallback().

Foil Team: In Vault we use the resolution settlementPrice (not capped) to create the next epoch and compute new bounds.

When a new epoch is created by calling Epoch.createValid() two virtual tokens are deployed and used to create a new UniswapV3Pool.

The virtual tokens are deployed by the Epoch contract via the CREATE2 opcode. The owner of the Foil market will pass a salt parameter which will determine the address of the newly deployed tokens.

A malicious entity can frontrun the epoch creation transaction and use the salt passed in order to calculate the addresses of the two virtual tokens. These addresses can then be used to call UniswapV3Factory.createPool().

The pool for the two tokens will be created and when the Foil owner's transaction calls createPool(), it will revert because the pool already exists and the epoch won't be created. This frontrunning can be executed to stop any epoch creation.

Be sure to use a private network RPC when submitting the create transaction.

Foil Team: Partially Resolved.

The function sqrtRatioX96ToPrice is used to obtain price in several parts of the codebase. The issue lies with performing a square of two uint160 numbers which could overflow uint256. Overflow occurs when sqrtRatioX96 exceeds 2^128 - 1.

Perform >> 96 shift operation on sqrtRatioX96 first before doing square operation. Alternatively, use Uniswap's FullMath.mulDiv which handles the intermediate overflow case.

Foil Team: The issue was resolved in PR#168.

The Position.getRequiredCollateral() function is not used anywhere

Consider removing it if unnecessary

Foil Team: The issue was resolved in PR#168.

In quoteModifyTraderPosition, validateNotSettled is called redundantly twice. Additionally, the validateSettlementSanity function in Epoch.sol is unused and can be removed.

Remove the redundant validateNotSettled call and delete the unused validateSettlementSanity function.

Foil Team: The issue was resolved in PR#168.

The FeeCollectorNft is used to assert a user is a fee collector. Fee collectors are given special privileges that allow them to take under collateralized loans.

The FeeCollectorNft is transferable, and a malicious fee collector could take advantage of this to sell their FeeCollectorNft to users so they can take under collateralized loans, and jeopardize the health of the protocol.

Do not allow fee collectors to transfer FeeCollectorNfts.

Foil Team: Acknowledged.

The FullMath and TickMath libraries were adapted from Uniswap, which relies on overflow wrapping behavior available only in Solidity versions <0.8. Foil’s implementation targets Solidity versions >0.8.2, where unchecked arithmetic is not default.

Without wrapping these functions with unchecked, phantom overflows may occur, causing unexpected reverts when intermediate values exceed 256 bits.

For example, mulDiv(type(uint).max, type(uint).max, type(uint).max) would revert in Solidity >0.8 but return type(uint).max in older versions.

Wrap all relevant function bodies in unchecked to prevent phantom overflows. See the Uniswap v0.8 library implementation for reference:

https://github.com/Uniswap/v3-core/blob/0.8/contracts/libraries/FullMath.sol

Foil Team: The issue was resolved in PR#168.

When a settlement price is submitted, disputer is set as address(0). However, the disputer is never updated in the even if a dispute happens and will remain as address(0).

Consider removing disputer as it is never used in the codebase or update it by getting the address from the oracle contract.

Foil Team: The issue was resolved in PR#169.

Epoch.validateNotSettled() will revert with EpochNotSettled if the epoch has expired and is not settled. This is slightly inaccurate because the main reason the revert happens is because the epoch has expired.

Consider changing the error to EpochExpired

Foil Team: The issue was resolved in PR#168.

Currently, only the position owners can modify liquidity or trade positions. However, since positions are NFTs, users can assign operators or approve other addresses to manage their NFTs. An operator, even if authorized, cannot modify users' positions.

Additionally, the error message during the ownership check is NotAccountOwnerOrAuthorized, which implies that authorized addresses should be able to modify positions.

Consider allowing authorized addresses to modify positions.

Foil Team: The issue was resolved in PR#169.

quoteCreateTraderPosition() and quoteModifyTraderPosition() are both functions that are meant to be used to quote prices, however neither of them are marked as view functions. They cannot be marked as view functions because they use IQuoterV2.

Uniswaps documentation on IQuoterV2 states, “These functions are not marked view because they rely on calling non-view functions and reverting to compute the result. They are also not gas efficient and should not be called on-chain.”

This will lead to users having to pay gas costs if these functions are called.

Document to users that they should only call quoteCreateTraderPosition() and quoteModifyTraderPosition() off-chain.

Foil Team: Acknowledged.

The Natspec comment above the Epoch.getCollateralRequirementsForTrade function has a typo: “Gets the reuired collateral amount…”

Update the comment.

Foil Team: The issue was resolved in PR#168.

An Epoch is meant to have it's price bounded between its minPriceD18 and maxPriceD18. Upon creation, the owner passes a startingSqrtPriceX96 parameter to initialize the epoch's pool with. This price is not validated to be in the allowed range which allows a pool creation with invalid price.

Validate the startingSqrtPriceX96 variable.

Foil Team: Later will be done by the vault, so will be secure.

The function getPositionCollateralValue should return the current value of a position. However, in the current implementation it only accounts for gains and not losses, therefore returning an inaccurate value if it is a losing position.

Do not cap totalNetValue to a minimum of zero, and subtract any losses from depositedCollateral.

Foil Team: The issue was resolved in PR#170.

When an epoch is created, epoch.pool is assigned the IUniswapV3Pool value of the newly deployed pool. After that, epoch.pool is again casted to IUniswapV3Pool, which is redundant since it's already a variable of that type.

You can use epoch.pool without casting.

Foil Team: The issue was resolved in PR#168.

When modifying positions in the TradeModule, the Trade.swapOrQuoteTokensExactIn function is called, which subsequently calls the Uniswap swap router.

The swap router performs the swap within the Uniswap pool, and the pool then invokes the uniswapV3SwapCallback function of the router.

The uniswapV3SwapCallback function expects at least one of the delta amounts to be greater than zero. However, if the trade amounts are very small, the swap steps in the Uniswap pool can result in both delta amounts being zero, which causes uniswapV3SwapCallback to revert.

Therefore, it is possible for a user to create a small trade e.g. long 1 wei vGas, and then be unable to close it due to the swap amounts rounding down in Uniswap when calculating swap steps.

This will ultimately cause a revert within the uniswapV3SwapCallback: require(amount0Delta > 0 || amount1Delta > 0);. Consequently, a user has a position they are unable to close.

Consider implementing minimum trade sizes and/or documenting this behavior.

Foil Team: The issue was resolved in PR#154.

The onlyOwner modifier in ConfigurationModule should revert with the MarketNotInitialized() error if the owner of the market is not set.

However, the onlyOwner modifier first checks if the msg.sender is the current market owner and if they are not, the transaction will revert with OnlyOwner() error.

In the case where the market is not initialized, i.e owner == address(0), the revert reason will always be OnlyOwner(), since nobody can send a call from address(0). In result, the MarketNotInitialized() error will never be used.

Switch the order of the two ifs.

Foil Team: The issue was resolved in PR#168.

ConfigurationModule.initializeMarket() is used to create the market. It also mints the FeeCollector NFT to the fee collectors. If any of them don't support receiving NFTs or revert intentionally, the market won't be created.

Be aware of this situation. If this happens, you can call initializeMarket again, but this time without the specific fee collector.

Foil Team: Acknowledged.

Epoch.validateEpochParams() validates that the bondAmount should be a positive number. However, it should be at least as big as the result of the getMinimumBond() function of the UMA oracle. Otherwise, the assertions will not be accepted.

Make sure to pass a valid bondAmount when creating the market.

Foil Team: The issue was resolved in PR#172.

Market.getTickSpacingForFee() returns the Uniswap tick spacing associated with the given fee tier. The tick spacings are hardcoded, but the Uniswap Factory has a function enableFeeAmount() which allows the owner to change the fee tiers. If this happens, the Foil contracts may use stale data.

Be aware of the risk.

Foil Team: Acknowledged.

Fee Collectors opened LP positions at the beginning of an epoch and deposit collateral after they've earned fees. This collateral could be streamed in periodically or provided in bulk at settlement.

Due to the under-collateralized LP positions, traders may find themselves unable to exit profitable positions until Fee Collectors deposit collateral. As Fee Collectors are expected to hold large LP positions, this may affect a large group of traders.

This leads to temporarily locked funds and potential loss of yield for traders who are unable to close a profitable position promptly.

Consider implementing a minimum deposit amount for fee collectors. Or else, document this risk for users.

Foil Team: Acknowledged.

ERC721EnumerableStorage.tokenByIndex() reverts with a custom error if a non existent token was passed. It does so when index > totalSupply(). Because the index of allTokens starts from 0, this statement will not catch all possible cases.

For example, when totalSupply is 1, there is only 1 token in the allTokens array, at index 0 and index 1 is empty. If we call tokenByIndex(1) it won't enter the if statement and will revert with index out of bounds error instead of the custom error.

Change the condition to index >= totalSupply()

Foil Team: The issue was resolved in PR#169.

The comment // net vEth from oritinal positon minus the vEth to zero misspells “original”.

Correct the typo.

Foil Team: The issue was resolved in PR#168.

Epoch trade and liquidity activity is prevented once the block.timestamp >= self.endTime as can be seen in function validateNotSettled.

However, price submissions are restricted with block.timestamp > epoch.endTime within function validateSubmission.

When the block.timestamp == epoch.endTime, prices can be submitted since market activity is disallowed at that point, but submissions are restricted with current validation.

Adjust the validations appropriately within the UMASettlementModule or clearly document this behavior as this is extremely edgecase behavior.

Foil Team: The issue was resolved in PR#169.