Description

Investors are able to deposit assets into an FCNVault through the FCNProduct contract's addToDepositQueue() function. This function pulls funds from the investor's wallet and adds Deposit objects into a global depositQueue array within the FCNProduct contract.

Subsequently, a trader admin is able to call processDepositQueue() to process these Deposit objects inside the depositQueue. On a high level, the processDepositQueue() function does the following:

1. Loops over the depositQueue a maximum of maxProcessCount times, or until it is empty.

2. For each deposit, it tracks the amount being deposited in the vault's metadata storage, accessed using the passed-in vaultAddress.

3. It calls the vault's deposit() function with the deposit amount and receiver address. This will send share tokens to the receiver.

4. If the depositQueue is empty afterwards, it will delete the queue.

5. Otherwise, it will shift over all remaining deposits in the queue to the beginning of the queue.

Now, there are only two checks in processDepositQueue() that are used to determine whether the vaultAddress that is passed corresponds to a valid, usable vault. They are as follows:

function processDepositQueue(address vaultAddress, uint256 maxProcessCount) public onlyTraderAdmin {
    FCNVaultMetadata storage vaultMetadata = vaults[vaultAddress];
    require(vaultMetadata.vaultStatus == VaultStatus.DepositsOpen, "500:WS");

    FCNVault vault = FCNVault(vaultAddress);
    require(!(vaultMetadata.underlyingAmount == 0 && vault.totalSupply() > 0), "500:Z");

    // [...]
}

These two checks are not enough. For example, if the trader admin deploys a malicious vault contract, then they can bypass both checks by doing the following:

1. Calling openVaultDeposits() with the address of their malicious vault contract.

2. Ensuring that their malicious vault contract contains a totalSupply() function that returns a value greater than zero.

function openVaultDeposits(address vaultAddress) public onlyTraderAdmin {
    FCNVaultMetadata storage vaultMetadata = vaults[vaultAddress];
    vaultMetadata.vaultStatus = VaultStatus.DepositsOpen;
}

After this is done, both of the checks will pass, and the code will treat the malicious vault as a valid FCNVault contract.

Impact

A malicious trader admin can steal investors' funds using the following steps. The funds being stolen here come out of funds that are currently awaiting to be deposited.

1. Set up a fake malicious vault contract as described in the previous section with an empty deposit() function and a maliciously crafted redeem() function (see further below).

2. Wait for investors to add deposits into the depositQueue.

3. Call processDepositQueue() with the malicious vault address as many times as needed to process all deposits in the queue. This sets the vault's status to NotTraded.

4. Call setTradeData() with the _tradeExpiry set to a time in the past.

5. Call sendAssetsToTrade() to send the deposited assets to the market maker. This sets the vault's status to Traded.

6. Call calculateCurrentYield() with the malicious vault address. This will set the vault's status to TradeExpired.

7. Call calculateVaultFinalPayoff() with the malicious vault address. This will set the vault's status to PayoffCalculated.

8. Call collectFees() with the malicious vault address. This will set the vault's status to FeesCollected.

9. Queue a withdrawal to a trader admin--controlled wallet address using the addToWithdrawalQueue() function. Any amountShares is fine here.

10. Call processWithdrawalQueue(). This function ends up calling the vault's redeem() function to determine how many assets to return to the receiver of the withdrawal.

11. Since this is the Trader Admin's malicious vault contract, all they need to do is ensure that the malicious redeem() function returns balanceOf(address(fcnProduct)) for themselves and 0 for all other receivers.

After the final step, all asset tokens in the FCNProduct contract will be transferred out to the wallet address specified in step 9.

Recommendations

In receiveAssetsFromCega(), we see the following:

function receiveAssetsFromCegaState(address vaultAddress, uint256 amount) public {
    require(msg.sender == address(cegaState), "403:CS");
    FCNVaultMetadata storage vaultMetadata = vaults[vaultAddress];
    // a valid vaultAddress will never have vaultStart = 0
    require(vaultMetadata.vaultStart != 0, "400:VA");

    IERC20(asset).safeTransferFrom(msg.sender, address(this), amount);
    vaultMetadata.currentAssetAmount += amount;
}

This same check should be added to processDepositQueue() and all other places where a vaultAddress is passed in as an argument. This will prevent invalid vault contract addresses from being used in the contract.

Remediation

The client has acknowledged and remediated this issue by adding an onlyValidVault modifier that guarantees that the vaultAddress argument passed to all required functions is valid. This was done in commit f64513a9↗.