DSCEngine depositeCollateral may have some vulnerabilities.

[fraolb]

Looking at the depositeCollateral function, my question is: what if the sender calls this function without having the collateral token? It increases the s_collateralDeposited of the sender but does not transfer the amount from the sender's address.

Current code:

function depositeCollateral(address tokenCollateralAddress, uint256 amountCollateral)
    external
    moreThanZero(amountCollateral)
    isAllowedToken(tokenCollateralAddress)
    nonReentrant
{
    s_collateralDeposited[msg.sender][tokenCollateralAddress] += amountCollateral;
    emit CollateralDeposited(msg.sender, tokenCollateralAddress, amountCollateral);
    bool success = IERC20(tokenCollateralAddress).transferFrom(msg.sender, address(this), amountCollateral);
    if (!success) {
        revert DSCEngine__TransferFailed();
    }
}

Potential Issue

The sender may call this function without having the required collateralToken amount in their wallet. This leads to s_collateralDeposited being incremented even though the transfer fails.

Proposed Fix

The check for the successful transfer should occur before incrementing s_collateralDeposited to ensure that the state is only updated if the transfer is successful.

Revised code:

function depositeCollateral(address tokenCollateralAddress, uint256 amountCollateral)
    external
    moreThanZero(amountCollateral)
    isAllowedToken(tokenCollateralAddress)
    nonReentrant
{
    bool success = IERC20(tokenCollateralAddress).transferFrom(msg.sender, address(this), amountCollateral);
    if (!success) {
        revert DSCEngine__TransferFailed();
    } else {
        s_collateralDeposited[msg.sender][tokenCollateralAddress] += amountCollateral;
        emit CollateralDeposited(msg.sender, tokenCollateralAddress, amountCollateral);
    }
}

Is there something I am missing here?

[EngrPips]

Hello @fraolb, If the balance is increased and the code below does not return a success status, the whole transaction will revert as nothing happened in the first place.

bool success = IERC20(tokenCollateralAddress).transferFrom(msg.sender, address(this), amountCollateral);
    if (!success) {
        revert DSCEngine__TransferFailed();
    }

I understand from what perspective you are looking at it. You think we need to make the transfer first and ensure we get the money before increasing the state? Still, it can be done differently than that, as we prefer to update all of our states before interacting with anything outside our contract.

[fraolb]

ohh okay, thank you.

[EngrPips]

You are welcome, Friend.