cheatcode - Potential for Loss of Funds in `announceLeverageOpen`

[sherlock-admin2]

cheatcode

high

Potential for Loss of Funds in announceLeverageOpen

Summary

The potential for loss of funds in the announceLeverageOpen function arises from the order of operations within the function, particularly the transfer of tokens before all necessary checks are completed to ensure that the order can be executed.

Vulnerability Detail

The function transfers tokens from the caller to the contract before it has confirmed that the order can be executed. This is done with the safeTransferFrom function call:

    vault.collateral().safeTransferFrom(msg.sender, address(this), margin + keeperFee + tradeFee);

After the tokens are transferred, the function performs several checks to determine if the order is valid. These checks include verifying the maximum fill price, checking leverage criteria, and ensuring that the position does not create bad debt:

    if (maxFillPrice < currentPrice) revert FlatcoinErrors.MaxFillPriceTooLow(maxFillPrice, currentPrice);
    leverageModule.checkLeverageCriteria(margin, additionalSize);
    if (ILiquidationModule(vault.moduleAddress(FlatcoinModuleKeys._LIQUIDATION_MODULE_KEY)).getLiquidationMargin(additionalSize, maxFillPrice) >= margin)
        revert FlatcoinErrors.PositionCreatesBadDebt();

Impact

If any of these checks fail after the tokens have been transferred, the function will revert, and the tokens will remain in the contract. Since the function does not include a mechanism to return the tokens to the sender in case of a failed check, the tokens are effectively stuck in the contract.

Code Snippet

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

Tool used

Manual Review

Recommendation

The checks should be performed before any tokens are transferred. This ensures that if any condition is not met, the function will revert before the user's funds are moved. The code should be restructured as follows:

1. Perform all necessary checks to ensure that the order can be executed.
2. Only after all checks have passed, transfer the tokens from the caller to the contract.

[sherlock-admin]

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

takarez commented:

invalid

[nevillehuang]

Invalid, if checks revert, the transfer of tokens from caller to contract will not occur in the first place