Description

When a user calls addToDepositQueue(), they are required to pass the address of a receiver as the second argument. The function pulls an amount of asset tokens from the receiver via the use of safeTransferFrom():

function addToDepositQueue(uint256 amount, address receiver) public {
    require(isDepositQueueOpen, "500:NotOpen");
    queuedDepositsCount += 1;
    queuedDepositsTotalAmount += amount;
    require(queuedDepositsTotalAmount + sumVaultUnderlyingAmounts <= maxDepositAmountLimit, "500:TooBig");

    IERC20(asset).safeTransferFrom(receiver, address(this), amount);
    depositQueue.push(Deposit({ amount: amount, receiver: receiver }));
    emit DepositQueued(receiver, amount);
}

This implies that the receiver must preapprove the FCNProduct contract, as the safeTransferFrom() will revert otherwise. Generally, the approval amount is set to the maximum uint256 value. This introduces a vector through which an attacker can deposit more assets on behalf of the receiver at a later point in time.

Impact

Consider the following scenario:

1. The victim decides they want to invest 1,000 USDC into an FCN product.

2. The victim max approves the FCNProduct contract and uses addToDepositQueue() to invest 1,000 USDC. They have no intention of investing more than this amount.

3. Some amount of time later, the attacker notices that the victim's wallet has been transferred 50,000 USDC from elsewhere.

4. The victim plans to use this USDC for other things, but the attacker now calls addToDepositQueue() with receiver set to the victim's address.

5. Since the victim has already approved the FCNProduct contract, this deposit will go through, and now the victim is at risk of losing a part of this money.

The impact here is that the victim is griefed by the attacker. The attacker may or may not benefit from depositing funds on the victim's behalf, but the victim now stands to lose this money if, for example, the vault experiences a knock in event (a downside protection for user-deposited capital). There is also no way for the victim to cancel their deposit while it is in the deposit queue.

We have noted that addToWithdrawalQueue() uses a similar pattern. However, we do not believe a similar attack vector exists there.

Recommendations

We recommend removing the use of the receiver argument and instead pulling funds directly from msg.sender.

Remediation

The client has acknowledged and fixed this issue by removing the receiver parameter and using msg.sender instead. This was fixed in commit 4a95e773↗.