Skip to content
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

Convert SafeMath to use assert instead of require #312

Open
bingen opened this issue Feb 24, 2021 · 3 comments
Open

Convert SafeMath to use assert instead of require #312

bingen opened this issue Feb 24, 2021 · 3 comments
Assignees

Comments

@bingen
Copy link
Collaborator

bingen commented Feb 24, 2021

Generally speaking, at least for subtraction and division, we are not using SafeMath for input validation, so having underflows or zero divisions would likely mean a bug. Having assert instead of require would allow some of these bugs to be caught by tools like Echidna.

Some references:

OpenZeppelin/openzeppelin-contracts#1120
https://media.consensys.net/when-to-use-revert-assert-and-require-in-solidity-61fb2c0e5a57

@tabshaikh
Copy link

tabshaikh commented May 7, 2021

Can I help on this?

@bingen
Copy link
Collaborator Author

bingen commented May 10, 2021

Hi @tabshaikh , thanks a lot for the offering! This issue is not a high priority now, for 2 reasons:

  • It depends on being able to get this other one done: Add Echidna scripts to the CI #293
  • As we have already deployed to mainnet, we don’t plan to do much changens to contracts in the short term.

To change this, we should analyze first all the usages of Safe Math along our code base, and make sure that legit reverts like for instance due to user input are properly protected.

If you still feel like you want to give it a try, I would be happy to review it and help.

@tabshaikh
Copy link

tabshaikh commented May 10, 2021

Then @bingen could me point to anything that's high priority would like to help and learn :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants
@bingen @tabshaikh and others