Zaros Part 1
Zaros
DeFiFoundry
60,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

`TradingAccountBranch::depositMargin` doesn't account for fee on transfer tokens (USDT)

tedox

Summary

When a user deposits USDT as margin collateral into their account the contract doesn't account for the fact that USDT has a fee on transfer and the amount received will not be equal to the amount sent which will lead to the system suffering continuous losses.

Vulnerability Details

When calling TradingAccountBranch::depositMargin with USDT as collateral type the user specifies the amount of tokens they want to deposit and as a result that is the same amount with which their balance in the system is increased but in the case of USDT when safeTransferFrom is used the contract will receive less tokens than amount as the token takes a fee from every transfer it is used in meaning that the actual balance will be less than the recorded balance in a user's account.

/// @notice Deposits margin collateral into the given trading account.
/// @param tradingAccountId The trading account id.
/// @param collateralType The margin collateral address.
/// @param amount The amount of margin collateral to deposit.
function depositMargin(uint128 tradingAccountId, address collateralType, uint256 amount) public virtual {
// fetch storage slot for this collateral's config config
MarginCollateralConfiguration.Data storage marginCollateralConfiguration =
MarginCollateralConfiguration.load(collateralType);
​
// load existing trading account; reverts for non-existent account
// does not enforce msg.sender == account owner so anyone can deposit
// collateral for other traders if they wish
TradingAccount.Data storage tradingAccount = TradingAccount.loadExisting(tradingAccountId);
​
// convert uint256 -> UD60x18; scales input amount to 18 decimals
UD60x18 amountX18 = marginCollateralConfiguration.convertTokenAmountToUd60x18(amount);
​
// uint128 -> UD60x18
UD60x18 depositCapX18 = ud60x18(marginCollateralConfiguration.depositCap);
​
// uint256 -> UD60x18
UD60x18 totalCollateralDepositedX18 = ud60x18(marginCollateralConfiguration.totalDeposited);
​
// enforce converted amount > 0
_requireAmountNotZero(amountX18);
​
// enforce new deposit + already deposited <= deposit cap
_requireEnoughDepositCap(collateralType, amountX18, depositCapX18, totalCollateralDepositedX18);
​
// enforce collateral has configured liquidation priority
_requireCollateralLiquidationPriorityDefined(collateralType);
​
// get the tokens first
@> IERC20(collateralType).safeTransferFrom(msg.sender, address(this), amount); //@audit will receive less than amount
​
// then perform the actual deposit
tradingAccount.deposit(collateralType, amountX18); //@audit increases the user's balance with amount
​
emit LogDepositMargin(msg.sender, tradingAccountId, collateralType, amount);
}

Impact

The contract will be essentially paying for every fee of the USDT token on deposit and suffer continuous losses.

Tools Used

Manual review
VS Code

Recommendations

Handle the fee on transfer by checking the balanceOf(address(this)) before and after the transfer to determine the exact amount of tokens received.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.