An example of a shortened ONE Wallet contract with equivalent functionalities #4

ivan-homoliak-sutd · 2021-06-23T10:00:00Z

No description provided.

code/contracts/ONEWallet2.sol

polymorpher · 2021-06-27T11:05:30Z

code/contracts/ONEWallet2.sol

+
+    // Management of daily spendings
+    uint256 dailyLimit; // uint128 is sufficient, but uint256 is more efficient since EVM works with 32-byte words.
+    mapping(uint => uint) dailySpendings; // day => amount ; can be cleaned up once in a while to save storage


We shouldn't need to track this by day since only the latest matter and the information can be reconstructed on-demand by querying past transactions of the contract address anyway.

I agree and the goal is not to track all days but the current one. Everything that is older than the current day is stale and thus can be cleaned up immediately, but I do it just once in a while. Using the mapping is just an implementation detail and has a big advantage over using two vars for tracking a current day spending + the timestamp of a day it corresponds to. The advantage is that mapping has data valid all the time, while two vars can get stale as soon as the day changes and nobody sends tx to contract, modifying these two vars, so getters for these vars would return an invalid value (which can be handle in the client of course, but it is not as intuitive as mapping).

That's true. I am avert to using mapping unless necessary because it stores data at pseudo-random location and requires random access, also costs more gas. But in practice it should not make much difference.

The gas cost of using maps is really negligible. That I'd not worry about. I slightly updated it in the recent version, though I still kept the map there. However, it can have only a single element at the time (except for the reveal execution which checks it for the current day and deletes the last day with the transfer). So far, it is still the clearest solution I see.

code/contracts/ONEWallet2.sol

polymorpher · 2021-06-27T11:14:41Z

code/contracts/ONEWallet2.sol

+        address payable addr; 
+        uint amount;        
+    }    
+    mapping(uint32 => Commit[]) public commits; // idx of OTP valid during a commitment => commitment data


I noticed only revealed commits are deleted (line 103). The commits would grow unbounded if an attacker spams it.

Yes, correct. However, it is on the attacker's expense of submitting commits. Anyway, clean-up method would definitely help. Thanks for pointing out it.

It clutters the blockchain. But another potential danger is if we are to use an upgrade pattern in this contract. The attacker who is allowed to make unlimited commits may find a way to override key storage locations through spamming commits, though the exact formulation of the attack is unknown at this time due to the assumed reliability of pseudo-randomness of SHA3.

I'd say we can safely assume that sha3 is collision-resistant and thus provides enough pseudo-randomness.

Proper clean-up method was already added.

polymorpher · 2021-06-27T11:17:24Z

code/contracts/ONEWallet2.sol

+    /**
+     * This is a general reveal for all types of operation.
+     */
+    function reveal(bytes32[] calldata neighbors, uint32 otpIdx, bytes32 eotp) external  returns (bool) {                        


I think this is a nice way of unifying the threes reveal functions into a single one. As mentioned above if we want to hide the nature of the commit, we would then need to add another parameter here (a struct that unifies parameters of all kinds of operations)

Actually, the unified parameters are already in the struct Commit, which is submitted during commit(). Therefore, here are no operation-specific parameters. Anyway, I will think about the use case that you mention if it is really something that we should be concerned about (I still do not see any serious example).

IMPORTANT: My conclusion is that we have to store parameters of the operation along with the hash of the commit because the smart contract has to verify in reveal() that h(submitted parameters || eotp) == submitted hash. This is how the first matching entry is found. Actually, this approach is the simplified 3-stage protocol for replacing the root hash in SmartOTPs. While I was designing our commit/reveal, I realized that 3-stages can be reduced to 2-stage, where in 1st stage, you submit hash of commit + params.

This has an implication of showing the parameters of the operation, as mentioned above (which should not be an issue).

code/contracts/ONEWallet2.sol

polymorpher · 2021-06-28T04:58:18Z

I will incorporate some good practices into the contract on master branch

polymorpher · 2021-07-06T08:12:12Z

Let's reopen this when Ivan is back from holiday

ivan-homoliak-sutd

Reactions of comments of the simple design. There are two issues to me: 1) reverting in execute transfer needs to be removed and added return statement instead. 2) if the commit really needs to hide the nature of the operation (type, value etc), the design needs to be modified. However, so far I do not see any use case that would require it.

ivan-homoliak-sutd · 2021-07-26T17:40:34Z

code/contracts/ONEWallet2.sol

