Skip to content

Commit 012d2ca

Browse files
zigturzigtur
committed
HeyMint - second audit - ERC1155 beginning
1 parent 7dcce04 commit 012d2ca

10 files changed

+1408
-46
lines changed

‎ERC1155/HeyMintERC1155-ZigturAudit.md

Lines changed: 254 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,254 @@
1+
# Introduction
2+
3+
4+
# Disclaimer
5+
6+
7+
# About Zigtur
8+
9+
Zigtur is a cybersecurity professional. Graduated from a cyberdefense engineering school, he now focuses its efforts on smart contract auditing.
10+
11+
Twitter: [@zigtur](https://twitter.com/zigtur)
12+
Discord: [@zigtur](https://discordapp.com/users/909209529558921276)
13+
14+
15+
# About HeyMint
16+
**HeyMint project:** https://heymint.xyz/
17+
18+
"HeyMint is thrilled to create amazing no-code tools that help NFT creators & collectors do what they love. 100% free because we ♡ you."
19+
20+
HeyMint is a project that aims to allow every creators to easily deploy their own tokens collection (NFT and normal tokens). Some advanced features like presale, royalties, loans will be included. A user will just have to set the config, by using a web interface.
21+
22+
HeyMint will soon launch an ERC1155 platform to easily deploy ERC1155 contracts.
23+
24+
25+
# Threat Model & Methodology
26+
## Actors
27+
There are 4 types of actors:
28+
- HeyMint: Deployer of all the extensions (A to G), the Reference contract and the AddressRelay contract. This actor also has control on some settings on all the `HeyMintERC1155Child`.
29+
- Creator: Deployer of an `HeyMintERC1155Child`, this actor can control almost all the settings of this child. It can also be named "Owner".
30+
- Presale user: User of a child, he has a presale ticket.
31+
- Normal user: User of a child.
32+
33+
## Vulnerability classification
34+
Only the impact will be studied in this audit.
35+
36+
| Impact level | Explanation | Example |
37+
|-------------------------|------------------------------------------------------------------------------------|-------------------------|
38+
| Critical | This vulnerability would have a tremendous impact on HeyMint and all the creators. | All funds are getting stolen from all childs |
39+
| High | A lot of end-users are impacted by the vulnerability or multiple child contracts | Malicious creator steals all funds by malicious behavior |
40+
| Medium | Only a few end-users are impacted by the vulnerability. | A user can't mint its presale tokens |
41+
| Low | This has no impact on project, but would better be fixed before deployment | Gas optimizations, not mandatory checks, ... |
42+
43+
## Solutions
44+
Solutions can be given in the following audit. Some of them can be done easily, and so a fixed code will be given.
45+
46+
When the fix is too heavy, no code proposal will be given. However, ways to solve the issue will be detailed to help HeyMint team fixing the issue.
47+
48+
# Findings
49+
50+
## Presale tickets can be reused on other childs, if owner uses same signer address
51+
52+
**Vulnerability classification: High**
53+
54+
A user with one presale on a creator contract could be able to mint on every other creator contracts.
55+
56+
### Explanations
57+
58+
*MISSING CONTENT*
59+
60+
### Vulnerable code
61+
62+
*MISSING CONTENT*
63+
64+
### Vulnerability test
65+
66+
*MISSING CONTENT*
67+
68+
### Solution
69+
70+
*MISSING CONTENT*
71+
72+
#### First solution
73+
74+
*MISSING CONTENT*
75+
76+
#### Second solution
77+
78+
*MISSING CONTENT*
79+
80+
## Presale users can lose presale mints because of other users
81+
*Note: This vulnerability holds if presale minting period overlaps with normal minting period. As no checks are done in public mint functions to verify overlapping, this is considered as valid.*
82+
83+
**Vulnerability classification: High**
84+
85+
All the presale users could be impacted by this vulnerability.
86+
87+
### Explanations
88+
89+
During a presale mint, a require statement is used to check that, once tokens will be minted, the total supply will not be greater than the presale max supply.
90+
91+
A public mint will increase the total supply. As public mint and presale mint periods can overlap each other, the total supply can reach the presale max supply by using only public mints. So, presale max supply can be reached without a single presale mint.
92+
93+
Then, presale users will not be able to use their presale mints because of the require statement.
94+
95+
### Vulnerable code
96+
- File: `HeyMintERC1155ExtensionC.sol`
97+
- Function: `presaleMint`
98+
- Lines: `187-192`
99+
- Explanations: `state.data.totalSupply[_tokenId] + _numTokens` is checked against `tokenConfig.presaleMaxSupply`. But not all mints in `totalSupply` were presale mints, there could be public mints in it.
100+
- Vulnerable code:
101+
```solidity
102+
require(
103+
tokenConfig.presaleMaxSupply == 0 ||
104+
state.data.totalSupply[_tokenId] + _numTokens <=
105+
tokenConfig.presaleMaxSupply,
106+
"MAX_SUPPLY_EXCEEDED"
107+
);
108+
```
109+
110+
### Vulnerability test
111+
Multiple unit tests for this vulnerability can be found in `ZigturAudit.js`, under the names:
112+
- `Presale users can lose presale mints because of other users - mintToken case`
113+
- `Presale users can lose presale mints because of other users - giftTokens case`
114+
115+
### Solution
116+
#### First solution
117+
One way to solve the problem is to:
118+
- Add a `presaleTotalSupply` field, to keep track of how many tokens were minted using presales
119+
- Add a `presaleTokensMintedByAddress` field, to keep track of how many presale tokens were minted by user
120+
- Modify checks in `presaleMint()` to verify that the new `presaleTotalSupply` is not greater than `presaleMaxSupply`
121+
- Increment `tokensMintedByAddress` AND `presaleTokensMintedByAddress` when presale minting
122+
123+
#### Second solution
124+
Another way to fix the issue is by not overlapping presale mint and public mint periods.
125+
126+
It is worth saying that this solution **is not** the most user-friendly one.
127+
128+
129+
130+
131+
## Presale users can lose presale mints, if they use normal mint
132+
*Note: This vulnerability holds if presale minting period overlaps with normal minting period. As no checks are done in mintToken() to verify overlapping, this is considered as valid.*
133+
134+
**Vulnerability classification: Medium**
135+
136+
All the presale users could be impacted by this vulnerability. But, it is a "self attack".
137+
138+
### Explanations
139+
During a presale mint, two require statements are used. The first one checks that, once tokens will be minted, the number of tokens minted by address will not be greater than the number of presale mints allowed per address. The second one checks that, once tokens will be minted, the number of tokens minted by address will not be greater than the number of presale mints allowed to the sender user.
140+
141+
A public mint will increase the number of tokens minted by the user. As public mint and presale mint periods can overlap each other, the number of tokens minted by the user can reach both number of presale allowed to this user and allowed per address by using only public mints. So, the number of presale mints allowed to user can be reached without using presale mint.
142+
143+
Then, presale user will not be able to use its presale mints because of the require statement.
144+
145+
### Vulnerable code
146+
- File: `HeyMintERC1155ExtensionC.sol`
147+
- Function: `presaleMint`
148+
- Lines: `167-180`
149+
- Explanations: `tokensMintedByAddress[msg.sender][_tokenId]` is checked against `presaleMintsAllowedPerAddress`, but it doesn't count only presale mints. Same for `_maximumAllowedMints`.
150+
- Vulnerable code:
151+
```solidity
152+
require(
153+
tokenConfig.presaleMintsAllowedPerAddress == 0 ||
154+
state.data.tokensMintedByAddress[msg.sender][_tokenId] +
155+
_numTokens <=
156+
tokenConfig.presaleMintsAllowedPerAddress,
157+
"MAX_MINTS_PER_ADDRESS_EXCEEDED"
158+
);
159+
require(
160+
_maximumAllowedMints == 0 ||
161+
state.data.tokensMintedByAddress[msg.sender][_tokenId] +
162+
_numTokens <=
163+
_maximumAllowedMints,
164+
"MAX_MINTS_EXCEEDED"
165+
);
166+
```
167+
168+
### Vulnerability test
169+
Multiple unit tests for this vulnerability can be found in `ZigturAudit.js`, under the names:
170+
- `Presale users can lose their presale mint, if they use normal mint - mintToken and MAX_MINTS_PER_ADDRESS_EXCEEDED case`
171+
- `Presale users can lose their presale mint, if they use normal mint - mintToken and MAX_MINTS_EXCEEDED case`
172+
- `Presale users can lose their presale mint, if they use normal mint - freeClaim and MAX_MINTS_PER_ADDRESS_EXCEEDED case`
173+
174+
*Note: only `mintToken` and `freeClaim` have been tested for public minting, but `burnToMint` should work too.*
175+
176+
177+
### Solution
178+
One way to fix it:
179+
- Add a presaleTotalSupply field
180+
- Modify checks in presaleMint()
181+
- Increment tokensMintedByAddress AND presaleTokensMintedByAddress when presale Minting
182+
183+
#### First solution
184+
*Note: this solution is pretty similar to the first solution of [Presale users can lose presale mints because of other users](#first-solution).*
185+
One way to solve the problem is to:
186+
- Add a `presaleTokensMintedByAddress` field, to keep track of how many presale tokens were minted by user
187+
- Modify checks in `presaleMint()` to verify that the new `presaleTokensMintedByAddress` is not greater than `_maximumAllowedMints` and `presaleMintsAllowedPerAddress`
188+
- Increment `tokensMintedByAddress` AND `presaleTokensMintedByAddress` when presale minting
189+
190+
#### Second solution
191+
Another way to fix the issue is by not overlapping presale mint and public mint periods.
192+
193+
It is worth saying that this solution **is not** the most user-friendly one.
194+
195+
196+
## User can max out tokenPublicMintsAllowedPerAddress by using creditCardMint
197+
198+
**Vulnerability classification: Medium**
199+
200+
### Explanations
201+
In the `creditCardMint` function, a check is done to verify that the address to which tokens will be minted is less or equal than tokensMintedByAddress.
202+
203+
The problem is that when tokens are minted in this function, it is the creditCardAddress's tokensMintedByAddress field that is increased (i.e. the `msg.sender` here), and not the user's tokensMintedByAddress field (i.e. the `_to` address in this function).
204+
205+
So, user can mint more than the tokensMintedByAddress limit using creditCard by buying multiple times (multiple transactions).
206+
207+
### Vulnerable code
208+
- File: `HeyMintERC1155ExtensionE.sol`
209+
- Function: `creditCardMint`
210+
- Lines: `160`
211+
- Explanations: `state.data.tokensMintedByAddress[msg.sender][_tokenId]` is increased. But `msg.sender` isn't really the minter. The minter should be `_to`.
212+
- Vulnerable code:
213+
```solidity
214+
require(
215+
state.tokens[_tokenId].publicMintsAllowedPerAddress == 0 ||
216+
state.data.tokensMintedByAddress[_to][_tokenId] + _numTokens <=
217+
state.tokens[_tokenId].publicMintsAllowedPerAddress,
218+
"MAX_MINTS_EXCEEDED"
219+
);
220+
221+
// ...
222+
223+
state.data.totalSupply[_tokenId] += _numTokens;
224+
// Line 160 below
225+
state.data.tokensMintedByAddress[msg.sender][_tokenId] += _numTokens;
226+
_mint(_to, _tokenId, _numTokens, "");
227+
```
228+
229+
### Vulnerability test
230+
The unit test for this vulnerability can be found in `ZigturAudit.js`, under the name `User can max out tokenPublicMintsAllowedPerAddress by using creditCardMint`.
231+
232+
### Solution
233+
- Explanations: Modify line `160` to increase mint balance of the `_to` address instead of the mint balance of the `msg.sender`.
234+
- Fixed code:
235+
```solidity
236+
require(
237+
state.tokens[_tokenId].publicMintsAllowedPerAddress == 0 ||
238+
state.data.tokensMintedByAddress[_to][_tokenId] + _numTokens <=
239+
state.tokens[_tokenId].publicMintsAllowedPerAddress,
240+
"MAX_MINTS_EXCEEDED"
241+
);
242+
243+
// ...
244+
245+
state.data.totalSupply[_tokenId] += _numTokens;
246+
// line 160 fixed
247+
state.data.tokensMintedByAddress[_to][_tokenId] += _numTokens;
248+
_mint(_to, _tokenId, _numTokens, "");
249+
```
250+
251+
252+
253+
254+

‎ERC721/AddressRelay.sol/AddressRelay.md

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
# AddressRelay.sol
2+
3+
## getImplAddress(bytes4 _functionSelector)
4+
5+
### Unneeded require statement
6+
Type: Code optimization
7+
8+
This has no real impact, as it is a view function.
9+
10+
11+
![Unneeded require](images/getImplAddress_unneeded-require.PNG)
12+
13+
The require statement at the end of the function is not needed. Right before, the fallbackImplAddress is set if `implAddress == address(0)`, so `implAddress` will never be `address(0)` here.
14+
15+
16+

‎ERC721/AddressRelay.sol/images/getImplAddress_unneeded-require.PNG

12.1 KB
Loading

0 commit comments

Comments
 (0)