Lack of input validation announceStableDeposit function

Summary

In the announceStableDeposit function, the depositAmount is the amount of collateral to deposit. However, there is no check to ensure that the depositAmount is greater than zero. This could lead to unexpected behavior if the function is called with a depositAmount of zero or less.

Vulnerability Detail

function announceStableDeposit(
    uint256 depositAmount,
    uint256 minAmountOut,
    uint256 keeperFee
) external whenNotPaused {
    uint64 executableAtTime = _prepareAnnouncementOrder(keeperFee);
    vault.checkCollateralCap(depositAmount);
    if (depositAmount < MIN_DEPOSIT)
        revert FlatcoinErrors.AmountTooSmall({amount: depositAmount, minAmount: MIN_DEPOSIT});
    // Check that the requested minAmountOut is feasible
    uint256 quotedAmount = IStableModule(vault.moduleAddress(FlatcoinModuleKeys._STABLE_MODULE_KEY))
        .stableDepositQuote(depositAmount);
    if (quotedAmount < minAmountOut)
        revert FlatcoinErrors.HighSlippage(quotedAmount, minAmountOut);
    _announcedOrder[msg.sender] = FlatcoinStructs.Order({
        orderType: FlatcoinStructs.OrderType.StableDeposit,
        orderData: abi.encode(
            FlatcoinStructs.AnnouncedStableDeposit({depositAmount: depositAmount, minAmountOut: minAmountOut})
        ),
        keeperFee: keeperFee,
        executableAtTime: executableAtTime
    });
    // Sends collateral to the delayed order contract first before it is settled by keepers and sent to the vault
    vault.collateral().safeTransferFrom(msg.sender, address(this), depositAmount + keeperFee);
    emit FlatcoinEvents.OrderAnnounced({
        account: msg.sender,
        orderType: FlatcoinStructs.OrderType.StableDeposit,
        keeperFee: keeperFee
    });
}

Impact

In the above code, the depositAmount is used directly without any validation. If a user were to call this function with a depositAmount of zero, the function would still execute, potentially leading to unexpected results.

For example, the depositAmount is used to transfer collateral from the user to the contract. If the depositAmount is zero, no collateral would be transferred, but the function would still execute and the order would still be announced. This could lead to a situation where an order is announced but no collateral is actually deposited, which could cause issues when the order is executed.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L67

Tools Used

Manual Review

Recommendations

To prevent this, the function should include a check to ensure that the depositAmount is greater than zero before proceeding. This could be done with a simple require statement at the beginning of the function:

require(depositAmount > 0, "Deposit amount must be greater than zero");

This would cause the function to revert if the depositAmount is not greater than zero, preventing the rest of the function from executing and avoiding the potential issues described above

sherlock-admin commented 5 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid

nevillehuang commented 4 months ago

Invalid, it will revert here

sherlock-audit / 2023-12-flatmoney-judging

cheatcode - Lack of input validation announceStableDeposit function #253