Summary:
The function createTradingAccountAndMulticall in the TradingAccountBranch contract causes a variable collision when using the multicall feature. This issue arises when appending the tradingAccountId to the calldata, leading to incorrect parameter values during delegate calls. This can cause unintended behavior, such as parameter misinterpretation, which could be exploited by malicious actors.
Vulnerability Details:
The primary issue occurs when concatenating tradingAccountId with the calldata before making a delegate call. This method introduces a variable collision, where the parameters are misaligned, resulting in incorrect values being passed to the called functions.
For example, when calling the depositMargin function with the provided calldata, the intended parameters are:
abi.encodeWithSignature(
"depositMargin(uint128,address,uint256)",
uint128(1),
address(usdc),
uint256(500e6)
);
However, after concatenation, the amount parameter is overwritten with the usdc address converted to uint256, leading to unintended behavior and potential exploitation.
Code Snippet:
https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/TradingAccountBranch.sol#L285-L311
function createTradingAccountAndMulticall(
bytes[] calldata data,
bytes memory referralCode,
bool isCustomReferralCode
)
external
payable
virtual
returns (bytes[] memory results)
{
uint128 tradingAccountId = createTradingAccount(referralCode, isCustomReferralCode);
results = new bytes[](data.length);
for (uint256 i; i < data.length; i++) {
@> bytes memory dataWithAccountId = bytes.concat(data[i][0:4], abi.encode(tradingAccountId), data[i][4:]);
(bool success, bytes memory result) = address(this).delegatecall(dataWithAccountId);
if (!success) {
uint256 len = result.length;
assembly {
revert(add(result, 0x20), len)
}
}
results[i] = result;
}
}
Proof Of Concept:
Narouto calls the createTradingAccountAndMulticall function to create a new trading account and uses the multicall feature to deposit margin.
To the call the depositMargin function, the calldata is constructed as follows:
abi.encodeWithSignature(
"depositMargin(uint128,address,uint256)",
uint128(1),
address(usdc),
uint256(500e6)
);
The tradingAccountId is appended to the calldata before making the delegate call.
The depositMargin function is called with the modified calldata, causing the amount parameter to be overwritten with the usdc address converted to uint256.
The depositMargin function processes the transaction with the incorrect parameter values, leading to unintended behavior.
To test the POC, run the following Forge test:
forge test --mt test_FortisAudits_MulticallParameterMisalignment -vvvv
Proof Of Code:
pragma solidity ^0.8.25;
import { Base_Test } from "./Base.t.sol";
import { UD60x18, ud60x18 } from "@prb-math/UD60x18.sol";
import { SD59x18 } from "@prb-math/SD59x18.sol";
import { Errors } from "@zaros/utils/Errors.sol";
contract BugTest is Base_Test {
function setUp() public override {
Base_Test.setUp();
changePrank({ msgSender: users.owner.account });
configureSystemParameters();
createPerpMarketsFork();
}
function test_FortisAudits_MulticallParameterMisalignment() public {
bytes[] memory data = new bytes[](1);
data[0] = abi.encodeWithSignature(
"depositMargin(uint128,address,uint256)", uint128(1), address(usdc), uint256(500e6)
);
deal({ token: address(usdc), to: users.naruto.account, give: 1000e6 });
string memory customReferralCode = "customReferralCode";
changePrank({ msgSender: users.owner.account });
perpsEngine.createCustomReferralCode(users.owner.account, customReferralCode);
changePrank({ msgSender: users.naruto.account });
vm.expectRevert();
to the amount to deposit.
For this test the usdc address is = 0x059C5B023B577e82C346721653420711bFbdE3f9
when calling the deposit margin function the usdc address is being overwritten to the amount to deposit
address --> uint256
0x059C5B023B577e82C346721653420711bFbdE3f9 --> uint256 = 32031798082553246377275555095113161341086327801 this
in the amount param
*/
perpsEngine.createTradingAccountAndMulticall(data, bytes(customReferralCode), true);
}
}
Forge Test stack trace:
Perps Engine::createTradingAccountAndMulticall([0x49cdd2cf0000000000000000000000000000000000000000000000000000000000000001000000000000000000000000059c5b023b577e82c346721653420711bfbde3f9000000000000000000000000000000000000000000000000000000001dcd6500], 0x637573746f6d526566657272616c436f6465, true)
│ ├─ [264510] TradingAccountBranch::createTradingAccountAndMulticall([0x49cdd2cf0000000000000000000000000000000000000000000000000000000000000001000000000000000000000000059c5b023b577e82c346721653420711bfbde3f9000000000000000000000000000000000000000000000000000000001dcd6500], 0x637573746f6d526566657272616c436f6465, true) [delegatecall]
│ │ ├─ [126222] Trading Account NFT::mint(Naruto Uzumaki: [0xE9480E7b47FfAd4A512FabE731477A7565BC8229], 1)
│ │ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: Naruto Uzumaki: [0xE9480E7b47FfAd4A512FabE731477A7565BC8229], tokenId: 1)
│ │ │ ├─ [4585] Perps Engine::notifyAccountTransfer(Naruto Uzumaki: [0xE9480E7b47FfAd4A512FabE731477A7565BC8229], 1)
│ │ │ │ ├─ [1839] TradingAccountBranch::notifyAccountTransfer(Naruto Uzumaki: [0xE9480E7b47FfAd4A512FabE731477A7565BC8229], 1) [delegatecall]
│ │ │ │ │ └─ ← [Stop]
│ │ │ │ └─ ← [Return]
│ │ │ └─ ← [Stop]
│ │ ├─ emit LogCreateTradingAccount(tradingAccountId: 1, sender: Naruto Uzumaki: [0xE9480E7b47FfAd4A512FabE731477A7565BC8229])
│ │ ├─ emit LogReferralSet(user: Naruto Uzumaki: [0xE9480E7b47FfAd4A512FabE731477A7565BC8229], referrer: Owner: [0x93B07479cC807d037A98B6bB122CE8201958a595], referralCode: 0x637573746f6d526566657272616c436f6465, isCustomReferralCode: true)
│ │ ├─ [9782] Perps Engine::depositMargin(1, ECRecover: [0x0000000000000000000000000000000000000001], 32031798082553246377275555095113161341086327801 [3.203e46]) [delegatecall]
│ │ │ ├─ [7017] TradingAccountBranch::depositMargin(1, ECRecover: [0x0000000000000000000000000000000000000001], 32031798082553246377275555095113161341086327801 [3.203e46]) [delegatecall]
│ │ │ │ └─ ← [Revert] DepositCap(0x0000000000000000000000000000000000000001, 32031798082553246377275555095113161341086327801000000000000000000 [3.203e64], 0)
│ │ │ └─ ← [Revert] DepositCap(0x0000000000000000000000000000000000000001, 32031798082553246377275555095113161341086327801000000000000000000 [3.203e64], 0)
│ │ └─ ← [Revert] DepositCap(0x0000000000000000000000000000000000000001, 32031798082553246377275555095113161341086327801000000000000000000 [3.203e64], 0)
│ └─ ← [Revert] DepositCap(0x0000000000000000000000000000000000000001, 32031798082553246377275555095113161341086327801000000000000000000 [3.203e64], 0)
└─ ← [Stop]
Tools Used:
Manual Analysis
Recommendations:
To resolve this vulnerability, ensure the correct alignment of parameters when constructing the calldata. One possible solution is to define specific functions for handling these calls or use a different method to pass the tradingAccountId without causing collisions.
Here is the recommended mitigation:
function createTradingAccountAndMulticall(
bytes[] calldata data,
bytes memory referralCode,
bool isCustomReferralCode
)
external
payable
virtual
returns (bytes[] memory results)
{
uint128 tradingAccountId = createTradingAccount(referralCode, isCustomReferralCode);
results = new bytes[](data.length);
for (uint256 i; i < data.length; i++) {
- bytes memory dataWithAccountId = bytes.concat(data[i][0:4], abi.encode(tradingAccountId), data[i][4:]);
- (bool success, bytes memory result) = address(this).delegatecall(dataWithAccountId);
+ (bool success, bytes memory result) = address(this).delegatecall(data[i]);
if (!success) {
uint256 len = result.length;
assembly {
revert(add(result, 0x20), len)
}
}
results[i] = result;
}
}