+
+    // Management of daily spendings
+    uint256 dailyLimit; // uint128 is sufficient, but uint256 is more efficient since EVM works with 32-byte words.
+    mapping(uint => uint) dailySpendings; // day => amount ; can be cleaned up once in a while to save storage


I agree and the goal is not to track all days but the current one. Everything that is older than the current day is stale and thus can be cleaned up immediately, but I do it just once in a while. Using the mapping is just an implementation detail and has a big advantage over using two vars for tracking a current day spending + the timestamp of a day it corresponds to. The advantage is that mapping has data valid all the time, while two vars can get stale as soon as the day changes and nobody sends tx to contract, modifying these two vars, so getters for these vars would return an invalid value (which can be handle in the client of course, but it is not as intuitive as mapping).

code/contracts/ONEWallet2.sol

ivan-homoliak-sutd · 2021-07-26T17:53:21Z

code/contracts/ONEWallet2.sol

+        address payable addr; 
+        uint amount;        
+    }    
+    mapping(uint32 => Commit[]) public commits; // idx of OTP valid during a commitment => commitment data


Yes, correct. However, it is on the attacker's expense of submitting commits. Anyway, clean-up method would definitely help. Thanks for pointing out it.

ivan-homoliak-sutd · 2021-07-26T17:58:58Z

code/contracts/ONEWallet2.sol

