Draft review: ERC20Snapshot

[elopio]

@frangio asked for more people to look at the new ERC20Snapshot contract, implemented as a contribution between mswezey23 (from Validity Labs), @nventuro and @frangio.

I want to do something different here, so please leave your thoughts about this review, and more meta, about making the review in this form. And even better, share your own reviews of this contract!

This can't be an audit like the ones we do at Zeppelin because I'm not an independent objective auditor. I'm part of the community, with super strong subjective opinions of what should be part of OpenZeppelin. So my idea is to make it more open, report the issues I find, but also critique the code and make open questions for others to join the discussion. The goal is to add this new feature to a [non-draft] stable OpenZeppelin release, in a way that is safe, transparent, and that satisfies users and contributors.

Back to the contract at hand, this is a nice and clever solution to the problem. It is an unintuitive solution, but the harder parts of the code are accompanied by good comments and it has a good test suite to cover all the corner cases.

Here are my findings (and PITA opinions), in order of appearance:

• There is no ERC for the API of ERC20 snapshots.
• I think the name ERC20Snapshot is not good. To be fully explicit, a better name would be ERC20WithOnDemandSnapshots. If we want a shorter name, then I'm not sure what's best. We can brainstorm it.
• There is no link to the MiniMe Token that inspired this contract. [PR#1637]
• The contract's comment is very confusing: "Snapshots store a value at the time a snapshot is taken (and a new snapshot id created), and the corresponding snapshot id." Maybe it could be something like: "When a snapshot is made, the balances and totalSupply at the time of the snapshot are recorded for later access." [PR#1638]
• There is no documentation of how to use this contract. [PR#1639]
• It would be nice to document the extra gas costs for using this instead of a normal ERC20.
• Comments are not following natspec.
• The Snapshot event should have a comment that mentions when it is emitted.
• The justification for the struct implementation is very poor: " These could be an array of a Snapshot struct, but that would impede usage of functions that work on an array." It could be a good decision, but this comment makes me doubt it. It seems that the design is made to fit the libraries, instead of implementing the right design and libraries for it. Being super strict (because I like you ), for things like this I would expect a link where the more-straightforward implementation is compared to the chosen one, and the winner has at least some merits.
• Typo: snapshoted -> snapshotted. [PR#1640]
• I think snapshot is not a nice verb. I suggest to rename the snapshot function to makeSnapshot, the Snapshot event to SnapshotMade, and never ever say snapshotted.
• Snapshot ids start at 1. This is weird, and we are more used to 0-indexing so it could lead to off-by-1 errors.
• Digging into the above, I see that findUpperBound returns 0 to signal an error. This is wrong, 0 is a valid element of an array. We need a better way to handle errors. Because solidity's exceptions are not very useful here, we can use the go style of returning a tuple with a boolean to indicate success, a style already used in this contract.
• I thought it would be nice to record the block of the snapshot somewhere, maybe even using it as the id. However, multiple transactions can be in the same block, so there could be multiple snapshots with the same block.
• Anybody can call snapshot. There should be a role for this.
• The snapshot implementation would be nicer if Counter.increment returned the new value. Any reason not to do this?
• balanceOfAt has no comment.
• I dislike balanceOf, but balanceOfAt is terrible. I suggest getBalanceFromSnapshot.
• totalSupplyAt has no comment.
• I suggest totalSupplyAt -> getTotalSupplyFromSnapshot.
• _valueAt -> _valueFromSnapshot
• The contract overrides _transfer, _mint and burn. It would be better to refactor ERC20 to have functions _updateBalance and _updateTotalSupply, and override those two.
• _updateSnapshot has an if without an else. This code path is taken when either no snapshots have been made and it is ok to do nothing; or when _lastSnapshotId(snapshots.ids) > currentId which is an impossible state with the current code, confirmed by the comprehensive test suite. There should be a comment explaining that there is no else by design, instead of an error condition that's not failing loudly. In particular, special care has to be taken to make sure that this function is the only place that modifies the arrays, to avoid breaking the strict relation of _lastSnapshotId(snapshots.ids) <= currentId.
• There is no test that checks the return value of snapshot.

[nventuro]

Thanks a lot for your thorough review @elopio! I find many of your comments super interesting.

@elopio

Digging into the above, I see that findUpperBound returns 0 to signal an error. This is wrong, 0 is a valid element of an array.

That 0 isn't signaling an error, it is covering this case (from its documentation):

If searched element is bigger than any array element function then returns first index after last element (i.e. all values inside the array are smaller than the target).

For an empty array, this would mean an index of 0 (i.e. the index matches the array length).

@elopio

Snapshot ids start at 1. This is weird, and we are more used to 0-indexing so it could lead to off-by-1 errors.

At first I also wasn't a fan of this, but ids are simply that, ids, and should not be e.g. iterated over. The fact that they are sequential is a happy coincidence, but they could very well be any arbitrary number.

@elopio

The justification for the struct implementation is very poor: " These could be an array of a Snapshot struct, but that would impede usage of functions that work on an array." It could be a good decision, but this comment makes me doubt it. It seems that the design is made to fit the libraries, instead of implementing the right design and libraries for it.

That'd require a new library to be created just for this contract though (copy-pasting the code from the Arrays library), which isn't a very good idea IMO. All that's lost is a little bit of encapsulation, but I'd argue the benefits far outweigh the loss of the slightly nicer syntax.

@elopio

Anybody can call snapshot. There should be a role for this.

It's very easy to override this method and add access control to it, if desired, whereas there'd be no way to get rid of all of the Roles machinery if such mechanism wasn't required. I think giving everyone the ability to issue snapshots by default is an interesting design decision, if controversial. We may want to add a comment here.

@elopio

The snapshot implementation would be nicer if Counter.increment returned the new value. Any reason not to do this?

It used to do that, but was removed (if I recall correctly) due to added gas costs, even when the returned value wasn't actually used. I'd be fine with adding the return value back.

@elopio

I think snapshot is not a nice verb. I suggest to rename the snapshot function to makeSnapshot , the Snapshot event to SnapshotMade , and never ever say snapshotted.

Hm, does this stem from us using the word 'snapshot' to refer to both the verb and the noun? You may have a point here. @frangio WDYT?

[elopio]

@nventuro

That 0 isn’t signaling an error, it is covering this case (from its documentation):

If searched element is bigger than any array element function then returns first index after last element (i.e. all values inside the array are smaller than the target).

You are right, my comment was wrong. But if I run findUpperBound([0], 0) it returns 0. findUpperBound([], 0) also returns 0. I don't like it, I think that last case should signal error in a more explicit way.

[nventuro]

@elopio

I don’t like it, I think that last case should signal error in a more explicit way.

Except it's not an error, an empty collection is a valid argument for a function that performs a search

[elopio]

Maybe it should be
Anyway, it seems we’ll just disagree on this one. One of the things that makes this review different is that I trust your decisions as maintainers, even if I don’t like them.

[frangio]

Thanks for the review @elopio!

@elopio

I would expect a link where the more-straightforward implementation is compared to the chosen one, and the winner has at least some merits.

I like this idea.

@elopio

getBalanceFromSnapshot, getTotalSupplyFromSnapshot

I think I like these. The current function names were taken from MiniMe, but to be honest the semantics are different because we use snapshot ids instead of block numbers. I'm also a bit reluctant to stray too far from ERC20 function names.

@elopio

Anybody can call snapshot. There should be a role for this.

It was a design decision to allow anyone to call snapshot. Have you identified this as a problem? Taking a snapshot increases the gas cost of the transfers that follow, but the effect is limited because this only affects the transfer that immediately follows a snapshot, and multiple snapshots do not increase the cost more than one.

As for the verb, what about takeSnapshot, SnapshotTaken, etc.?

[3sGgpQ8H]

@frangio An ability for anybody to create snapshots opens an attack vector. By spending fixed amount of gas an attacker may force potentially unlimited number of token holders to overpay for their next transfers. Thus attack cost is fixed and total damage is potentially unlimited.

BTW, long time ago I’ve implemented token smart contract with snapshot ability. The difference is that it also allows iterating through the list of token holders at particular snapshot ID.

[nventuro]

@3sGgpQ8H

@frangio An ability for anybody to create snapshots opens an attack vector. By spending fixed amount of gas an attacker may force potentially unlimited number of token holders to overpay for their next transfers. Thus attack cost is fixed and total damage is potentially unlimited.

I've also come to think that having it be public is too opinionated: I'd keep the current snapshot function, but make it internal, and allow users to implement it however they wish.