Fix DoS vulnerability at the commit stage #60

polymorpher · 2021-07-27T11:25:51Z

…ts to provide verificationHash; verify commit’s verificationHash with eotp and restrict reveal to first valid commit entry in respective commit hash

…v7 and v3 compatibility; parseCommits util

…oning

ivan-homoliak-sutd

Security-wise, I do not see any critical issues, so I provide suggestions for optimizations and questions to possible minor issues.

ivan-homoliak-sutd · 2021-08-02T10:08:14Z

code/contracts/ONEWallet.sol

-        require(c.timestamp > 0, "Invalid commit hash");
+    function _completeReveal(bytes32 commitHash, uint32 commitIndex) internal {
+        Commit[] storage cc = commitLocker[commitHash];
+        require(cc.length > 0, "Invalid commit hash");


This and the following line will never evaluate to false since the function verifyReveal ensures that returned index is always within the allowed range. The same for timestamp check. Since require is related to the workflow of the code and asserts check internal errors, I'd recommend here to use asserts instead of requires. So, it will not confuse the auditors seeking for the code logic.

ivan-homoliak-sutd · 2021-08-02T10:32:07Z

code/contracts/ONEWallet.sol

+        require(cc.length > commitIndex, "Invalid commitIndex");
+        Commit storage c = cc[commitIndex];
+        require(c.timestamp > 0, "Invalid commit timestamp");
+        // should not happen
        uint32 index = uint32(c.timestamp) / interval - t0;
        _incrementNonce(index);
        _cleanupNonces();


In function _cleanupNonces (4th line), can it happen that (indexMinUnadjusted > t0) is not true?

If it is not true, indexMin would remain at 0. _cleanupNonces would do nothing, so there is no harm.

ivan-homoliak-sutd · 2021-08-02T10:39:47Z

code/contracts/ONEWallet.sol

+            uint32 counter = c.timestamp / interval - t0;
+            require(counter == index, "Index - timestamp mismatch");
+            uint8 expectedNonce = nonces[counter];
+            require(nonce >= expectedNonce, "Nonce too low");


Why do you enforce this condition? It might have negative consequences on the use case of the shared wallet that you mentioned before. I'd say that since each nonce corresponds to its own EOTP there is no need for such a constraint. EOTPs aggregated by the Merkle enable selective acknowledgment (see SmartOTPs paper.)

Yes, this is a remnant of an early version to prevent some types of attacks. It should be modified to require(nonce < maxOperationsPerInterval, "Nonce is invalid");

My previous comment in this thread is wrong. It would always be true that nonce < maxOperationsPerInterval since nonce is the remainder of modulus maxOperationsPerInterval. I think the only remaining purpose now for nonce is to track how many operations are already performed in the current time interval. The difference this line makes is whether we enforce nonce to be monotonically increasing. Suppose if we remove this line, then clients (possessing proofs) would be free to choose which nonce to use. It would be chaotic, since clients would have no idea what nonces other clients are using. If two clients choose the same nonce at the same time, one of them would fail. So in that situation, if two clients are trying nonces incrementally, it would not really be any better than the current situation enforced by this line: two clients would be forced to query the current nonce of the current block, and one of them would be forced to wait until the other finishes. On the other hand, if two clients coordinate with each other and try different nonces at the same time, then both transactions could go through. But then the client implementation would get complicated - WebSocket or WebRTC would have to be used for coordination, and we would inevitably need a server to broker the communication before the clients connect with each other. In view of that, I don't think we should remove the current constraint imposed by this line, until we do have proper WebSocket or WebRTC implementations to coordinate multiple clients. For now, the user(s) should coordinate manually and let one transaction finishes before the other in the multi-client use case. This approach is better for security reason as well - an attacker with the capability to intercept OTPs and read client side data would be forced to block the commit from the user's client, as otherwise the user's transaction would race with the attacker's. It makers this kind of attack, which is already very difficult to pull off, even easier to detect and harder to succeed.

ivan-homoliak-sutd · 2021-08-02T10:43:02Z

code/contracts/ONEWallet.sol

+            Commit[] storage cc = commitLocker[commits[i]];
+            for (uint32 j = 0; j < cc.length; j++) {
+                Commit storage c = cc[j];
+                hashes[index] = c.hash;


Hash can be removed from the Commit structure. It is never utilized for any verification, so storage can be saved. Moreover, the hash is a key to commitLocker map, so you already have that info.

Discussed over chat and I agree.

ivan-homoliak-sutd · 2021-08-02T10:53:22Z

code/contracts/ONEWallet.sol

+            for (uint32 j = 0; j < cc.length; j++) {
+                Commit storage c = cc[j];
+                hashes[index] = c.hash;
+                paramHashes[index] = c.paramsHash;


Commit structure contains paramHash, which should hide the details of the operation and thus ensure privacy. However, this is not met in all the cases, since paramHash might contain a repeated transfer to the same address with the same amount or the operation with a single parameter (recovery), which can be easily determined by the attacker. It can be resolved by adding some random 32B salt into paramHash, generated by the client.

Anyway, I still think that there is no reason for hiding the operation details in commit:

confirmation by OTPs is supposed to be used only for small amounts, so the attacker will not pay for overturning the blockchain to make some double-spending.

if you compare it with the native transfer of ONE, signed by ECDSA, the details of the transfer are leaked as well, so the MITM attacker can eclipse the victim in the same manner as in the case of OTPs.

I agree in parts. A salt seems reasonable. For the later part regarding reasons to hide operations at commit time, note a distinction between the situation here and that of ECDSA transfer in a single transaction:

There is one block confirmation difference (ECDSA transaction would be executed in the same block, here the reveal only happens in the next block).

Gas is already paid for the commit (which will be deducted from the wallet, as soon as Account Abstraction is implemented) prior to the occurrence of the fund-transfer transaction.

These two factors made it very easy for an opponent (in an application which the user is competing with others) to act in response to the user's intent (consider auctions, gamblings, games, etc.). Previously (or in the ECDSA case) the opponent has to operate a very active mining (validation) node, actively scan and parse all transactions in the current block, and pay a much higher gas if the attacker wants to utilize the revealed information to do anything (such as changing price for an auction, place a slightly higher bid, changing odds in a game the attacker hosts, or make advanced movements in a game, etc.). Now, with the parameters revealed in commit, the attacker can just lay back and examine a block that is already confirmed and index where the commit resides.

ivan-homoliak-sutd · 2021-08-02T11:09:37Z

code/contracts/ONEWallet.sol

        require(commits.length < MAX_COMMIT_SIZE, "Too many commits");
        commits.push(hash);
-        commitLocker[hash] = nc;


The hash under you store to commitLocker is sufficient when it contains only h(EOTP). The true commitment hash that commits to the parameters using EOTP is stored as the verification hash in commit structure. So, removing the neighbor and indexWithNonce from hash should be made. (Also, the hash field can be dropped from the commit structure - as I already mentioned in other review.)

Also discussed over chat, and I agree.

On a second thought - I think indexWithNonce should be kept. There may be repetitions in EOTP (especially if double OTP and controlled randomness are not used), though the probability that they occur less than two time intervals apart are very unlikely. The computation for 32 bytes more data in keccak256 is cheap anyway. And for backward compatibility reasons, it doesn't hurt to have 32 more bytes for neighbor as well. Let's keep this as is for now.

polymorpher · 2021-08-14T09:17:18Z

All comments are addressed in branch https://github.com/polymorpher/one-wallet/tree/v9.0 , except those commented above

polymorpher added 18 commits July 26, 2021 18:52

v6 contract

084af5b

Merge branch 'master' into better-commit

6a1e0e1

contract: allow multiple commits with same commit hash; require commi…

cee521b

…ts to provide verificationHash; verify commit’s verificationHash with eotp and restrict reveal to first valid commit entry in respective commit hash

core lib: verification hash

9bf9ef1

make tests work with new commit scheme

4165ee1

minor

e24f881

upgrade contract version

0669d58

relayer: work with v7 contract and backward compatibility with v6

b726825

core api: find commit v7 and v6 compatibility, new commit, getCommit …

0551027

…v7 and v3 compatibility; parseCommits util

core flow: v7 commit and SecureFlowsV7; minor bug fixes on v6

edd3175

contract recompile

4ac5fe4

add two deprecated methods to preserve selector

a942df3

update harmony network id

9214b61

fix V6 flow paramHash verification bug

977a379

use new lookupCommit function name

b740e5e

set default

54f2e57

fix issues with relayer version check and new contract creation versi…

22f4412

…oning

fix harmony-mainnet network id config

5d26c02

polymorpher merged commit 1ed4417 into master Jul 27, 2021

polymorpher changed the title ~~Resolve DoS vulnerability at the commit stage of the commit-reveal mechanism~~ Fix DoS vulnerability at the commit stage of the commit-reveal mechanism Jul 27, 2021

polymorpher changed the title ~~Fix DoS vulnerability at the commit stage of the commit-reveal mechanism~~ Fix DoS vulnerability at the commit stage Jul 27, 2021

ivan-homoliak-sutd reviewed Aug 2, 2021

View reviewed changes

polymorpher mentioned this pull request Aug 6, 2021

Contract cleanup #68

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DoS vulnerability at the commit stage #60

Fix DoS vulnerability at the commit stage #60

polymorpher commented Jul 27, 2021

ivan-homoliak-sutd left a comment

ivan-homoliak-sutd Aug 2, 2021

polymorpher Aug 3, 2021

ivan-homoliak-sutd Aug 2, 2021

polymorpher Aug 3, 2021

ivan-homoliak-sutd Aug 2, 2021

polymorpher Aug 3, 2021

polymorpher Aug 14, 2021

ivan-homoliak-sutd Aug 2, 2021

polymorpher Aug 3, 2021

ivan-homoliak-sutd Aug 2, 2021

polymorpher Aug 3, 2021

ivan-homoliak-sutd Aug 2, 2021

polymorpher Aug 3, 2021

polymorpher Aug 14, 2021

polymorpher commented Aug 14, 2021

Fix DoS vulnerability at the commit stage #60

Fix DoS vulnerability at the commit stage #60

Conversation

polymorpher commented Jul 27, 2021

ivan-homoliak-sutd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polymorpher commented Aug 14, 2021