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.

Sign up for GitHub

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

[gms-1627] erc20 improvements #204

Merged
merged 2 commits into from
Apr 16, 2024
Merged

[gms-1627] erc20 improvements #204

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
83d9185
[gms-1627] erc20 improvements
jasonzwli Apr 15, 2024
f768414
comments
jasonzwli Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Immutable Contracts is a library of smart contracts targeted at developers who w
- [ImmutableERC721](./contracts/token/erc721/preset/ImmutableERC721.sol)
- [ImmutableERC721MintByID](./contracts/token/erc721/preset/ImmutableERC721MintByID.sol)
- [ImmutableERC1155](./contracts/token/erc1155/preset/ImmutableERC1155.sol)
- [ImmutableERC20](./contracts/token/erc20/preset/ImmutableERC20.sol)
- [ImmutableERC20MinterBurnerPermit](./contracts/token/erc20/preset/ImmutableERC20MinterBurnerPermit.sol)
- [ImmutableERC20FixedSupplyNoBurn](./contracts/token/erc20/preset/ImmutableERC20FixedSupplyNoBurn.sol)

- Bridging contracts
Expand Down
13 changes: 6 additions & 7 deletions audits/token/202404-threat-model-preset-immutable-erc20.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
This document is a thread model for ImmutableERC20 token contracts built by Immutable.

Contracts covered under this model include:
[ImmutableERC20](../../contracts/token/erc20/preset/ImmutableERC20.sol)
[ImmutableERC20MinterBurnerPermit](../../contracts/token/erc20/preset/ImmutableERC20MinterBurnerPermit.sol)


## Context
Expand All @@ -21,13 +21,12 @@ The ERC20 token built by Immutable is meant to be used by game studios to releas

### ImmutableERC20

ImmutableERC20 is a simple wrapper around the [ERC20 implementation from openzeppelin](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol). It also extends [ERC20Permit](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC20Permit.sol) allowing token holders to offload the approval costs to a third party who wish to operate on behalf of the holder.
ImmutableERC20 is a simple wrapper around the [ERC20 implementation from openzeppelin](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol). It also extends [ERC20Permit](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC20Permit.sol), [ERC20Capped](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC20Capped.sol) and [ERC20Burnable](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC20Burnable.sol) allowing token holders to offload the approval costs to a third party who wish to operate on behalf of the holder.

The contract defines an immutable `_maxSupply` value to track the maximum number of tokens allowed to be in circulation. This helps the token to derive value using scarcity.

The `renounceOwnership()` method from the `Ownable` extension has been overridden to prevent the contract from being ownerless. This is to help Immutable Hub to link tokens based of their owners.
The `renonceRole()` method from the `AccessControl` extension has been overridden to prevent the contract from being ownerless. This is to help Immutable Hub to link tokens based of their owners.

`Owner` and `DEFAULT_ADMIN_ROLE` will share the same key at depolyment, but the `Ownership` can be transferred.


## Attack Surfaces
Expand All @@ -48,7 +47,6 @@ Functions that *change* state:
| ------------------------------------------------------------- | ----------------- | --------------------- |
| mint(address,uint256) | 0x40c10f19 | MINTER_ROLE |
| burn(address,uint256) | 0x9dc29fac | None - permissionless |
| renounceOwnership() | 0x715018a6 | OnlyOwner |
| transfer(address,uint256) | 0xa9059cbb | None - permissionless |
| approve(address,uint256) | 0x095ea7b3 | None - permissionless |
| increaseAllowance(address,uint256) | 0x39509351 | None - permissionless |
Expand All @@ -60,7 +58,7 @@ Functions that *change* state:
| renounceRole(bytes32,address) | 0x36568abe | None - permissionless |
| revokeMinterRole(address) | 0x69e2f0fb | DEFAULT_ADMIN_ROLE |
| revokeRole(bytes32,address) | 0xd547741f | DEFAULT_ADMIN_ROLE |
| transferOwnership(address) | 0xf2fde38b | OnlyOwner |
| burnFrom(address,uint256) | 0x79cc6790 | None - permissionless |

