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

Payable `delegatecall` Loop Vulnerability in `createTradingAccountAndMulticall` Exposes Contract to `msg.value` Manipulation

0xabhayy

Summary

The createTradingAccountAndMulticall function in the TradingAccountBranch contract uses delegatecall within a loop in a payable function, potentially allowing for the reuse of msg.value and other security risks associated with unrestricted delegatecall.

Vulnerability Details

The vulnerable function is:

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;
}
}

The function is payable and uses delegatecall in a loop.

Each delegatecall has access to the full msg.value of the transaction.

Users can provide arbitrary calldata for the delegatecall.

There's no whitelist or restriction on which functions can be called via delegatecall.

Potential Issue

Alice wants to create a trading account and perform multiple operations.

Alice sends 1 ETH to the createTradingAccountAndMulticall function along with the data for multiple operations.

The function creates a trading account.
It then loops through the provided data and uses delegatecall to execute each operation.

Suppose a future version of the contract introduces a function that adjusts user balances based on msg.value.

Eve, an attacker, notices this and crafts a series of operations that exploit the retained msg.value.

Eve sends 1 ETH to the createTradingAccountAndMulticall function with crafted data.

Each delegatecall within the loop retains the original msg.value (1 ETH).

Suppose one of the called functions adjusts user balances based on msg.value. Eve's crafted data ensures this function is called multiple times within the loop.

Each call within the loop believes it is receiving 1 ETH due to the retained msg.value.

The function adjusting balances based on msg.value is executed multiple times, each time thinking it received 1 ETH.

Eve's balance is artificially inflated without her actually sending multiple ETH.

Impact

Potential Exploits: Attackers can use the batching functionality to manipulate balances or other state variables without sending actual funds.

Future Risk: If future versions of the contract or protocol use msg.value, this could introduce severe vulnerabilities.

Tools Used

Manual Review & Two Rights Might Make A Wrong

Recommendations

Short term, document the risks associated with the use of msg.value and ensure that all
developers are aware of this potential attack vector.

Long term, detail the security implications of all functions in both the documentation and
the code to ensure that potential attack vectors do not become exploitable when code is
refactored or added.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 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.