Zaros Part 1

Summary

Use of delegatecall with arbitrary data can result in destruction of contract and loss of users' funds.

Vulnerability Detail

In the function createTradingAccountAndMulticall, the caller can pass in arbitrary bytes[] calldata data which is concatenated with tradingAccountId and then passed to a delegatecall

Since tradingAccountid is predictable, the danger lies with an attacker passing in data which makes up a malicious contract address that contains a selfdestructcall that would destroy the TradingAccountBranch contract.

Also, because other users grant TradingAccountBranch approval for token transfers (during deposits), some users may unwittingly grant unlimited approval limits, a practice which is quite common. An attacker could then abuse the delegatecall to steal tokens from these users.

Impact

Critical. TradingAccountBranch contract may be destroyed and other users' funds could be stolen.

Code Snippet

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

Tool used

Manual Review

Recommendation

Avoid the use of delegatecall if possible or whitelist the function. Otherwise, perform validation on the data param to ensure no malicious calls are being made.