Secure commit-reveal and various contract, core library, and client upgrades for that #56
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Address issue #47 man-in-the-middle attack; Added a map for constant time commit lookup; Make some global variables immutable; Added appropriate comments for complex functions; Fixed a bug where token keys are computed using sha256 isntead of keccak256; Unified all reveal functions into one
| // constants | ||
| uint32 constant REVEAL_MAX_DELAY = 60; | ||
| uint32 constant SECONDS_PER_DAY = 86400; | ||
| uint256 constant AUTO_RECOVERY_TRIGGER_AMOUNT = 1 ether; |
There was a problem hiding this comment.
By doing so, the security of this wallet is 100% delegated to the security of the wallet that serves for last resort functionality. Therefore, I'd suggest to have at least some condition that would allow transfer to last resort only after some interval of inactivity, e.g., 1 month or something better.
There was a problem hiding this comment.
I agree. We should find a more flexible condition. One use case of recovery by auto trigger is loss-prevention upon theft. Requiring a long delay would render recovery in this use case ineffective. Ideally, the condition should also take this into consideration and balance on the trade-offs.
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Update 7/26/2021: This design has a mild DoS issue, identified by Andrianna Polydouri and her team. I will post another PR soon to fix it.
Fixes #47
Contract is upgraded to v6, which requires two arguments in commits: a commit hash, and a parameter hash. The commit hash is a hash of the credentials (neighbor, index, eotp). The parameter hash is a hash of the arguments for each specific operation type. For example, in a transfer operation, the parameter hash is a
keccak256(destination . amount)where both arguments are casted to 32 bytes and.is concatenation.All core library (
/code/lib) functions are updated to work with the new commit-reveal structure.The client and relayer are also upgraded. They are also made backward-compatible with wallets that are v5 or below.
All core library tests (
/code/test) are also updated and made to work again. The tests are very primitive right now. They should be expanded soon in next versions.