Remove source credential type check in process_consolidation_request

[ensi321]

One of the conditions for a consolidation request to be processed is if source validator has credential type of 0x01 or 0x02.

However one can argue that source validator with 0x00 BLS credential will not pass the source address check ie. the last 20 bytes of the BLS credential cannot match the request's source address.

BLS credential is made up from a hash of validator's public key, whereas request's source address is populated from msg.sender ie. the execution address on the contract level. It is impossible for these two to be equal so it is unnecessary to check for credential type here.

[jtraglia]

Hey @ensi321, just curious what the rationale for this change is. Just simplification? I agree with BLS creds it's practically impossible to have a source address collision. But the withdrawal creds type check is very minimal and it doesn't hurt to be safe. I'm not sure how I feel about this change.

[mkalinin]

It is impossible for these two to be equal so it is unnecessary to check for credential type here.

In general, there can be a collision

[nflaig]

In general, there can be a collision

in theory yes, but is this possibility likely in practice? eg. I could generate a new seed right now and if I am lucky I get Vitalik's private key but that's not gonna happen ever in practice

[jtraglia]

So while very very unlikely, it's not impossible therefore we must keep this check. Closing this PR.

[mkalinin]

In general, there can be a collision

in theory yes, but is this possibility likely in practice? eg. I could generate a new seed right now and if I am lucky I get Vitalik's private key but that's not gonna happen ever in practice

I am not a cryptographer, but afaik, outputs of two different hash functions has a higher chance of collision than outputs of a single one

[nflaig]

I am not a cryptographer, but afaik, outputs of two different hash functions has a higher chance of collision than outputs of a single one

I think the issue is more severe and not just a random collision, it seems like it's possible to set arbitrary withdrawal credentials when submitting a deposit to the contract (it's user input in the deposit function) and there is no check it seems on the smart contract level and neither on the CL side to would prevent any 20 bytes to be set there to produce the collision with source address.

The changes as proposed in this PR could be merged as is and it would pass the CL spec tests as we rely on source_validator.withdrawal_credentials[12:] != consolidation_request.source_address to reject the consolidation request but this is not sufficient.... an attacker can manipulate the withdrawal_credentials in a way to pass this check with 0x00 credentials so the has_execution_withdrawal_credential is critical to have but the spec tests are lacking right now as they do not catch this when removing this check. However, this PR should address it #4162.