Description

Producers are not able to cancel and recover funds on queued orders using modifyExistingOrder(...) for cross-asset pools. Calling submitProducer(...) always sets shouldFarm = false and order.depositedShares > 0 using the oracle price:

if(shouldFarm) {
    IERC20(asset).safeTransferFrom(msg.sender, address(this), amount);
    order.depositedShares = IERC4626(vaultAdapter).deposit(amount, getAddressForPool(poolId)) / order.packetsRemaining;
} else {
    IERC20(asset).safeTransferFrom(msg.sender, getAddressForPool(poolId), amount);
}

However, modifyExistingOrder(...) has missing checks and assumes the orders were deposited in the vault asset instead of the pool asset:

if (order.depositedShares > 0) {
    getWalletForPool(poolId).withdrawFromVault(amountTokens, msg.sender, vaultAdapters[pool.vault]);
} else {
    getWalletForPool(poolId).withdraw(amountTokens, pool.asset, msg.sender);
}

The attempt to withdraw from the vault asset from the pool wallet will fail. Fortunately, there are no vault assets in the pool wallet to exploit because all vault assets are sent to the FNFT wallet (ResonateSmartWallet) when orders are matched. However, a producer would not be able to retreive the funds of their order.

It should be noted that attempting to fix this bug by only directing modifyExistingOrder(...) to retreive the pool asset instead of the vault asset will result in a critical exploit. This is because submitProducer(...) accounts for the price of the vault asset while modifyExisitngOrder(...) does not.

For example, the producer deposits amount of pool assets and gets credited packets equal to amount/ producerPacket:

...
sharesPerPacket = IOracleDispatch(oracleDispatch[vaultAsset][pool.asset]).getValueOfAsset(vaultAsset, pool.asset, true);
producerPacket = getAmountPaymentAsset(pool.rate * pool.packetSize/PRECISION, sharesPerPacket, vaultAsset, vaultAsset);
...
producerOrder = Order(uint112(amount/ producerPacket), sharesPerPacket, msg.sender.fillLast12Bytes());

Through getAmountPaymentAsset(...) the producerPacket scales linearly with the vault price. However, if the producer tries to later modify their order, there is no adjustment from the number of packets to the amount of pool asset:

...
if (isProvider) {
    providerQueue[poolId][position].packetsRemaining -= amount;
  } else {
    consumerQueue[poolId][position].packetsRemaining -= amount;
}
...
uint amountTokens = isProvider ? amount * pool.packetSize * pool.rate / PRECISION : amount * pool.packetSize;

If vault price > 1 the producer will not be refunded a sufficient amount of assets for the reduction in packets. This is because submitProducer(...) scales down the packets by the vault price, while modifyExistingOrder(...) does not commensurately scale up the amount of pool asset per packet.

If vault price < 1 the producer will be refunded an excessive amount of assets for the reduction in packets. This is because submitProducer(...) scales up the packets by the vault price, while modifyExistingOrder(...) does not commensurately scale down the amount of pool asset per packet.

Impact

Order cancelling for producers would be nonoperational.

Recommendations

The following changes should be made to modifyExistingOrder(...): (1) withdrawl the pool asset for cross-asset producer orders and (2) use the price of the vault asset at the time the order was submitted to correctly calculate amountTokens.

Remediation

This finding was remediated by Revest in commit fc3d96d91d7d8c5ef4a65a202cad18a3e86a3d09.