Foil Vault

Users can create redemption requests for their vault shares using the requestRedeem function, which will increase the totalPendingWithdrawals variable. The only requirement regarding the request amount is that the users' balance must be sufficient.

Users’ shares are neither transferred nor burned at the creation of the request. Since these shares are transferable, a user can create a request using requestRedeem, transfer shares to another address, create another request, and repeat this process as many times as desired.

As a result, totalPendingWithdrawals will be inflated. This allows users to manipulate pendingSharesToBurn and totalSupply, or even cause a complete DoS in the system due to an underflow here in the _reconcilePendingTransactions function.

The redeem workflow should transfer tokens during the request creation process, similar to the deposit flow.

The requestRedeem function should transfer shares from the user to the vault. And then, the _redeemShares function should burn these shares from the vault instead of burning from the owner.

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

When submitMarketSettlementPrice is called in Vault.sol, the vault is set as the asserter in the UMA oracle. Upon successful settlement of the assertion price, the bond is returned to the vault.

However, there is no mechanism to refund this bond to the user who submitted the price and paid for it. Additionally, there is no recovery function, causing the bond to remain permanently stuck in the vault.

In UMASettlementModule.submitSettlementPrice, allow the caller to specify an address to be set as the asserter. Then, in Vault.submitMarketSettlementPrice, ensure the caller’s address is passed as the asserter to enable proper bond refunds.

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

The recommendation of H-03 is to round up the trade ratio when going towards the long direction as a short. However, the fix implemented is the opposite - the trade ratio is being rounded down if isLongDirection and rounded up otherwise. Since the problem is not solved, the insolvency issue still exists.

Currently, tradeRatioD18 is used to compute both closePnL and vEthAmount/borrowedVEth. Foil's goal should always be to maximize borrowedVEth and minimize closePnL and vEthAmount. Because of this, different rounding directions should be used depending on what's being calculated.

The end goal should be to maximize the borrowedVEth and minimize the vEthAmount and closePnL. To accomplish this, you can have two different tradeRatios - one rounded down and one rounded up. You will also have three different vEthToZero.

The first one will be to calculate the closePnL and you will use the tradeRatio that’s rounded down if the position is a long, otherwise use the rounded up one. The second vEthToZero will always use the tradeRatio that’s rounded down and the third vEthToZero will always use the tradeRatio that’s rounded up.

Next, you will also have two different vEthFromZero for each vEthToZero. Finally, in the if/else statement where you set borrowedVEth and vEthAmount you will choose the appropriate vEthFromZero.

For the if case you should use the vEthFromZero which absolute value is bigger to maximize borrow and for the else case you should use the vEthFromZero which absolute value is smaller to minimize the credited vETH.

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

Proof of concept: PoC

When a new epoch is created, the Vault uses the assets in its reserves to deposit them as collateral in order to create a liquidity position. The vault will call quoteLiquidityPositionTokens to get the amount0 and amount1 that can be added as liquidity for the available collateral.

However, Epoch.requiredCollateralForLiquidity() now adds 1 to loanAmount0 and loanAmount1. This means the actual required collateral for the position may exceed the available collateral in the vault.

In result, the transaction will revert because of InsufficientCollateral() and the epoch creation will not be successful. This issue can occur with non-trivial amounts, for example 1e17.

Consider implementing higher minimum collateral amounts and documenting this behavior for clarity. Another option to consider is subtracting 1 wei from amount0 and amount1 when creating the LiquidityMintParams, which should account for the additional 1 wei.

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

Proof of concept: PoC

When a position is operating with small amounts, the required collateral for the position can be calculated to be zero due to rounding when calculating value of debt. Consequently, a user can modify their position to a size within a couple thousand wei and have to provide zero collateral.

All their prior deposited collateral would be returned, and their position would have no backing. In the original review, this issue was not possible since the minimum requiredCollateral was always at least 2 wei.

Have a minimum required collateral.

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

Since the Vault contract will be executing the onlyOwner createEpoch() function, it will be set as the owner of the foil system. The ConfigurationModule.updateMarket() function can be called by the Foil owner to update the market parameters.

However, this function is never called in the Vault contract. This means the market can never be updated once the ownership is transferred to the contract. There is also no call to transferOwnership in Vault, so you can't just use a new vault as the owner.

Add calls to updateMarket and transferOwnership in the vault.

Foil Team: We are keeping everything immutable.

Deposits before the first epoch are possible, with a minimum deposit amount of 1e3. Any pending deposits before the first epoch are utilized to establish the initial liquidity position within the _createNewLiquidityPosition function.

