Decentralise: remove owner, eip712 helper, latest address setter#209
Conversation
…setter In light of the external audit findings documented in raiden-network#135, we decided to remove the owner and functions that the owner used to change the contract state. - removed `owner_address` - removed `setLatestVersionAddress` - removed `setEip712HelperContract` - removed `MicroRaidenEIP712Helper` and included the getMessageHash logic into the main contract again (we can re-deploy after EIP712 changes)
|
My concern about any breaking change from EIP712 at this point is: it is not standardized, we cannot be sure that the current (experimental) implementation will be degraded gracefully. I implemented the upgradable message hash exactly due to this reasoning. |
|
@loredanacirstea one point about it is that we're not tied to the EIP712 implementation/revision. We could make the client/server identify when contract uses eip's rev.1 or 2 (maybe a small constant?), and use old hashing algorithm. Only drawback is that in these legacy cases, Metamask will present user with an opaque hash for signing (same as with Parity or any client which doesn't support either EIP712 proposals), and user will need to check it old style (maybe a UI link on how to generate that same hash externally and verify it?).
|
|
@andrevmatos , agreed. This approach is better than having an upgradable contract with possible trust and centralization issues. The idea with providing tools or explanation for how to verify old style, human-unreadable messages is indeed acceptable from my point of view, especially that this (human-unreadable messages) is the current way of doing things. |
The version of the contract should be sufficient for this, right?
@andrevmatos Can you elaborate a bit on what old-style is? How would that work? |
|
@LefterisJP old-style is user be oriented to externally check that the data it's supposed to agree really produces the hash he's about to be asked to sign. EIP712 is just a way for the wallet (Metamask) to offer itself (in a standardized way) as a trusted 3rd party to verify, show, hash and sign these data. In legacy code, as the wallet won't (possibly) be able to do it for the user (or hasn't support for the eip yet, being able only to sign), the user will be shown the data, and linked to simple instructions on how to verify it. Maybe an on-chain contract with only const functions for the known hashing formats, and etherscan forms to interact with it. Of course we don't expect the user to do this everytime, users may do that only if in doubt the data to be signed is correct in a trustless manner (or at least trusting things out of receiver's control, as blockchain). |
In light of the external audit findings documented in #135, issue 2:
we decided to remove the owner and functions that the owner used to change the contract state.
owner_addresssetLatestVersionAddresssetEip712HelperContractMicroRaidenEIP712Helperand included thegetMessageHashlogic into the main contract again (we can re-deploy after EIP712 changes)