Predictable Registry Hashes May Lead To Storage Collisions and Data Corruption

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/libraries/DelegateTokenRegistryHelpers.sol#L16-L22

Vulnerability details

Summary

The DelegateTokenRegistryHelpers contract relies on the DelegateRegistry to store important access control data. However, the registry uses predictable hashes and storage locations that an attacker may be able to collide. This could lead to delegation data being overwritten and corrupted.

Vulnerability Details

The DelegateRegistry uses the keccak256 hash of predictable parameters like contract address and token ID to derive a storage location for each delegation. For example:

bytes32 registryHash = keccak256(abi.encodePacked(
   delegateFrom,
   delegateTo, 
   tokenContract,
   tokenId
));

registry.store(registryHash, data);

The delegateFrom, delegateTo, tokenContract, and tokenId values are all predictable. This allows an attacker to potentially provide colliding values that hash to the same storage location. If two delegations resolve to the same hash, the newer one will overwrite the older one.

For example, an attacker could create a fake delegation with:

delegateFrom: 0x1234...5678 delegateTo: 0x1234...5679 tokenContract: 0x0000000000000000000000000000000000000123 tokenId: 1

This could collide with and overwrite a target delegation with:

delegateFrom: 0x1234...5678 delegateTo: 0x1234...5678 tokenContract: 0x0000000000000000000000000000000000000001
tokenId: 2

By overwriting the target delegation, the attacker could gain access to assets.

Impact

This vulnerability could allow an attacker to overwrite arbitrary delegation storage slots if they can find colliding parameter values. They could then replace existing delegations with their own fake data and gain unintended access to assets.

Code Snippet

The loadTokenHolder function uses the predictable registryHash to load delegation data:

function loadTokenHolder(address delegateRegistry, bytes32 registryHash) internal view returns (address delegateTokenHolder) {
  return RegistryStorage.unpackAddress(
     IDelegateRegistry(delegateRegistry).readSlot(
       bytes32(uint256(RegistryHashes.location(registryHash)) + RegistryStorage.POSITIONS_SECOND_PACKED))
  );
}

Proof of Concept

1. Attacker finds collision between legitimate delegation A and fake delegation B

   // Legitimate delegation A
   bytes32 registryHashA = keccak256(abi.encodePacked(
     0x1234...5678, // delegateFrom
     0x1234...5678, // delegateTo
     0x0000000000000000000000000000000000000001, // tokenContract 
     2 // tokenId
   ));
   
   // Fake delegation B
   bytes32 registryHashB = keccak256(abi.encodePacked(  
     0x1234...5678, // delegateFrom
     0x1234...5679, // delegateTo
     0x0000000000000000000000000000000000000123, // tokenContract
     1 // tokenId
   ));
   
   // registryHashA collides with registryHashB
   require(registryHashA == registryHashB, "No collision found");

2. Attacker creates fake delegation B, overwriting A's slot

   // Attacker creates fake delegation
   IDelegateRegistry(delegateRegistry).delegateERC721(
     0x1234...5678, 
     0x1234...5679,
     0x0000000000000000000000000000000000000123,
     1,
     fakeData
   );

3. Contract calls loadTokenHolder() for delegation A, gets B's data

   // Contract tries to load original delegation
   address originalDelegate = DelegateTokenRegistryHelpers.loadTokenHolder(
     delegateRegistry,
     registryHashA // which now matches registryHashB!
   );
   
   // originalDelegate is now the attacker's fake delegate from B  

Tools Used

Manual code review

Recommended Mitigation Steps

1. Use unpredictable salts/nonces in hashes

   // Add salt param to prevent collisions
   bytes32 saltedHash = keccak256(abi.encodePacked(
     salt, 
     delegateFrom,
     delegateTo,
     ...
   ));

2. Validate registryHash integrity

   // Verify hash matches expected params
   function verifyHashMatch(
     bytes32 registryHash,
     address expectedFrom, 
     address expectedTo,
     ...) view returns (bool) {
   
      bytes32 computedHash = keccak256(abi.encodePacked(
          expectedFrom,
          expectedTo,
          ...
      ));
   
      return computedHash == registryHash;
   }

3. Handle collisions gracefully

   // Check for pre-existing slot data
   if (registry.getSlot(registryHash) != 0) {
     // Hash collision, do not overwrite 
     revert("Storage slot collision");
   }

4. Use enumeration instead of direct access

   // Safely iterate delegations instead of direct access
   address delegate = registry.getDelegateAddressForOwner(owner, token, id); 

Assessed type

Other

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid