Zaros Part 1

Summary

The createTradingAccountAndMulticall is a payable function that uses delegateCall in a loop.

Vulnerability Details

Using delegateCall in a loop in a payable function can be used to drain the whole protocol. This vulnerability comes from the fact that msg.value and msg.sender are persisted in delegatecall. The protocol is upgradable and new functionalities my be added. If any of them uses msg.value at any point this will be used to drain all the funds.
Consider the following scenario:
The protocol updates and it allows backing up positions with collateral in the form of ETH. If it has a deposit function that uses msg.value to calculate a user's deposit this may be used by an attacker to steal all funds by using createTradingAccountAndMulticall.

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/TradingAccountBranch.sol#L285

Impact

All ETH can be stolen from the contract in case of an update - High

Tools Used

Manual review

Recommendations

Remove the payable keyword from createTradingAccountAndMulticall.