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

Improper Handling of `msg.value` in `createTradingAccountAndMulticall`

pelz

Summary

The createTradingAccountAndMulticall function in TradingAccountBranch.sol is made payable but does not handle the msg.value that can be sent along with the call. This can result in funds being stuck in the contract with no way to withdraw them, posing a significant risk of loss of funds.

Vulnerability Details

The createTradingAccountAndMulticall function allows for the creation of a trading account and multiple calls to be executed in a single transaction. Although the function is marked as payable, it does not process or forward the msg.value received, leading to the potential for funds to be trapped within the contract.

Since none of the functions within the contract require ETH to be sent, and all delegate calls will still be made back to the contract itself, this risk of funds getting stuck is significant. The contract does not have a mechanism to handle the received ETH, which increases the likelihood of funds becoming irretrievable.

Proof of Concept

function createTradingAccountAndMulticall(
bytes[] calldata data,
bytes memory referralCode,
bool isCustomReferralCode
)
external
payable // @audit ignored msg.value
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 Test

function test_WhenETHIsSentAlong()
external
whenTheDataArrayDoesNotProvideARevertingCall
whenTheUserHasAReferralCode
whenTheReferralCodeIsCustom
{
bytes[] memory data = new bytes[](0);
​
string memory customReferralCode = "customReferralCode";
changePrank({ msgSender: users.owner.account });
perpsEngine.createCustomReferralCode(users.owner.account, customReferralCode);
​
changePrank({ msgSender: users.naruto.account });
​
// it should emit {LogReferralSet} event
vm.expectEmit({ emitter: address(perpsEngine) });
emit TradingAccountBranch.LogReferralSet(
users.naruto.account, users.owner.account, bytes(customReferralCode), true
);
vm.deal(users.naruto.account, 20 ether);
​
perpsEngine.createTradingAccountAndMulticall{value: 20 ether}(data, bytes(customReferralCode), true);
assertEq(address(perpsEngine).balance, 20 ether, "createTradingAccountAndMulticall: balance");
}

Impact

The vulnerability allows funds sent to the createTradingAccountAndMulticall function to become stuck in the contract. Since there is no mechanism to withdraw these funds, users could lose their ETH without any means of recovery. This can affect user trust and the financial stability of the platform.

Tools Used

Manual review

Recommendations

  1. Handle msg.value Appropriately: Ensure the function either processes the received ETH or forwards it to a designated address to avoid trapping funds.

  2. Add Withdrawal Mechanism: Implement a function to allow the withdrawal of any ETH mistakenly sent to the contract.

Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

`createTradingAccountAndMulticall` shouldn't be payable

Support

FAQs

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

Give us feedback!