Functions that *do not change* state:
| Name | Function Selector | Access Control |
Expand All @@ -84,9 +82,10 @@ Functions that *do not change* state:
| supportsInterface(bytes4) | 0x01ffc9a7 | None - permissionless |
| symbol() | 0x95d89b41 | None - permissionless |
| totalSupply() | 0x18160ddd | None - permissionless |
| cap() | 0x355274ea | None - permissionless |

## Tests
`forge test` will run all the related tests for the above mentioned repos. The test plan and cases are written in the test files describing the scenario is it testing for. The test plan can be seen [here](../../test/token/erc20/preset/README.md)

## Diagram
![](./202404-threat-model-preset-immutable-erc20/ImmutableERC20.png)
![](./202404-threat-model-preset-immutable-erc20/ImmutableERC20MinterBurnerPermit.png)
Binary file removed audits/token/202404-threat-model-preset-immutable-erc20/ImmutableERC20.png
View file
Open in desktop
Binary file not shown.
Binary file added ...202404-threat-model-preset-immutable-erc20/ImmutableERC20MinterBurnerPermit.png
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ImmutableERC20 should be ImmutableERC20MinterBurnerPermit

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion contracts/access/MintingAccessControl.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Check warning on line 5 in contracts/access/MintingAccessControl.sol

View workflow job for this annotation

GitHub Actions / Run solhint 

imported name AccessControl is not used

Check warning on line 5 in contracts/access/MintingAccessControl.sol

View workflow job for this annotation

GitHub Actions / Run solhint 

imported name IAccessControl is not used
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If importing AccessControl and IAccessControl is important, please add a Solhint exclusion to remove the warning messages


abstract contract MintingAccessControl is AccessControlEnumerable {
/// @notice Role to mint tokens
Expand Down
9 changes: 6 additions & 3 deletions contracts/token/erc20/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,14 @@
This directory contains ERC 20 token contracts that game studios could choose to use
directly or extend.

| Contract | Description |
|---------------------------------|-----------------------------------------------|
| 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. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still correct, "Designed to be extended" ? Do we envisage game studios will deploy this as is?

| 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).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Status

Contract threat models and audits:
Expand Down
4 changes: 0 additions & 4 deletions contracts/errors/ERC20Errors.sol → contracts/token/erc20/preset/Errors.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,4 @@ pragma solidity 0.8.19;

interface IImmutableERC20Errors {
error RenounceOwnershipNotAllowed();

error MaxSupplyExceeded(uint256 maxSupply);

error InvalidMaxSupply();
}
73 changes: 0 additions & 73 deletions contracts/token/erc20/preset/ImmutableERC20.sol
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion contracts/token/erc20/preset/ImmutableERC20FixedSupplyNoBurn.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ pragma solidity 0.8.19;

import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import {IImmutableERC20Errors} from "../../../errors/ERC20Errors.sol";
import {IImmutableERC20Errors} from "./Errors.sol";


