Merging Compound's Governance Token with OpenZeppelin ERC20 implementation

Support Contracts
,
#1

Hello, I just merged Compound’s Comp ERC-20 code with OpenZeppelin Token library in our token contract, as noted Compound’s Comp.sol used uint96 instead of uint256 in many places including balances mapping variable.

We have completed the merge and tested on remix, it seems to work fine and currently writing unit tests, but as the saying goes, two heads are better than one, I will be glad if the community can help glance through our implementations and point out what we might have done wrong.

We will go for auditing though, just that we want this to be our first approach. Thank you everyone

Code Repository: https://github.com/LibertyPie/Governance

1 Like
#2

Hi @zakpie,

:warning: I am a Community Manager not a Security Researcher. I have only had a very quick look at your token contract.

I suggest going through:

If you are planning an audit, it is a good idea to prepare early and start the process of organizing it. You can request an audit from OpenZeppelin: https://openzeppelin.com/request/

I would suggest targeting high code coverage with your unit tests.
Also review every line of code (even print it out) with everyone in the team. (even if the team is just you).

If you have commented out code, this should be addressed, either removed or be uncommented.

I am not familiar with Compound’s Comp ERC20 smart contract code. I would suggest reading through audits of this contract and checking for anything that you should address.
If other projects have forked this code, you may also want to look at the audits from those projects too.

You should have SPDX-License-Identifier on every contract with the appropriate license.
If you have merged smart contract code (under the appropriate license), you could add comments in the smart contract with the GitHub commit to the original and what license it is being used under, along with what modifications you have made. You want to make it easier for your community and any auditors to see what is going on.

When I first saw your mint function, I didn’t see any Access Control. You may want to consider the clarity of extending from the Preset ERC20 when you are adding to it, it may (or may not) be clearer to extend ERC20.

Finally, I would suggest looking at OpenZeppelin Defender as you have admin functionality: https://openzeppelin.com/defender/ and using a multi-sig to control your admin functions, see: https://docs.openzeppelin.com/learn/preparing-for-mainnet#admin-accounts

#3

Thank you so much for the expert review,

SPDX-License-Identifier will be added.

The main contract inherits ERC20PresetMinterPauser which has it own Access Control mechanism
with the Roles PauserRole & MinterRole, so our minter function calls super.mint which does the access control checks from the parent contract.

Finally we will look into OpenZeppelin Defender for extra sucurity and better admin functionalities.

Once again Thank You

1 Like
#4

Hi @zakpie,

I am neither an expert, nor did I review, just a quick look. :smile:

It took me a moment to pick that up that you were extending the ERC20 Preset.
So it might not be immediately clear to someone else reading it. You could always add a comment that you are using the AccessControl from there.