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

Missing Ownership Verification in depositMargin Function

TZARred

Relevant GitHub Links

https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/TradingAccountBranch.sol#L317-L352

Summary

The depositMargin function allows deposits into any trading account without verifying the ownership of the account. This could lead to unintended deposits or abuse by malicious actors.

Vulnerability Details

Lack of ownership verification for the trading account.
The depositMargin function allows deposits into any trading account without verifying the ownership of the account. This could lead to unintended deposits or abuse by malicious actors.

Impact

Unauthorized users can deposit funds into accounts they do not own, leading to potential misuse or confusion.

Tools Used

Manual

Recommendations

Verify the ownership of the trading account before allowing deposits. Ensure that only the owner of the account can make deposits.

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
TradingAccount.Data storage tradingAccount = TradingAccount.loadExisting(tradingAccountId);
​
+ // Verify the ownership of the trading account
+ require(tradingAccount.owner == msg.sender, "Only the owner can deposit margin");
​
// 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);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge
10 months ago
inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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