`Summa.sol` : Issue with `submitProofOfAddressOwnership()`

[zzzuhaibmohd]

Describe the bug

Issue#1: submitProofOfAddressOwnership() does not allow to resubmit AddressOwnershipProof twice

Currently the require check at Summa.sol#L120 allows the onlyOwner to add the AddressOwnershipProof associated with the cexAddress only once.

But imagine a scenario, wherein wrong signature or message was submitted during the first iteration, it that case, there is no way to update these values in the future.

---

Issue#2: proofIndex generation does no take into account the name of the chain

The root cause of the issue can be slightly linked to the first issue, The hash calculation at Summa.sol#L117 does not take into consideration the chain as the input during calculation. While this might not look an issue for EVM chains. But when considering a multi chain architecture, using at least two input for generating the hash is a good practice. Currently thinking of a practical impact due to this.

To Reproduce Yet to update PoC

Expected behavior Issue#1: submitProofOfAddressOwnership() does not allow to resubmit AddressOwnershipProof twice

I my opinion , since the submitProofOfAddressOwnership() is permissioned(can only be called by onlyOwner), updating the message and signature should be allowed.

---

Issue#2: proofIndex generation does no take into account the name of the chain

Make the following changes in the addressHash generation to generate a more unique hash

-- bytes32 addressHash = keccak256(
--     abi.encodePacked(_addressOwnershipProofs[i].cexAddress)
-- );
++
++ bytes32 addressHash = keccak256(
++     abi.encodePacked(_addressOwnershipProofs[i].cexAddress, _addressOwnershipProofs[i].chain)
++ );

Additional context Add any other context about the problem here.

[flyingnobita]

Issue#2: proofIndex generation does no take into account the name of the chain

To expand on the points given, it is not uncommon to have the same address on multiple chains (e.g. Ethereum Mainnet and a EVM derived change). Wouldn't using the address only for the hash will result in address of these chains to have the same hash, albeit they're separate wallets? Could this potentially leads to an address of 1 chain to be verified, while the same address on another chain to not needing verification?

[sebastiantf]

Agree that it is kinda confusing in the current implementation what is considered unique: (cexAddress) or (cexAddress, chain). Ideally it should be the latter to avoid confusion, be verbose and avoid the issues posted above

[0xalizk]

As a general note: I think Summa should incorporate domain separation

[alxkzmn]

This is a valid concern when dealing with different blockchain architectures, not chains. Imagine a situation when one exchange claims the ownership of hash("0xabcd...123", "137") on Polygon, and the other of the same address hash("0xabcd...123", "1") but on mainnet :) This can be addressed either by convention offchain (e.g., all chains with the same architecture must have the same "chain ID" in context of Summa, like "EVM"), or enforcing the chain list in the smart contract.