/**
Expand Down
70 changes: 70 additions & 0 deletions contracts/token/erc20/preset/ImmutableERC20MinterBurnerPermit.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Copyright (c) Immutable Pty Ltd 2018 - 2024
// SPDX-License-Identifier: MIT
pragma solidity 0.8.19;

import {ERC20Permit, ERC20} from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol";
import {ERC20Burnable} from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Burnable.sol";
import {ERC20Capped} from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Capped.sol";
import {MintingAccessControl, AccessControl, IAccessControl} from "../../../access/MintingAccessControl.sol";
import {IImmutableERC20Errors} from "./Errors.sol";

/**
* @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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be updated. The contract no longer has the concept of an "owner"

* This account has no rights to execute any administrative actions within the contract,
* with the exception of transferOwnership and role grants/revokes. This account is accessed via the owner()
* function. The Immutable Hub uses this function to help associate the ERC 20 contract
* with a specific Immutable Hub account.
*/
contract ImmutableERC20MinterBurnerPermit is ERC20Capped, ERC20Burnable, ERC20Permit, MintingAccessControl {
/// @notice Role to mint tokens
bytes32 public constant HUB_OWNER_ROLE = bytes32("HUB_OWNER_ROLE");

/**
* @dev Delegate to Open Zeppelin's contract.
* @param _name Name of the token.
* @param _symbol Token symbol.
* @param _hubOwner The account that owns the contract and is associated with Immutable Hub.
* @param minterRole The account that has the MINTER_ROLE.
* @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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

_grantRole(DEFAULT_ADMIN_ROLE, _admin);
_grantRole(HUB_OWNER_ROLE, _hubOwner);
_grantRole(MINTER_ROLE, minterRole);
}


/**
* @dev Mints `amount` number of token and transfers them to the `to` address.
* @param to the address to mint the tokens to.
* @param amount The amount of tokens to mint.
*/
function mint(address to, uint256 amount) external onlyRole(MINTER_ROLE) {
_mint(to, amount);
}


/**
* @dev Renounces the role `role` from the calling account. Prevents the last hub owner and admin from
* renouncing their role.
* @param role The role to renounce.
* @param account The account to renounce the role from.
*/
function renounceRole(bytes32 role, address account) public override(AccessControl, IAccessControl) {
if (getRoleMemberCount(role) == 1 && (role == HUB_OWNER_ROLE || role == DEFAULT_ADMIN_ROLE)) {
revert IImmutableERC20Errors.RenounceOwnershipNotAllowed();
}
super.renounceRole(role, account);
}


/**
* @dev Delegate to Open Zeppelin's ERC20Capped contract.
*/
function _mint(address account, uint256 amount) internal override(ERC20, ERC20Capped) {
ERC20Capped._mint(account, amount);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MinterBurnerPermit but it is also Capped.

}

}
2 changes: 1 addition & 1 deletion test/token/erc20/preset/ImmutableERC20FixedSupplyNoBurn.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ pragma solidity 0.8.19;
import "forge-std/Test.sol";

import {ImmutableERC20FixedSupplyNoBurn} from "contracts/token/erc20/preset/ImmutableERC20FixedSupplyNoBurn.sol";
import {IImmutableERC20Errors} from "contracts/errors/ERC20Errors.sol";
import {IImmutableERC20Errors} from "contracts/token/erc20/preset/Errors.sol";