This function deducts a dust amount of 1e4 from the deposited collateral amounts. If a user intentionally deposits an amount between 1e3 and 1e4 before the first epoch, and there are no other deposits, the initialization will fail due to underflow at this line.

Consider setting the minimum deposit amount higher than the dust. Alternatively, keep the codebase unchanged but externally deposit the difference if this situation occurs.

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

After the implementation of the Vault, epoch settlements and the creation of the new epoch happens at the same transaction via callbacks. Because of this atomic behavior, failure of the pool creation for the next epoch will DoS the settlement of the previous epoch.

An attacker can precompute the virtual token addresses and create the Uniswap pool with these addresses as creating pools is permissionless in the Uniswap. This will cause Epoch.createValid function to revert while calling IUniswapV3Factory.createPool due to require(getPool[token0][token1][fee] == address(0)) check in the factory.

The attack can cause complete blocking of the epoch settlements and creations. However, attackers must keep frontrunning and create new pools every time someone tries to settleAssertion in the optimistic oracle.

Check whether the pool already exists or not by calling the getPool in the factory instead of directly calling the createPool. If the pool already exists, check whether it was already initialized or not and set the starting price. Alternatively, always make sure to use a private RPC to prevent frontrunning.

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

When a new epoch is created, block.timestamp is used as the salt for generating two virtual tokens. In _createVirtualToken, a loop probes for an available salt if a collision occurs.

However, the salt increments by 1 on each iteration, making it highly predictable and susceptible to front-running. An attacker can exploit this predictability to deliberately create collisions.

During testing, each iteration of the loop was found to cost approximately 600k gas, making it feasible for an attacker to force the epoch creation process to fail due to an Out-of-Gas error.

Consider using a less predictable and more robust mechanism for generating the salt, such as hashing with block variables. Alternatively, consider using CREATE3 which ensure that the address is only dependent on deployer and salt.

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

When a position is operating with small amounts, the required collateral for the position can be calculated to be zero due to rounding when calculating value of debt. Consequently, a user can modify their position to a size within a couple thousand wei and have to provide zero collateral.

All their prior deposited collateral would be returned, and their position would have no backing. In the original review, this issue was not possible since the minimum requiredCollateral was always at least 2 wei.

Have a minimum required collateral.

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

M-01 of the previous audit was not addressed. 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.

Proof of concept: PoC

The M-05's recommendation to let the user specify an amount of collateral to be added when decreasing a liquidity position has not been implemented which leaves the problem unsolved.

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

Foil Team: Letting users know to decrease by larger than a few wei is acceptable due to this rounding issue.

Proof of concept: PoC

When closing a position, the vEthToZero is calculated as initialSize * tradeRatio and should be equal to the signedTradedVEth. However, Solidity division truncates the result. Because of this, tradeRatio will be slightly off - both when rounded down or up - therefore vEthToZero as well.

Even though vEthFromZero should be roughly equal to targetSize * tradeRatio, the value assigned to it (for targetSize = 0) will be non-zero - positive or negative depending on the rounding. After that the absolute value of vEthFromZero will be assigned to vEthAmount.

In result, closed positions end up having positive vEthAmount, which is especially bad for long positions. This ultimately leads to an undercollateralized market, preventing the last user from settling.

Consider setting the vEthAmount of the new position to 0, if its size is 0 as well.

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

Function _createEpochAndPosition passing is critical to the Vault's flow, since if the resolutionCallback fails the Vault's functionality is stopped. If the Vault has more collateral than the current minimum collateral, the Vault attempts to _createNewLiquidityPosition.

The issue is that even with enough collateral to meet the minimum threshold, is it not guaranteed that the liquidity to-be minted from the calculated amount0 and amount1 is greater than 0 due to Uniswap rounding down on small amounts, which would trigger a revert in UniswapV3Pool.mint: require(amount > 0);

Ultimately, the Vault will attempt to mint which will revert, causing the callback to fail and the mints/epoch creation will not occur.

Consider enforcing a higher minimum collateral.

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

Proof of concept: PoC

When a long position is being modified, its borrowedVEth is set to the absolute value of vEthFromZero. In some cases, it's possible to have a small amount of vGasAmount with 0 vEthFromZero due to the traded vETH matching the vEthToZero, primarily when operating with small position sizes and trade prices.

This leads to a long position that does not have a loaned amount. This leaves the Foil contract with less available collateral than it should have and in result, the last user will not be able to exit.

