Skip to content

Commit df3280b

Browse files
kumaryash90kumaryash90
authored
Token audit macro (#249)
* (C-1) mintWithSignature() front-running vulnerability * (M-1) TokenERC20 charges price when quantity is 0 * (M-2) Native tokens can get locked in the contract * (M-3) Use _safeMint instead of _mint * (L-1) platformFeeBps is not bounded * (Q-1) Documentation discrepancy for price in TokenERC20.sol * (Q-3) TokenERC721 can have empty URI * (G-1) Calldata parameters * (Q-4) Inconsistency: NATIVE_TOKEN * run prettier * (Q-4) Inconsistency: collectPrice params in TokenERC20 * (Q-4) Inconsistency: platformFeeBps visibility * (Q-4) Inconsistency: reentrancy guard init in TokenERC20 * (Q-2) Inconsistency between Token contracts regarding pause/unpause functionality
1 parent ff037c9 commit df3280b

File tree

7 files changed

+151
-111
lines changed

7 files changed

+151
-111
lines changed

‎contracts/token/TokenERC1155.sol‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,6 @@ contract TokenERC1155 is
7171
/// @dev Max bps in the thirdweb system
7272
uint256 private constant MAX_BPS = 10_000;
7373

74-
/// @dev The address interpreted as native token of the chain.
75-
address private constant NATIVE_TOKEN = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;
76-
7774
/// @dev Owner of the contract (purpose: OpenSea compatibility, etc.)
7875
address private _owner;
7976

@@ -93,7 +90,7 @@ contract TokenERC1155 is
9390
uint128 private royaltyBps;
9491

9592
/// @dev The % of primary sales collected by the contract as fees.
96-
uint128 public platformFeeBps;
93+
uint128 private platformFeeBps;
9794

9895
/// @dev Contract level metadata.
9996
string public contractURI;
@@ -141,6 +138,8 @@ contract TokenERC1155 is
141138
platformFeeRecipient = _platformFeeRecipient;
142139
primarySaleRecipient = _primarySaleRecipient;
143140
contractURI = _contractURI;
141+
142+
require(_platformFeeBps <= MAX_BPS, "exceeds MAX_BPS");
144143
platformFeeBps = _platformFeeBps;
145144

146145
_owner = _defaultAdmin;
@@ -217,7 +216,7 @@ contract TokenERC1155 is
217216
/// @dev Mints an NFT according to the provided mint request.
218217
function mintWithSignature(MintRequest calldata _req, bytes calldata _signature) external payable nonReentrant {
219218
address signer = verifyRequest(_req, _signature);
220-
address receiver = _req.to == address(0) ? _msgSender() : _req.to;
219+
address receiver = _req.to;
221220

222221
uint256 tokenIdToMint;
223222
if (_req.tokenId == type(uint256).max) {
@@ -281,7 +280,7 @@ contract TokenERC1155 is
281280
external
282281
onlyRole(DEFAULT_ADMIN_ROLE)
283282
{
284-
require(_platformFeeBps <= MAX_BPS, "bps <= 10000.");
283+
require(_platformFeeBps <= MAX_BPS, "exceeds MAX_BPS");
285284

286285
platformFeeBps = uint64(_platformFeeBps);
287286
platformFeeRecipient = _platformFeeRecipient;
@@ -378,23 +377,27 @@ contract TokenERC1155 is
378377
_req.validityStartTimestamp <= block.timestamp && _req.validityEndTimestamp >= block.timestamp,
379378
"request expired"
380379
);
380+
require(_req.to != address(0), "recipient undefined");
381+
require(_req.quantity > 0, "zero quantity");
381382

382383
minted[_req.uid] = true;
383384

384385
return signer;
385386
}
386387

387388
/// @dev Collects and distributes the primary sale value of tokens being claimed.
388-
function collectPrice(MintRequest memory _req) internal {
389+
function collectPrice(MintRequest calldata _req) internal {
389390
if (_req.pricePerToken == 0) {
390391
return;
391392
}
392393

393394
uint256 totalPrice = _req.pricePerToken * _req.quantity;
394395
uint256 platformFees = (totalPrice * platformFeeBps) / MAX_BPS;
395396

396-
if (_req.currency == NATIVE_TOKEN) {
397+
if (_req.currency == CurrencyTransferLib.NATIVE_TOKEN) {
397398
require(msg.value == totalPrice, "must send total price.");
399+
} else {
400+
require(msg.value == 0, "msg value not zero");
398401
}
399402

400403
address saleRecipient = _req.primarySaleRecipient == address(0)

‎contracts/token/TokenERC20.sol‎

Lines changed: 23 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import "../extension/interface/IPrimarySale.sol";
1010

1111
// Token
1212
import "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20BurnableUpgradeable.sol";
13-
import "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20PausableUpgradeable.sol";
1413
import "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20VotesUpgradeable.sol";
1514

1615
// Security
@@ -38,7 +37,6 @@ contract TokenERC20 is
3837
ERC2771ContextUpgradeable,
3938
MulticallUpgradeable,
4039
ERC20BurnableUpgradeable,
41-
ERC20PausableUpgradeable,
4240
ERC20VotesUpgradeable,
4341
ITokenERC20,
4442
AccessControlEnumerableUpgradeable
@@ -54,7 +52,6 @@ contract TokenERC20 is
5452
);
5553

5654
bytes32 internal constant MINTER_ROLE = keccak256("MINTER_ROLE");
57-
bytes32 internal constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
5855
bytes32 internal constant TRANSFER_ROLE = keccak256("TRANSFER_ROLE");
5956

6057
/// @dev Returns the URI for the storefront-level metadata of the contract.
@@ -64,7 +61,7 @@ contract TokenERC20 is
6461
uint128 internal constant MAX_BPS = 10_000;
6562

6663
/// @dev The % of primary sales collected by the contract as fees.
67-
uint128 internal platformFeeBps;
64+
uint128 private platformFeeBps;
6865

6966
/// @dev The adress that receives all primary sales value.
7067
address internal platformFeeRecipient;
@@ -88,19 +85,21 @@ contract TokenERC20 is
8885
address _platformFeeRecipient,
8986
uint256 _platformFeeBps
9087
) external initializer {
88+
__ReentrancyGuard_init();
9189
__ERC2771Context_init_unchained(_trustedForwarders);
9290
__ERC20Permit_init(_name);
9391
__ERC20_init_unchained(_name, _symbol);
9492

9593
contractURI = _contractURI;
9694
primarySaleRecipient = _primarySaleRecipient;
9795
platformFeeRecipient = _platformFeeRecipient;
96+
97+
require(_platformFeeBps <= MAX_BPS, "exceeds MAX_BPS");
9898
platformFeeBps = uint128(_platformFeeBps);
9999

100100
_setupRole(DEFAULT_ADMIN_ROLE, _defaultAdmin);
101101
_setupRole(TRANSFER_ROLE, _defaultAdmin);
102102
_setupRole(MINTER_ROLE, _defaultAdmin);
103-
_setupRole(PAUSER_ROLE, _defaultAdmin);
104103
_setupRole(TRANSFER_ROLE, address(0));
105104
}
106105

@@ -127,7 +126,7 @@ contract TokenERC20 is
127126
address from,
128127
address to,
129128
uint256 amount
130-
) internal override(ERC20Upgradeable, ERC20PausableUpgradeable) {
129+
) internal override {
131130
super._beforeTokenTransfer(from, to, amount);
132131

133132
if (!hasRole(TRANSFER_ROLE, address(0)) && from != address(0) && to != address(0)) {
@@ -166,12 +165,9 @@ contract TokenERC20 is
166165
/// @dev Mints tokens according to the provided mint request.
167166
function mintWithSignature(MintRequest calldata _req, bytes calldata _signature) external payable nonReentrant {
168167
address signer = verifyRequest(_req, _signature);
169-
address receiver = _req.to == address(0) ? _msgSender() : _req.to;
170-
address saleRecipient = _req.primarySaleRecipient == address(0)
171-
? primarySaleRecipient
172-
: _req.primarySaleRecipient;
168+
address receiver = _req.to;
173169

174-
collectPrice(saleRecipient, _req.currency, _req.price);
170+
collectPrice(_req);
175171

176172
_mintTo(receiver, _req.quantity);
177173

@@ -189,7 +185,7 @@ contract TokenERC20 is
189185
external
190186
onlyRole(DEFAULT_ADMIN_ROLE)
191187
{
192-
require(_platformFeeBps <= MAX_BPS, "bps <= 10000.");
188+
require(_platformFeeBps <= MAX_BPS, "exceeds MAX_BPS");
193189

194190
platformFeeBps = uint64(_platformFeeBps);
195191
platformFeeRecipient = _platformFeeRecipient;
@@ -203,23 +199,25 @@ contract TokenERC20 is
203199
}
204200

205201
/// @dev Collects and distributes the primary sale value of tokens being claimed.
206-
function collectPrice(
207-
address _primarySaleRecipient,
208-
address _currency,
209-
uint256 _price
210-
) internal {
211-
if (_price == 0) {
202+
function collectPrice(MintRequest calldata _req) internal {
203+
if (_req.price == 0) {
212204
return;
213205
}
214206

215-
uint256 platformFees = (_price * platformFeeBps) / MAX_BPS;
207+
uint256 platformFees = (_req.price * platformFeeBps) / MAX_BPS;
216208

217-
if (_currency == CurrencyTransferLib.NATIVE_TOKEN) {
218-
require(msg.value == _price, "must send total price.");
209+
if (_req.currency == CurrencyTransferLib.NATIVE_TOKEN) {
210+
require(msg.value == _req.price, "must send total price.");
211+
} else {
212+
require(msg.value == 0, "msg value not zero");
219213
}
220214

221-
CurrencyTransferLib.transferCurrency(_currency, _msgSender(), platformFeeRecipient, platformFees);
222-
CurrencyTransferLib.transferCurrency(_currency, _msgSender(), _primarySaleRecipient, _price - platformFees);
215+
address saleRecipient = _req.primarySaleRecipient == address(0)
216+
? primarySaleRecipient
217+
: _req.primarySaleRecipient;
218+
219+
CurrencyTransferLib.transferCurrency(_req.currency, _msgSender(), platformFeeRecipient, platformFees);
220+
CurrencyTransferLib.transferCurrency(_req.currency, _msgSender(), saleRecipient, _req.price - platformFees);
223221
}
224222

225223
/// @dev Mints `amount` of tokens to `to`
@@ -237,6 +235,8 @@ contract TokenERC20 is
237235
_req.validityStartTimestamp <= block.timestamp && _req.validityEndTimestamp >= block.timestamp,
238236
"request expired"
239237
);
238+
require(_req.to != address(0), "recipient undefined");
239+
require(_req.quantity > 0, "zero quantity");
240240

241241
minted[_req.uid] = true;
242242

@@ -264,34 +264,6 @@ contract TokenERC20 is
264264
);
265265
}
266266

267-
/**
268-
* @dev Pauses all token transfers.
269-
*
270-
* See {ERC20Pausable} and {Pausable-_pause}.
271-
*
272-
* Requirements:
273-
*
274-
* - the caller must have the `PAUSER_ROLE`.
275-
*/
276-
function pause() public virtual {
277-
require(hasRole(PAUSER_ROLE, _msgSender()), "not pauser.");
278-
_pause();
279-
}
280-
281-
/**
282-
* @dev Unpauses all token transfers.
283-
*
284-
* See {ERC20Pausable} and {Pausable-_unpause}.
285-
*
286-
* Requirements:
287-
*
288-
* - the caller must have the `PAUSER_ROLE`.
289-
*/
290-
function unpause() public virtual {
291-
require(hasRole(PAUSER_ROLE, _msgSender()), "not pauser.");
292-
_unpause();
293-
}
294-
295267
/// @dev Sets contract URI for the storefront-level metadata of the contract.
296268
function setContractURI(string calldata _uri) external onlyRole(DEFAULT_ADMIN_ROLE) {
297269
contractURI = _uri;

‎contracts/token/TokenERC721.sol‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -67,9 +67,6 @@ contract TokenERC721 is
6767
/// @dev Max bps in the thirdweb system
6868
uint256 private constant MAX_BPS = 10_000;
6969

70-
/// @dev The address interpreted as native token of the chain.
71-
address private constant NATIVE_TOKEN = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;
72-
7370
/// @dev Owner of the contract (purpose: OpenSea compatibility, etc.)
7471
address private _owner;
7572

@@ -89,7 +86,7 @@ contract TokenERC721 is
8986
uint128 private royaltyBps;
9087

9188
/// @dev The % of primary sales collected by the contract as fees.
92-
uint128 public platformFeeBps;
89+
uint128 private platformFeeBps;
9390

9491
/// @dev Contract level metadata.
9592
string public contractURI;
@@ -130,6 +127,8 @@ contract TokenERC721 is
130127
platformFeeRecipient = _platformFeeRecipient;
131128
primarySaleRecipient = _saleRecipient;
132129
contractURI = _contractURI;
130+
131+
require(_platformFeeBps <= MAX_BPS, "exceeds MAX_BPS");
133132
platformFeeBps = _platformFeeBps;
134133

135134
_owner = _defaultAdmin;
@@ -197,7 +196,7 @@ contract TokenERC721 is
197196
returns (uint256 tokenIdMinted)
198197
{
199198
address signer = verifyRequest(_req, _signature);
200-
address receiver = _req.to == address(0) ? _msgSender() : _req.to;
199+
address receiver = _req.to;
201200

202201
tokenIdMinted = _mintTo(receiver, _req.uri);
203202

@@ -252,7 +251,7 @@ contract TokenERC721 is
252251
external
253252
onlyRole(DEFAULT_ADMIN_ROLE)
254253
{
255-
require(_platformFeeBps <= MAX_BPS, "bps <= 10000.");
254+
require(_platformFeeBps <= MAX_BPS, "exceeds MAX_BPS");
256255

257256
platformFeeBps = uint64(_platformFeeBps);
258257
platformFeeRecipient = _platformFeeRecipient;
@@ -303,9 +302,10 @@ contract TokenERC721 is
303302
tokenIdToMint = nextTokenIdToMint;
304303
nextTokenIdToMint += 1;
305304

305+
require(bytes(_uri).length > 0, "empty uri.");
306306
uri[tokenIdToMint] = _uri;
307307

308-
_mint(_to, tokenIdToMint);
308+
_safeMint(_to, tokenIdToMint);
309309

310310
emit TokensMinted(_to, tokenIdToMint, _uri);
311311
}
@@ -342,23 +342,26 @@ contract TokenERC721 is
342342
_req.validityStartTimestamp <= block.timestamp && _req.validityEndTimestamp >= block.timestamp,
343343
"request expired"
344344
);
345+
require(_req.to != address(0), "recipient undefined");
345346

346347
minted[_req.uid] = true;
347348

348349
return signer;
349350
}
350351

351352
/// @dev Collects and distributes the primary sale value of tokens being claimed.
352-
function collectPrice(MintRequest memory _req) internal {
353+
function collectPrice(MintRequest calldata _req) internal {
353354
if (_req.price == 0) {
354355
return;
355356
}
356357

357358
uint256 totalPrice = _req.price;
358359
uint256 platformFees = (totalPrice * platformFeeBps) / MAX_BPS;
359360

360-
if (_req.currency == NATIVE_TOKEN) {
361+
if (_req.currency == CurrencyTransferLib.NATIVE_TOKEN) {
361362
require(msg.value == totalPrice, "must send total price.");
363+
} else {
364+
require(msg.value == 0, "msg value not zero");
362365
}
363366

364367
address saleRecipient = _req.primarySaleRecipient == address(0)

‎contracts/token/signatureMint.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ struct MintRequest {
6666
| tokenId | The tokenId of the token to mint. (Only applicable for ERC1155 tokens)|
6767
| uri | The metadata URI of the token to mint. (Not applicable for ERC20 tokens)|
6868
| quantity | The quantity of tokens to mint.|
69-
| pricePerToken | The price to pay per quantity of tokens minted.|
69+
| pricePerToken | The price to pay per quantity of tokens minted. (For TokenERC20, this parameter is `price`, indicating the total price of all tokens)|
7070
| currency | The currency in which to pay the price per token minted.|
7171
| validityStartTimestamp | The unix timestamp after which the payload is valid.|
7272
| validityEndTimestamp | The unix timestamp at which the payload expires.|

‎src/test/token/TokenERC1155.t.sol‎

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,19 @@ contract TokenERC1155Test is BaseTest {
263263
tokenContract.mintWithSignature{ value: 0 }(_mintrequest, _signature);
264264
}
265265

266+
function test_revert_mintWithSignature_MsgValueNotZero() public {
267+
vm.warp(1000);
268+
269+
_mintrequest.pricePerToken = 1;
270+
_mintrequest.currency = address(erc20);
271+
_signature = signMintRequest(_mintrequest, privateKey);
272+
273+
// shouldn't send native-token when it is not the currency
274+
vm.prank(recipient);
275+
vm.expectRevert("msg value not zero");
276+
tokenContract.mintWithSignature{ value: 1 }(_mintrequest, _signature);
277+
}
278+
266279
function test_revert_mintWithSignature_InvalidSignature() public {
267280
vm.warp(1000);
268281

@@ -285,6 +298,28 @@ contract TokenERC1155Test is BaseTest {
285298
tokenContract.mintWithSignature(_mintrequest, _signature);
286299
}
287300

301+
function test_revert_mintWithSignature_RecipientUndefined() public {
302+
vm.warp(1000);
303+
304+
_mintrequest.to = address(0);
305+
_signature = signMintRequest(_mintrequest, privateKey);
306+
307+
vm.prank(recipient);
308+
vm.expectRevert("recipient undefined");
309+
tokenContract.mintWithSignature(_mintrequest, _signature);
310+
}
311+
312+
function test_revert_mintWithSignature_ZeroQuantity() public {
313+
vm.warp(1000);
314+
315+
_mintrequest.quantity = 0;
316+
_signature = signMintRequest(_mintrequest, privateKey);
317+
318+
vm.prank(recipient);
319+
vm.expectRevert("zero quantity");
320+
tokenContract.mintWithSignature(_mintrequest, _signature);
321+
}
322+
288323
function test_event_mintWithSignature() public {
289324
vm.warp(1000);
290325

@@ -609,7 +644,7 @@ contract TokenERC1155Test is BaseTest {
609644
address _platformFeeRecipient = address(0x123);
610645
uint256 _platformFeeBps = 10001;
611646

612-
vm.expectRevert("bps <= 10000.");
647+
vm.expectRevert("exceeds MAX_BPS");
613648
vm.prank(deployerSigner);
614649
tokenContract.setPlatformFeeInfo(_platformFeeRecipient, _platformFeeBps);
615650
}

0 commit comments

Comments
 (0)