DittoETH

Summary

The liquidateSecondary function in the contract is vulnerable to gas exhaustion attacks due to lack of input validation. It takes a "batches" array as a parameter, containing records of shorts to liquidate. However, it does not check the length of this array before entering a for loop that iterates from 0 to batches.length.

Inside the for loop, the _setMarginCallStruct function is called, which reads data from storage based on the current batches array element:

 function liquidateSecondary(batches MTypes.BatchMC[] memory batches) {

   for (uint256 i; i < batches.length; i++) {

  MTypes.MarginCallSecondary memory m;

  m = _setMarginCallStruct(
  asset, batches[i].shorter, batches[i].shortId // reads storage
      );
    }
  }

By not validating the length of the input array, a malicious actor can pass a very large batches array, forcing excessive storage reads via _setMarginCallStruct inside the loop. Since each storage operation costs gas, a long-enough array could potentially cause gas exhaustion, disrupting transactions.

In summary, the lack of input validation on the batches array makes the contract vulnerable to disruption through a gas exhaustion attack. Proper length checking is required to fix this high severity issue.

Vulnerability Details

The liquidateSecondary function is used to process multiple batches of short records for liquidation. However, it does not validate the length of the batches array passed in before iterating over it.

This lack of input validation on a loop boundary makes the function vulnerable to gas exhaustion attacks.

Exploitation:
A malicious actor could craft a transaction to call liquidateSecondary with a very large batches array:

 batches = new MTypes.BatchMC[](1000000); // array of 1 million elements

This would force unnecessary operations during the processing loop:

for (uint256 i; i < batches.length; i++) {

 // Calls storage-reading function batches.length times  
   _setMarginCallStruct(batches[i]);  

     }

Each call to the storage-reading _setMarginCallStruct costs gas. With a large enough input, the cumulative gas cost of these storage operations could exceed the block gas limit.

This would cause the transaction to fail due to out of gas, disrupting the liquidation process.

The attacker would not need any special access - any user could trigger it by constructing a long batches array.

Proper input validation is critical when user-provided data is used as a loop boundary or to read storage, to avoid vulnerabilities like this.

Impact

Disrupted Liquidations:

By causing liquidateSecondary to fail, an attacker could prevent short positions from being closed out as intended through the liquidation process. This undermines the core risk management mechanism.

Manipulated Market Prices:

If an attacker was able to selectively disrupt liquidations, they may be able to manipulate market prices in their favor by selectively saving positions that should be liquidated.

Wasted Gas:

Each failed transaction due to running out of gas wastes the gas spent on those storage operations. This reduces the number of legitimate transactions that can be processed per block.

In summary, the impacts include undermining core risk functions, potential for market manipulation, reduced network performance, and damage to the protocol's reputation. Properly validating inputs is important to prevent exploits with serious consequences.

Tools Used

Solidity Scan

Manual Review

Recommendations

Fix: Add Input Validation

The liquidateSecondary function should validate the length of the batches array before using it in the for loop:

 function liquidateSecondary(MTypes.BatchMC[] memory batches) {

 // Add input validation
   require(batches.length > 0, "Batches must not be empty");

    for (uint256 i; i < batches.length; i++) {

        //...
      }
    }

Explanation:
Add a require statement to check batches.length is greater than 0
This validates there is at least one element in the array
It prevents an empty or excessively large array from being passed
The error message "Batches must not be empty" clearly indicates expected format
Requiring a non-zero length protects the loop index from going out of bounds
It effectively puts an upper limit on the number of iterations
This prevents unintended long-running loops due to malicious inputs
Input validation like this is considered a best practice for functions with loops

By adding this upfront check, it ensures the for loop iteration count is within expected parameters. This fixes the vulnerability by disallowing any array size that could potentially cause gas exhaustion through excessive storage operations.

Proper input validation is necessary when external data factors into control flow like loops, to avoid vulnerabilities. This check securely bounds the problem space.