Validate that any opened long position has positive borrowedVEth: require(borrowedVEth > 0)

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

Proof of concept: PoC

When a user calls withdrawRequestRedeem(), if their balance after is less than minimumCollateral then resetTransaction() will set their pending amount to zero. However, totalPendingWithdrawals will only be decremented by the amount of shares the user passes in.

This will lead to totalPendingWithdrawals being larger than the actual amount that is intended to be withdrawn. A malicious user could continuously call requestRedeem() in conjunction with withdrawRequestRedeem in order to inflate totalPendingWithdrawals to be larger than collateralFromPreviousEpoch plus totalPendingDeposits.

This will cause a DoS via underflow when _reconcilePendingTransactions() is called. Additionally, when a user calls withdrawRequestDeposit(), totalPendingDeposits is only decremented by assets. This will lead to the user’s remaining tokens to be donated to other users of the protocol.

If the user’s remaining amount is less than the minimumCollateral, then decrement totalPendingWithdrawals by the full amount or refund the remainder of their balance before calling resetTransaction(), depending on if it is a redeem or deposit.

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

When updating share price after an epoch, if no collateral was received, the share price is set to 1e18. This creates a significant issue as depositors can redeem their entire collateral even though no collateral was received after closing the liquidity position.

Effectively, this allows depositors to withdraw funds that belong to the next epoch's depositors, who have already transferred their collateral into the contract.

Initially, setting sharePrice to 0 instead of 1e18 was considered. However, this would affect the minting of new shares for the next epoch.

As a solution, if no collateral is received, set the sharePrice for the current epoch to 0 while ensuring the sharePrice for the next epoch is reset to 1e18.

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

Guardian Team: The issue was not fixed. The price of the epoch is hardcoded to 1e18 if no collateral is received.

Foil Team: Let’s halt the vault and allow a request deposit of something higher than 1e8 which will fix this.

_calculateTickBounds() uses modulus 200 in order to set the target tick value to the closest acceptable tick range. However, Foil is compatible with multiple fee tiers, but the value 200 is not.

For instance, the 0.3% fee tier uses a tick spacing of 60, which is not a divisor of 200. This will cause a revert when attempting to create the epoch.

Instead of hardcoding 200, use the appropriate value for the fee tier of the pool.

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

In _calculateNextStartTime, if there is a significant delay in resolving an epoch, the vault skips an entire vaultCycleDuration to maintain synchronization with other vaults in the circuit.

However, if only a single vault exists in the circuit, this synchronization is unnecessary. Skipping vaultCycleDuration in this scenario causes unnecessary downtime where no vaults are available.

Introduce a condition to check if only one vault exists in the circuit. In such cases, avoid skipping the vaultCycleDuration and instead start the next epoch immediately after resolution.

Foil Team: Acknowledged.

There is a comment left on L-02 that Foil now uses OpenZeppelin's code for its calculations, but the code in DecimalPrice is not changed - it's still possible for the result of the multiplication to exceed 2^256-1

Fix the issue.

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

In the Vault contract, only the owners can request deposits and redemptions. However, claiming of these requests are external and anyone can claim on behalf of the owner.

Even though the owner created these requests, timing of the claim might matter for the owner and these actions should be access controlled.

Not allow other users to claim on behalf of owners.

Foil Team: I don’t think there’s any advantage to claiming after the epcoh is settled, if anything, these functions not being gated gives us flexibility to force redemptions to clear any pending txns.

totalVaults is stored as an immutable variable when Vault.sol is created. This implies that no new vaults can be added after the first batch of vaults. This may run counter to protocol design that new collateral types may be added.

Consider allowing for new vaults to be added.

Foil Team: At least the plan right now is not to add any more vaults once a vault is initialized.

Vault.requestRedeem requires the amount of shares being redeemed to be greater than minimumCollateral. However, minimumCollateral is denominated in assets, not shares.

Consider having different validation with the proper denomination.

Foil Team: Maybe a rename of the variable would be better. Will do that.

As pointed out in this issue, anyone can dispute rightful assertions to delay the start of a given epoch by paying the bond of $5000.

Since now anyone can submit a price, the same entity can assert a rightful price and dispute it at the same time towards the end of the assertionLiveness period.

By doing so, they will receive half of their disputer bond. In result, the cost of the attack will be reduced from $5000 to $2500.

Be aware of the reduction in cost.

Foil Team: Acknowledged.

The last user attempting to settle their position may be unable to do so if: market.collateralAsset.balanceOf(address(this)) < withdrawableCollateral.

