[gms-1627] erc20 improvements #204
Conversation
jasonzwli
force-pushed
the
jason/auditfix
branch
from
1391691 to
48bed2c
Compare
jasonzwli
force-pushed
the
jason/auditfix
branch
from
48bed2c to
83d9185
Compare
| @@ -2,7 +2,7 @@ | |||
| // SPDX-License-Identifier: Apache 2.0 | |||
| pragma solidity 0.8.19; | |||
|
|
|||
| import {AccessControlEnumerable} from "@openzeppelin/contracts/access/AccessControlEnumerable.sol"; | |||
| import {AccessControlEnumerable, AccessControl, IAccessControl} from "@openzeppelin/contracts/access/AccessControlEnumerable.sol"; | |||
There was a problem hiding this comment.
If importing AccessControl and IAccessControl is important, please add a Solhint exclusion to remove the warning messages
There was a problem hiding this comment.
ImmutableERC20 should be ImmutableERC20MinterBurnerPermit
contracts/token/erc20/README.md
Outdated
| | ImmutableERC20 | Provides basic ERC 20 capability. Designed to be extended. | | ||
| | Contract | Description | | ||
| |--------------------------------------- |-----------------------------------------------| | ||
| | preset/ImmutableERC20MinterBurnerPermit| Provides basic ERC 20 Permit, Capped token supply and burn capability. Designed to be extended. | |
There was a problem hiding this comment.
Is this still correct, "Designed to be extended" ? Do we envisage game studios will deploy this as is?
contracts/token/erc20/README.md
Outdated
| | preset/ImmutableERC20FixedSupplyNoBurn | ERC 20 contract with a fixed supply defined at deployment. | | ||
|
|
||
| ## ImmutableERC20MinterBurnerPermit | ||
|
|
||
| This contract contains Permit methods, allowing the token owner to give a third party operator a `Permit` which is a signed message that can be used by the third party to give approval to themselves to operate on the tokens owned by the original owner. Users of permit should take care of the signed messages, as anyone who has access to this signed message can use it to gain access to the tokens. Read more on the EIP here: [EIP-2612](https://eips.ethereum.org/EIPS/eip-2612). |
There was a problem hiding this comment.
Permit is more complex (and safer) than what is described here.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.9/contracts/token/ERC20/extensions/ERC20Permit.sol#L49
There was a problem hiding this comment.
How about:
This contract contains Permit methods, allowing the token owner to give a third party operator a Permit which is a signed message that can be used by the third party to give approval to themselves to operate on the tokens owned by the original owner. Users take care when signing messages. If they inadvertantly sign a malicious permit, then the attacker could use use it to gain access to the user's tokens. Read more on the EIP here: EIP-2612.
|
|
||
| /** | ||
| * @notice ERC 20 contract that wraps Open Zeppelin's ERC 20 contract. | ||
| * This contract has the concept of an owner, called _owner in the constructor. |
There was a problem hiding this comment.
This needs to be updated. The contract no longer has the concept of an "owner"
| * @param _maxTokenSupply The maximum supply of the token. | ||
| * @param _admin The account that has the DEFAULT_ADMIN_ROLE. | ||
| */ | ||
| constructor(string memory _name, string memory _symbol, address _hubOwner, address minterRole, uint256 _maxTokenSupply, address _admin) ERC20(_name, _symbol) ERC20Permit(_name) ERC20Capped(_maxTokenSupply) { |
There was a problem hiding this comment.
I would order the parameters:
_roleAdmin
_minterAdmin
_hubOwner
_name
_symbol
_maxTokenSupply
That is, order all of the admin roles together and all of the token stuff together.
| that this contract extends Open Zeppelin's ERC 20 contract which is extensively tested here: | ||
| https://github.com/OpenZeppelin/openzeppelin-contracts/tree/release-v4.9/test/token/ERC20 . | ||
|
|
||
| All of the tests defined in the table below are in test/erc20/preset/ImmutableERC20.t.sol. | ||
| All of the tests defined in the table below are in test/erc20/preset/ImmutableERC20MinterBurnerPermit.t.sol. |
There was a problem hiding this comment.
also need tests for:
testRenounceLastHubOwnerBlocked
I would also have one test for each piece of OZ functionality you have brought in. This is just to make sure that the capability is there (the file was included correctly) and it at a high level works.
testBurn
testPermit
jasonzwli
force-pushed
the
jason/auditfix
branch
from
81e565b to
f768414
Compare
| * @param _maxTokenSupply The maximum supply of the token. | ||
|
|
||
| */ | ||
| constructor(address _roleAdmin, address _minterAdmin, address _hubOwner, string memory _name, string memory _symbol, uint256 _maxTokenSupply) ERC20(_name, _symbol) ERC20Permit(_name) ERC20Capped(_maxTokenSupply) { |
There was a problem hiding this comment.
It would be good to keep these constructor parameters similar across ERC20 contracts. Leading with common parameters string memory _name, string memory _symbol, address _hubOwner and then parameters that differ per ERC20.
| } | ||
|
|
||
| function testPermit() public { | ||
| uint256 ownerPrivateKey = 1; |
There was a problem hiding this comment.
makeAddrAndKey() is a nice way to get the address and privatekey in a single line.
(address alice, uint256 key) = makeAddrAndKey("alice");
| * @dev Delegate to Open Zeppelin's ERC20Capped contract. | ||
| */ | ||
| function _mint(address account, uint256 amount) internal override(ERC20, ERC20Capped) { | ||
| ERC20Capped._mint(account, amount); |
There was a problem hiding this comment.
MinterBurnerPermit but it is also Capped.
No description provided.