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

Multicall function can use msg.value with each function call

AuditTemple

Summary

createTradingAccountAndMulticall() does not implement any guards that prevent the caller from draining the contract's native funds.

Vulnerability Details

Currently, there isn’t any logic in any of the implementation contracts that use msg.value for any purpose. The only thing that needs native funds in the current state of the protocol is the Chainlink verification fee which seems unrelated to this function and a separate issue.

If the assumption that createTradingAccountAndMulticall() is payable for users to be able to make calls to functions that will use msg.value and that are not yet implemented (maybe added in Zaros Part 2) then there is an issue in the way createTradingAccountAndMulticall() handles msg.value.

Reference to code: link

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

When doing a multicall a user could call many function in a batch in one transaction. All these calls will have the same msg.value and if the functions use msg.value directly then the user will be able to drain the contracts funds.

This could happen when the user is sending 1 ETH with the multicall but each call from the batch spends 1 ETH from the contracts funds (lets say the user puts 10 such calls in that batch). At the end of the multicall the user have sent 1 ETH and spent 10 ETH.

Impact

User is able to drain the contracts native funds if there are any functions that work with msg.value.

Tools Used

Manual Review

Recommended Mitigation

In order to prevent this from happening createTradingAccountAndMulticall() should:

  1. record in storage the sent msg.value before looping through the calls

  2. the functions that work with msg.value should only trust this storage variable and reduce it with the amount they have used (depending on the logic)

  3. Before the multicall finishes execution reset the storage variable to 0.

+ uint256 public currentMsgValue;
function createTradingAccountAndMulticall(
bytes[] calldata data,
bytes memory referralCode,
bool isCustomReferralCode
)
external
payable
virtual
returns (bytes[] memory results)
{
+ currentMsgValue = msg.value;
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;
}
+ currentMsgValue = 0;
}
​
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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