This discrepancy can occur due to minor rounding errors during trade or liquidity activities, leaving the contract balance short by a few wei. As a result, the user cannot fully recover their collateral.

If market.collateralAsset.balanceOf(address(this)) is less than withdrawableCollateral, consider transferring the remaining contract balance to the user instead.

This ensures the user can recover as much of their collateral as possible without leaving residual funds in the contract.

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

With the implementation of the Vault contract, the settlement of the previous epoch and the creation of the next epoch happen in a single transaction.

Because of this, an unexpected failure at any step of the process (e.g., settlement, new epoch creation, quoting, or adding new liquidity) may cause the system to halt.

Consider implementing mechanisms like try/catch blocks along the transaction flow, allowing unexpected issues to be resolved externally and ensuring the system remains operational.

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

When closing a liquidity position, it can turn into a Trade position if it cannot be repaid. If the amount left for the new Trade position is less than the minTradeSize, the owner of the position will not be able to directly close it.

They will have to make a bigger trade and close if after that. By doing so, they suffer losses because of price impacts.

Be sure to warn the users of Foil about this case.

Foil Team: Acknowledged.

L-20 of the previous audit was not addressed. 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.

Epochs in Foil have startTime. However, liquidity and trades for a given epoch can be executed as soon as the epoch is created, no matter its startTime.

Be aware of this behavior.

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

"Previous deposit request is not in the same epoch" message in the withdrawRequestRedeem function (L642) should be "Previous withdraw request is not in the same epoch".

Change the error string in the require statement.

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

Vault contract has pendingDepositRequest and pendingRedeemRequest functions. However, these functions do not check the transaction type of the pending request and directly return userPendingTransactions[owner]. pendingDepositRequest function can return a redeem request and vice versa.

Consider checking the transaction type in these functions, or implement a single function (e.g. pendingRequest) for all transaction types.

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

A minTradeSize configuration has been added to the Market in response to L-15. This works fine for createTraderPosition, but it's wrongly implemented in modifyTraderPosition. It calls _checkTradeSize(size) to ensure the trade size is bounded.

However, the argument passed is size (the final size), not deltaSize. Because of this, small trades (below the minTradeSize) will still be successfully executed.

Pass deltaSize instead of size to _checkTradeSize to ensure trades are beyond a minimum delta.

Furthermore, consider also validating the resulting size of the position, such that situations do not arise where a user creates a large position, decreases by position size-1, such that the delta trade size is large enough but the final position size is 1 wei.

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

Proof of concept: PoC

Because the fee collector is not required to deposit collateral for their position, situations can arise where fee collectors close their liquidity position yet the the resulting position will become a Trade position with non-zero borrowed amounts but zero credit amounts.

This is because fee collectors will typically enter the following case if (position.depositedCollateralAmount < collateralDelta) due to no collateral requirements which will set a non-zero borrowedvETH.

Consequently, an unbacked Trade position may be created that cannot be directly decreased and closed, since the deltaSize would be zero and Errors.DeltaTradeIsZero() would be triggered.

Clearly document this behavior and even consider if FeeCollectors should close their LP positions before epoch settlement as this allows them to profit without ever depositing collateral.

Foil Team: Acknowledged.

During epoch creation, in _calculateTickBounds positive ticks are rounded down but negative ticks are rounded up, which could lead to unexpected behavior.

Consider rounding down negative ticks for consistency or clearly documenting this behavior.

Foil Team: Acknowledged.

Multiple functions in the Vault are not EIP compliant.

• totalAssets: Must not revert. However, it can revert if the positionId is not valid.
• convertToShares: Must not revert. However, it can revert if totalAssets == 0.
• preview functions: Must be as close as possible to on-chain conditions and must not revert based

on vault specific user/global limits. May only revert that would also cause mint/withdraw etc. to revert too. However, these function are not supported at all.

• deposit: Mints shares by depositing exactly assets amount. However, the amount is ignored in the

codebase.

• mint: Mints exactly the shares amount. However, the amount is ignored in the codebase.
• withdraw: Burns shares and sends exactly the assets amount. However, the amount is ignored in

the codebase.

• redeem: Burns exactly the shares amount. However, the amount is ignored in the codebase.

The contract incorrectly signals supporting the ERC4626 interface with the supportsInterface function.

One option is trying to make the contract EIP compliant. However, based on what the contract wants to achieve, it might be best to not support ERC4626.

Consider removing interfaceId == type(IERC4626).interfaceId line from the supportsInterface function to prevent incorrect signaling for the external integrators.

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