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

Deposit Cap Can Be Breached by Positive PnL Credit Without Updating` totalCollateralDeposited` and lack of update on the totalCollateralDeposited

sancybars

Summary

When a trader's position has a positive PnL, the PnL amount is credited in USD and added to the trader's collateral margin. This process involves calling the deposit function, but the function does not update totalCollateralDeposited, which tracks the total deposited amount for each collateral type.

The _requireEnoughDepositCap that ensures new deposits do not exceed the deposit cap can be bypassed. Traders can then deposit additional amounts without triggering the deposit cap error, effectively breaching the cap.

//enforce new deposit + already deposited <= deposit cap
​
_requireEnoughDepositCap(collateralType, amountX18, depositCapX18, totalCollateralDepositedX18);
​

This deposit cap is set when creating the margin collateral type. `Usd` is also a collateral type so there's a cap

function configure(
​
address collateralType,
​
uint128 depositCap,
​
uint120 loanToValue,
​
uint8 decimals,
​
address priceFeed,
​
uint32 priceFeedHearbeatSeconds
​
)
​
internal
​
{
​
Data storage self = load(collateralType);
​
​
​
self.depositCap = depositCap;
​
self.loanToValue = loanToValue;
​
self.decimals = decimals;
​
self.priceFeed = priceFeed;
​
self.priceFeedHearbeatSeconds = priceFeedHearbeatSeconds;
​
}
​
}
​

If a traders position had positive Pnl they are credited in Usd , which gets added to the collateral margin of the trader by calling depositin the traders tradingAccount

// if trader's old position had positive pnl then credit that to the trader
​
if (ctx.pnlUsdX18.gt(SD59x18\_ZERO)) {
​
ctx.marginToAddX18 = ctx.pnlUsdX18.intoUD60x18();
​
tradingAccount.deposit(ctx.usdToken, ctx.marginToAddX18);

However when depositing into the traders trading account the totalCollateralDeposited isn't updated

function deposit(Data storage self, address collateralType, UD60x18 amountX18) internal {
​
// load address : collateral map for this account
​
EnumerableMap.AddressToUintMap storage marginCollateralBalanceX18 = self.marginCollateralBalanceX18;
​
​
​
// get currently deposited scaled-to-18-decimals this account has
​
// for this collateral type, then add newly deposited amount also scaled to 18 decimals
​
UD60x18 newMarginCollateralBalance = getMarginCollateralBalance(self, collateralType).add(amountX18);
​
​
​
// converts updated scaled-to-18-decimals UD60x18 to uint256 and stores
​
// it in address : collateral map for this account
​
marginCollateralBalanceX18.set(collateralType, newMarginCollateralBalance.intoUint256());
​
​
​
// load collateral configuration for this collateral type
​
MarginCollateralConfiguration.Data storage marginCollateralConfiguration =
​
MarginCollateralConfiguration.load(collateralType);
​
​
​
// update total deposited for this collateral type
​
marginCollateralConfiguration.totalDeposited =
​
ud60x18(marginCollateralConfiguration.totalDeposited).add(amountX18).intoUint256();
​
}
​

Vulneriability Details

When the trader attempts to deposit more usd as collateral _requireEnoughDepositCap will pass even if the accumulated amount is above the cap.

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);
​
​
​
// then perform the actual deposit
​
tradingAccount.deposit(collateralType, amountX18);
​
​
​
emit LogDepositMargin(msg.sender, tradingAccountId, collateralType, amount);
​
}
​

totalCollateralDeposited will be less than the deposit cap as it was not updated during settlement while actual traders collateralType deposit will be above the cap.

function _requireEnoughDepositCap(
​
address collateralType,
​
UD60x18 amount,
​
UD60x18 depositCap,
​
UD60x18 totalCollateralDeposited
​
)
​
internal
​
pure
​
{
​
if (amount.add(totalCollateralDeposited).gt(depositCap)) {
​
revert Errors.DepositCap(collateralType, amount.intoUint256(), depositCap.intoUint256());
​
}
​
}
​

For example; depositCap for USD is set to 1000

Trader initial deposit totalCollateralDeposited = 900 USD

The trader gets positive Pnl and gets credited 100 USD

The actual total is now = 1000( the deposit cap)

However the user can still deposit more margin because when the credit got added totalCollateralDepositedwasn't increased

The trader can deposit 1000 and since _requireEnoughDepositCap is still true the trader gets 2000usd as collateral.. bypassing the the 1000 cap

Impact

The deposit cap is meant to limit the total amount of collateral that can be deposited for a given collateral type. By not updating totalCollateralDeposited when crediting positive PnL, the contract allows traders to exceed this cap.

Tools Used

Manual Review

Recommendations

Increase the totalCollateralDeposited when adding credit to prevent the cap from being breached, this will force users to withdraw first if cap is reached

Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
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.