-
Notifications
You must be signed in to change notification settings - Fork 343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs: add naught coin exercise #211
base: master
Are you sure you want to change the base?
Conversation
49 | | } | ||
``` | ||
|
||
You can see that the transfer reverted as expected. But... Our post-conditions weren't checked. If you have read the [ERC20 spec](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20.md), you will know that the `transfer` function returns a status `boolean`. In the case of success, we can emit a special `AssertionFailed` event that won't stop the execution of the function. This way, our post-conditions will be checked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is your opinion on using try
/catch
instead of relying on the return value? OZ tokens either revert or return true
:
if (currentTime < naughtCoin.timeLock()) {
// actions
try naughtCoin.transfer(bob,amount) {
// call succeeded, this is not ok
assert(false);
} catch {
// call reverted
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for your feedback, @glarregay-tob! As I described in my blog post, I initially went for the try
/catch
. They are much more intuitive and overall way better than return value checks. I switched to value checks for a couple of reasons:
- I couldn't get Echidna to work with
try
/catch
. The property was either always passing or always reverting - That's what Justin did in the Intro to AMM's invariants workshop, and I thought that it might be a better practice
- I must admit that in this particular case, the
try
/catch
with an empty catch block, as explained in the How to write good properties section is a little bit confusing to me.
I assumed that the OZ standard returns boolean values, but as you pointed out, this is wrong. The transfer
and transferFrom
functions return a boolean value, but the internal _transfer
function reverts on failure.
Thanks again for pointing that out; I will also fix this in my article.
I will re-write the properties with try
/catch
and update the exercise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be helpful to leave this section as it is and create a new follow-up section with Justin's setup using an external Setup
contract and re-write properties using try
/catch
to showcase that both options are valid. What do you think, @glarregay-tob?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is weird that it didn't work for you. Let's go with the option that works first, and then if you want you can add the new section.
As a side note, this can actually be generic for reverting and non reverting tokens: you can get the return value from a try
call by using try contract.function(param) returns (bool success) {
if that helps.
Maybe "How to write good properties" needs an update too :) thanks for pointing it out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a side note, this can actually be generic for reverting and non reverting tokens: you can get the return value from a try call by using try contract.function(param) returns (bool success) { if that helps.
This might actually be the case. The transfer
and transferFrom
functions return true if they succeed. The reverting mechanism is caused by the internal function _transfer
.
I will test it out in the follow-up section.
Thanks @glarregay-tob!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been experimenting with the try
/catch
, and this is where I got stuck the last time.
According to writing good properties step by step, if we expect a function to revert, we should place the assert(false)
in the first block. It means that the function call should not succeed, and when it does, we are bringing that up with the assert(false)
. The following code results in a property violation.
In this property, we expect that the transfer
will fail.
function token_transfer_should_fail_before_timelock_period(uint256 amount)
public
{
amount = _between(amount, 0, token.INITIAL_SUPPLY());
// pre-conditions
uint256 currentTime = block.timestamp;
uint256 playerBalanceBefore = token.balanceOf(address(player));
uint256 bobBalanceBefore = token.balanceOf(address(bob));
if (currentTime < token.timeLock()) {
// action
try
player.proxy(
address(token),
abi.encodeWithSignature(
"transfer(address,uint256)",
address(bob),
amount
)
)
{
assert(false);
} catch {
/* reverted */
}
// post-conditions
assert(token.balanceOf(address(player)) == playerBalanceBefore);
assert(token.balanceOf(address(bob)) == bobBalanceBefore);
}
}
And this Echidna's event sequence:
Call sequence:
1.token_transfer_should_fail_before_timelock_period(0)
Event sequence:
Panic(1)
merror Revert 0x
The assert(false)
gets reverted.
What does the merror Revert 0x
mean?
I got this property working by applying your suggestion about adding the returns(bool success)
:
try
player.proxy(
address(token),
abi.encodeWithSignature(
"transfer(address,uint256)",
address(bob),
amount
)
)
returns (bool success, bytes memory returnData) {
assert(!success);
} catch {
/* reverted */
}
This, however, leaves me with an unused second return parameter, returnData
. Should I ignore this warning, or is there a workaround?
The compiler won't let me ignore the second returned data type. The returns(bool success)
without the bytes memory returnData
results in: TypeError: Function returns 2 values, but returns clause has 1 variables.
Emitting this event also does not make sense as it won't be seen by Echidna in any case.
event Log(bytes data);
// ...
returns (bool success, bytes memory returnData) {
emit Log(returnData);
assert(!success);
} catch {
/* reverted */
}
I might be missing something obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the merror Revert 0x mean?
I believe this is caused by calling an external contract with the 0x0 address. Are you sure all the addresses are initialized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ggrieco-tob, Thanks for your response! I took a week off for a winter break. I will test this out as soon as I get back. Thanks 🎉
<details> | ||
<summary>4th property</summary> | ||
|
||
According to the ERC20 spec, the `approve` function should not fail if the caller has enough tokens to make the approval. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approve
won't fail even if you don't have enough tokens. It's like signing a blank check: "I allow this account to spend up to xxx of my funds in one or more transactions"
It might happen that I don't have the funds yet, but the approval goes through anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an oversight on my part. It's called an "allowance overrun", right?
Will fix that. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not aware of it having a special name, the ERC20 standard does not mention it. You just allow the account to spend your funds.
Reviewing OZ's implementation, the only way it can fail is if the address zero is involved, otherwise it just updates the allowance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Player should not `burn` tokens before the `timeLock` period. | ||
What is token burning? | ||
|
||
You can burn tokens by sending them to the `0` address or by sending them to a non existent address. Sending to the `0` address is not possible because of the `0` address checks in the `ERC20` OZ standard. Sending them to any address shouldn't be possible as well because of the `timeLock`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of a technicality, but transferring it to anywhere doesn't burn them, it just locks them.
A "proper" burning should decrease the total supply of the token, and not all tokens implement public functions to do so. Base ERC20 OZ token implements an internal _burn method that can be used by token implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a fair point. I think the word "effectively" is missing. Sending tokens to a non-existing address locks them up, which effectively burns them from further use.
The ERC20
used in this Ethernaut challenge did not implement a public burn functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
||
The token transfer via `transferFrom` should fail if the current `block.timestamp < timelock` and/or spender has enough allowance. | ||
|
||
This property in its base form is the same as the one with the `transfer` function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the previous one is changed to try
/catch
, this one should be changed too
50d31c0
to
43fdcff
Compare
|
||
// Note that the original symbol is changed from 0x0 to NTC | ||
// This is caused by https://github.com/crytic/echidna/issues/909 | ||
constructor(address _player) ERC20("NaughtCoin", "NTC") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ERC20
symbol had to be changed because of crytic/echidna#909
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this is fine, thanks for documenting this.
As a side note @glarregay-tob. Where should this exercise be placed? An idea that I had in my mind was to remove the numbers from the exercises altogether. This would prevent having broken links when adding new exercises. |
I think @ggrieco-tob will have a more accurate answer, but here's my take. Having exercise numbers can make it easier to organize them and give people a feeling of progression when someone is following the tutorials/examples. But since the increasing numbers aren't associated with an increased difficulty or anything anymore, we might need to review this approach. However, for this particular case, I'm a bit on the line if we should consider this a new exercise (because the explanation and the way it is presented is definitely a tutorial, it makes a lot of sense) or if we should open a new repo for Ethernaut solutions using Echidna. |
Yes, it definitely came out to be more of a tutorial rather than an exercise. I think that splitting the content into tutorials, how-to guides, exercises, explanations and references would be a good long-term approach. |
docs: add advanced setup instructions
@glarregay-tob, @ggrieco-tob, I've added the follow-up section with the external The only thing left is to change the name of this tutorial/exercise and place it in an appropriate place. I can reorder other exercises and place this one as |
@montyly what do you think about the name and reordering of these exercises? |
Can this one be converted to a PR to start the review? |
This is a draft PR for #207
tldr;
Where should this exercise be placed? Which number should I give to it?
setup
sectionIt came up to be a little bit wordy compared to other exercises. I wanted to narrow the gap between the first four exercises, which are relatively easy, to the last 4 harder exercises.
Instead of hints, I decided to go with hidden-by-default steps that the user can follow if he gets stuck. It does not spoil the solution.
I am publishing this as a draft so you can review it early and tell me if this is the right way to go about it.
Thanks!