+    /**
+     * This is a general reveal for all types of operation.
+     */
+    function reveal(bytes32[] calldata neighbors, uint32 otpIdx, bytes32 eotp) external  returns (bool) {                        


Actually, the unified parameters are already in the struct Commit, which is submitted during commit(). Therefore, here are no operation-specific parameters. Anyway, I will think about the use case that you mention if it is really something that we should be concerned about (I still do not see any serious example).

code/contracts/ONEWallet2.sol

polymorpher · 2021-07-27T22:06:53Z

code/contracts/ONEWallet2.sol

+        uint32 currentIdx = uint32(block.timestamp) / interval - t0; // check whether idx of OTP is timely
+        require(currentIdx - otpIdx <= MAX_REVEAL_DELAY, "Reveal is out of max allowed delay."); // here is OK to revert, coz if revert fails, submitted eotp + proof are invalidated already and cannot be misused anymore.
+
+        // 2) scan all the commits made within otpIdx interval and execute the first matching one        


Assume a user made a commit and revealed within the same otpIdx interval (30 seconds in general, which is necessary for good user experience). After observing the revealed EOTP but before it is confirmed and executed by the blockchain nodes, an attacker could make another commit with the correct EOTP using different parameters of operType, addr, and amount and produce a valid hash in that commit. The attacker can then reveal its own commit at a higher gas cost before the user's reveal is processed. The attacker's reveal will succeed because it meets the requirement here: under the attacker's parameters, it will be the first matching commit within the otpIdx interval.

That is true. It was very likely the reason why I put the parameters of an operation into the commit() in the original version instead of reveal(). The first matching one will be always the user's operation. Unfortunately, I did not make any comment to the code about it. So, with parameters in the commit(), it is secure but reveals user intention about the operation, which I still think does cause any problems in the scope of the standard wallet use cases.

Then the current implementation (#v0.4.1, also current master branch) would be similar, except keys of commits are encoded as commitHash to hide time information and to prevent attacker from committing at future slots, and parameters are also encoded as paramsHash to hide user's intent, and to make the structure work for all types of operations in addition to transfer. The hash here would be similar to verificationHash there

polymorpher · 2021-07-27T22:14:09Z

code/contracts/ONEWallet2.sol

+     */
+    function cleanUpCommits() external {                 
+        for (uint32 i = lastCleanUpOfCommitsIdx; i <= lastRevealedOTPIdx; i++){ // IH: in theory, this might lead to out-of-gas exception for settings of TOTP with very long validity but currently I'd not be affraid. Moreover, we are  getting the gas back for successfull deletions.
+            delete commits[i]; // NOTE: I'd say it makes no difference whether it was initialized or not, so we can delete any entry.        


For primitive arrays, a deletion on array itself is sufficient to delete everything. But I am uncertain whether it would be sufficient for struct arrays.

The compiler did not complain about it.

The spec reads:

delete a assigns the initial value for the type to a. I.e. for integers it is equivalent to a = 0, but it can also be used on arrays, where it assigns a dynamic array of length zero or a static array of the same length with all elements set to their initial value
(https://docs.soliditylang.org/en/v0.8.4/types.html#delete)

It is unclear from the language whether a dynamic array's elements would be set to their initial value

code/contracts/ONEWallet2.sol

polymorpher · 2021-07-27T22:18:13Z

code/contracts/ONEWallet2.sol

+    }  
+
+    function _drain() internal returns (bool) {
+        return lastResortAddress.send(address(this).balance);


similar issue with send as above

polymorpher · 2021-07-27T22:34:06Z

code/contracts/ONEWallet2.sol

+    //////////////////////// Internal Functions ////////////////////////// 
+
+    /**
+     * It cleans up the previous value of daily spending from the map (this ensures that map has always only a single value -- except the execution time of the reveal())


This procedure essentially reduces dailySpendings to a single value, so it needs not to be a mapping anymore

polymorpher · 2021-08-08T07:50:59Z

I am merging this into a separate folder - alternatives so we can track alternatives considered

Ivan Homoliak added 3 commits June 22, 2021 18:11

simplified TOTP contract

971b69f

another TOTP wallet proposal

ef6569a

recovery with a dedicated secret

1f816f2

ivan-homoliak-sutd requested a review from polymorpher June 23, 2021 10:00

polymorpher mentioned this pull request Jun 24, 2021

TODO before launching to a larger set of users (10k-100k people) #5

Open

74 tasks

polymorpher reviewed Jun 27, 2021

View reviewed changes

code/contracts/ONEWallet2.sol Outdated Show resolved Hide resolved

polymorpher reviewed Jun 27, 2021

View reviewed changes

code/contracts/ONEWallet2.sol Show resolved Hide resolved

polymorpher reviewed Jun 27, 2021

View reviewed changes

code/contracts/ONEWallet2.sol Outdated Show resolved Hide resolved

polymorpher reviewed Jun 27, 2021

View reviewed changes

code/contracts/ONEWallet2.sol Outdated Show resolved Hide resolved

polymorpher changed the title Ih An example of a shortened ONE Wallet contract with equivalent functionalities Jun 28, 2021

polymorpher closed this Jul 6, 2021

ivan-homoliak-sutd commented Jul 26, 2021

View reviewed changes

fixed issues raised by Aaron.

18472a7

polymorpher reopened this Jul 27, 2021

polymorpher reviewed Jul 27, 2021

View reviewed changes

code/contracts/ONEWallet2.sol Outdated Show resolved Hide resolved

polymorpher reviewed Jul 27, 2021

View reviewed changes

Ivan Homoliak added 2 commits July 31, 2021 17:42

reverted commit() from the original design, inspired by SmartOTPs

5e1c24c

call replaced send

29463b2

polymorpher merged commit 2dffb8a into simple-sc Aug 8, 2021

polymorpher mentioned this pull request Aug 8, 2021

Alternatives considered and reviewed #69

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

An example of a shortened ONE Wallet contract with equivalent functionalities #4

An example of a shortened ONE Wallet contract with equivalent functionalities #4

ivan-homoliak-sutd commented Jun 23, 2021

polymorpher Jun 27, 2021

ivan-homoliak-sutd Jul 26, 2021

polymorpher Jul 27, 2021

ivan-homoliak-sutd Jul 28, 2021

polymorpher Jun 27, 2021

ivan-homoliak-sutd Jul 26, 2021

polymorpher Jul 27, 2021

ivan-homoliak-sutd Jul 28, 2021

ivan-homoliak-sutd Jul 31, 2021

polymorpher Jun 27, 2021

ivan-homoliak-sutd Jul 26, 2021

ivan-homoliak-sutd Jul 31, 2021

polymorpher commented Jun 28, 2021

polymorpher commented Jul 6, 2021

ivan-homoliak-sutd left a comment

ivan-homoliak-sutd Jul 26, 2021

ivan-homoliak-sutd Jul 26, 2021

ivan-homoliak-sutd Jul 26, 2021

polymorpher Jul 27, 2021

ivan-homoliak-sutd Jul 28, 2021

polymorpher Jul 28, 2021 •

edited

Loading

polymorpher Jul 27, 2021

ivan-homoliak-sutd Jul 28, 2021

polymorpher Jul 28, 2021

polymorpher Jul 27, 2021

polymorpher Jul 27, 2021

polymorpher commented Aug 8, 2021

An example of a shortened ONE Wallet contract with equivalent functionalities #4

An example of a shortened ONE Wallet contract with equivalent functionalities #4

Conversation

ivan-homoliak-sutd commented Jun 23, 2021

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 Jun 28, 2021

polymorpher commented Jul 6, 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

polymorpher Jul 28, 2021 • edited Loading

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 8, 2021

polymorpher Jul 28, 2021 •

edited

Loading