Split vulnerable to preimage attack

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/main/src/Splits.sol#L143

Vulnerability details

Impact

A motivated attacker could invest the resources to craft a malicious SplitsReceiver to steal all of a users' pending funds.

Proof of Concept

This is a non-practical implementation of the attack, but shows by extending the SplitsReceiver array by any number of dummy receivers we have any number of bytes to find the desired hash.

    function testExploit() public {
      SplitsReceiver[] memory receivers = splitsReceivers(receiver1, 1, receiver2, 1); 
      setSplits(user, receivers);

      uint256 mallory = 42; 
      SplitsReceiver[] memory malicious = splitsReceivers(mallory, Splits._TOTAL_SPLITS_WEIGHT, 0, 0); 

      bytes32 target = Splits._splitsHash(user);
      bytes32 hash;
      for (uint i; i < type(uint).max; ++i) {
        malicious[1].userId = i;
        hash = Splits._hashSplits(malicious);
        if (target == hash) break;
      }   

      // extend receivers array until target hash has been found

      console.logBytes32(target);
      console.logBytes32(hash);

      // now steal a users funds
      Splits._split(user, asset, malicious);

    }

This is a similar issue with the current _splits implementation as attacking merkle trees as outlined here [0] and here [1]

Preimage attacks on keccak256 are practical as outlined in [2] and previous papers especially without many restrictions on message size.

[0] https://flawed.net.nz/2018/02/21/attacking-merkle-trees-with-a-second-preimage-attack/ [1] https://github.com/kmag/bad_examples/blob/master/bad_merkle_tree/bad_naive_merkle_tree.py [2] https://eprint.iacr.org/2023/101

Tools Used

Foundry

Recommended Mitigation Steps

In addition to verifying the hash, _assertSplitsValid[3] should also be called in _split making the attack impractical. Also storing the number of receivers in addition to the hash would render the attack impossible at the added cost of storing an additional uint8 (MAX_SPLITS_RECEIVERS supported).

[3] https://github.com/code-423n4/2023-01-drips/blob/main/src/Splits.sol#L226

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof

[GalloDaSballo]

Looks invalid, you are sending the split to the receivers