"rights" stored in memory is overwriting the memory block storing "from"

[c4-submissions]

Lines of code

https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/libraries/RegistryHashes.sol#L73-L83 https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/libraries/RegistryHashes.sol#L95-L107 https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/libraries/RegistryHashes.sol#L120-L130 https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/libraries/RegistryHashes.sol#L143-L156 https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/libraries/RegistryHashes.sol#L222-L232 https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/libraries/RegistryHashes.sol#L245-L258

Vulnerability details

Impact

Detailed description of the impact of this finding.

Expected code should keccak over packed encoding of (rights,from,to) but as 'rights' values are overwriting 'from' values. So values available for encoding is not as expected.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

mstore(add(ptr, 20), from) will store 'from' value from 20th position but 'rights' being 32 bytes long mstore(ptr, rights) will store 'rights' from 0th byte position till 31st byte position hence overwriting 'from' value in the memory.

https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/libraries/RegistryHashes.sol#L73-L83 https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/libraries/RegistryHashes.sol#L95-L107

Also in above 2 links as mstore(add(ptr, 40), to) stores 20 bytes 'to' in memory so the last 12 bytes given to keccak256(ptr, 72) will contain arbitrary values.

Similarly in https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/libraries/RegistryHashes.sol#L120-L130 https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/libraries/RegistryHashes.sol#L143-L156 https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/libraries/RegistryHashes.sol#L222-L232 https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/libraries/RegistryHashes.sol#L245-L258

mstore(add(ptr, 60), contract_) store 20 bytes 'contract_' in memory so the last 12 bytes given to keccak256(ptr, 92) will contain arbitrary values.

Tools Used

Manual review

Recommended Mitigation Steps

Carefully consider sizes of variables like in https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/libraries/RegistryHashes.sol#L73-L83 code can be modified as

mstore(add(ptr, 52), to) mstore(add(ptr, 32), from) mstore(ptr, rights)

Assessed type

en/de-code

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid

[GalloDaSballo]

Dup by same warden closing