contract ImmutableERC20FixedSupplyNoBurnTest is Test {
Expand Down
61 changes: 45 additions & 16 deletions test/token/erc20/preset/ImmutableERC20.t.sol → ...et/ImmutableERC20MinterBurnerPermit.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,18 @@ pragma solidity 0.8.19;

import "forge-std/Test.sol";

import {ImmutableERC20} from "contracts/token/erc20/preset/ImmutableERC20.sol";
import {IImmutableERC20Errors} from "contracts/errors/ERC20Errors.sol";
import {ImmutableERC20MinterBurnerPermit} from "contracts/token/erc20/preset/ImmutableERC20MinterBurnerPermit.sol";
import {IImmutableERC20Errors} from "contracts/token/erc20/preset/Errors.sol";


contract ImmutableERC20Test is Test {
contract ImmutableERC20MinterBurnerPermitTest is Test {

ImmutableERC20 public erc20;
ImmutableERC20MinterBurnerPermit public erc20;

address public minter;
address public hubOwner;
address public tokenReceiver;
address public admin;
string name;
string symbol;
uint256 maxSupply;
Expand All @@ -22,35 +23,63 @@ contract ImmutableERC20Test is Test {
hubOwner = makeAddr("hubOwner");
minter = makeAddr("minterRole");
tokenReceiver = makeAddr("tokenReceiver");
admin = makeAddr("admin");
name = "HappyToken";
symbol = "HPY";
maxSupply = 1000;

erc20 = new ImmutableERC20(name, symbol, hubOwner, minter, maxSupply);
erc20 = new ImmutableERC20MinterBurnerPermit(name, symbol, hubOwner, minter, maxSupply, admin);
}

function testInit() public {
assertEq(erc20.name(), name, "name");
assertEq(erc20.symbol(), symbol, "symbol");
assertEq(erc20.owner(), hubOwner, "Hub owner");
bytes32 minterRole = erc20.MINTER_ROLE();
assertTrue(erc20.hasRole(minterRole, minter));
bytes32 adminRole = erc20.DEFAULT_ADMIN_ROLE();
assertTrue(erc20.hasRole(adminRole, hubOwner));
assertEq(erc20.maxSupply(), maxSupply, "total supply");
assertTrue(erc20.hasRole(adminRole, admin));
assertEq(erc20.cap(), maxSupply, "total supply");
assertTrue(erc20.hasRole(erc20.HUB_OWNER_ROLE(), hubOwner), "hub owner");
}

function testChangeOwner() public {
address newOwner = makeAddr("newOwner");
function testRenounceLastHubOwnerBlocked() public {
vm.prank(hubOwner);
erc20.transferOwnership(newOwner);
assertEq(erc20.owner(), newOwner, "new owner");
bytes32 hubRole = erc20.HUB_OWNER_ROLE();
vm.expectRevert(abi.encodeWithSelector(IImmutableERC20Errors.RenounceOwnershipNotAllowed.selector));
erc20.renounceRole(hubRole, hubOwner);
}

function testRenounceOwnerBlocked() public {
vm.prank(hubOwner);
function testRenounceLastAdminBlocked() public {
vm.prank(admin);
bytes32 adminRole = erc20.DEFAULT_ADMIN_ROLE();
vm.expectRevert(abi.encodeWithSelector(IImmutableERC20Errors.RenounceOwnershipNotAllowed.selector));
erc20.renounceOwnership();
erc20.renounceRole(adminRole, admin);
}

function testRenounceAdmin() public {
address secondAdmin = makeAddr("secondAdmin");
vm.startPrank(admin);
erc20.grantRole(erc20.DEFAULT_ADMIN_ROLE(), secondAdmin);
assertTrue(erc20.hasRole(erc20.DEFAULT_ADMIN_ROLE(), secondAdmin));
vm.stopPrank();

vm.startPrank(admin);
erc20.renounceRole(erc20.DEFAULT_ADMIN_ROLE(), admin);
assertFalse(erc20.hasRole(erc20.DEFAULT_ADMIN_ROLE(), admin));
vm.stopPrank();
}

function testRenounceHubOwner() public {
address secondHubOwner = makeAddr("secondHubOwner");
vm.startPrank(admin);
erc20.grantRole(erc20.HUB_OWNER_ROLE(), secondHubOwner);
assertTrue(erc20.hasRole(erc20.HUB_OWNER_ROLE(), secondHubOwner));
vm.stopPrank();

vm.startPrank(hubOwner);
erc20.renounceRole(erc20.HUB_OWNER_ROLE(), hubOwner);
assertFalse(erc20.hasRole(erc20.HUB_OWNER_ROLE(), hubOwner));
vm.stopPrank();
}

function testOnlyMinterCanMint() public {
Expand Down Expand Up @@ -85,7 +114,7 @@ contract ImmutableERC20Test is Test {
vm.startPrank(minter);
erc20.mint(to, amount);
assertEq(erc20.balanceOf(to), amount);
vm.expectRevert(abi.encodeWithSelector(IImmutableERC20Errors.MaxSupplyExceeded.selector, maxSupply));
vm.expectRevert("ERC20Capped: cap exceeded");
erc20.mint(to, 1);
vm.stopPrank();
}
Expand Down
6 changes: 3 additions & 3 deletions test/token/erc20/preset/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@ All of the tests defined in the table below are in test/erc20/preset/ImmutableER
| testRenounceOwnershipBlocked | Ensure renounceOwnership reverts. | No | Yes |


## ImmutableERC20.sol
This section defines tests for contracts/erc20/preset/ImmutableERC20.sol. Note
## ImmutableERC20MinterBurnerPermit.sol
This section defines tests for contracts/erc20/preset/ImmutableERC20MinterBurnerPermit.sol. Note
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


| Test name |Description | Happy Case | Implemented |
|---------------------------------| --------------------------------------------------|------------|-------------|
Expand Down
Loading