Skip to content

Commit

Permalink
Audit fixes (#173)
Browse files Browse the repository at this point in the history 
* [M-1] The SequentialTokenIdERC1155 module fails to apply the correct tokenId when installed after initial minting

- updated sequentialTokenIdERC1155 to now be able to initialize nextTokenId

* [H-1] ClaimableERC20 and MintableERC20 modules incorrectly handle tokens with decimals other than 18

- uses IERC20.decimals instead of 1e18

* [H-2] Claimable modules lead to storage collisions when being updgraded

* [H-3] BatchMetadata modules may apply baseURI to incorrect token ids

* [Q-1] FallbackFunction array of Claimable modules can be reduced

* [Q-2] Nitpicks
  • Loading branch information
@GWSzeto
GWSzeto authored Oct 9, 2024
1 parent 8f45917 commit be6f40a
Show file tree
Hide file tree
Showing 16 changed files with 370 additions and 199 deletions.
1 change: 1 addition & 0 deletions src/callback/BeforeMintCallbackERC721.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ contract BeforeMintCallbackERC721 {
* @notice The beforeMintERC721 hook that is called by a core token before minting tokens.
*
* @param _to The address that is minting tokens.
* @param _startTokenId The token ID being minted.
* @param _amount The amount of tokens to mint.
* @param _data Optional extra data passed to the hook.
* @return result Abi encoded bytes result of the hook.
Expand Down
1 change: 1 addition & 0 deletions src/callback/BeforeMintWithSignatureCallbackERC1155.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ contract BeforeMintWithSignatureCallbackERC1155 {
* @notice The beforeMintWithSignatureERC1155 hook that is called by a core token before minting tokens.
*
* @param _to The address that is minting tokens.
* @param _id The token ID being minted.
* @param _amount The quantity of tokens to mint.
* @param _data Optional extra data passed to the hook.
* @param _signer The address that signed the minting request.
Expand Down
1 change: 1 addition & 0 deletions src/callback/BeforeMintWithSignatureCallbackERC721.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ contract BeforeMintWithSignatureCallbackERC721 {
* @notice The beforeMintWithSignatureERC721 hook that is called by a core token before minting tokens.
*
* @param _to The address that is minting tokens.
* @param _startTokenId The token ID being minted.
* @param _amount The amount of tokens to mint.
* @param _data Optional extra data passed to the hook.
* @param _signer The address that signed the minting request.
Expand Down
4 changes: 4 additions & 0 deletions src/core/token/ERC20Base.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,10 @@ contract ERC20Base is ERC20, Multicallable, Core, EIP712 {
* @param owner The account approving the tokens
* @param spender The address to approve
* @param amount Amount of tokens to approve
* @param deadline Deadline after which the approval is no longer valid
* @param v Signature param
* @param r Signature param
* @param s Signature param
*/
function permit(address owner, address spender, uint256 amount, uint256 deadline, uint8 v, bytes32 r, bytes32 s)
public
Expand Down
8 changes: 4 additions & 4 deletions src/module/token/metadata/BatchMetadataERC1155.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ contract BatchMetadataERC1155 is BatchMetadataERC721, UpdateMetadataCallbackERC1
FallbackFunction({selector: this.setBaseURI.selector, permissionBits: Role._MANAGER_ROLE});
config.fallbackFunctions[2] =
FallbackFunction({selector: this.getAllMetadataBatches.selector, permissionBits: 0});
config.fallbackFunctions[3] = FallbackFunction({selector: this.nextTokenIdToMint.selector, permissionBits: 0});
config.fallbackFunctions[4] = FallbackFunction({selector: this.getBatchId.selector, permissionBits: 0});
config.fallbackFunctions[5] = FallbackFunction({selector: this.getBatchRange.selector, permissionBits: 0});
config.fallbackFunctions[3] = FallbackFunction({selector: this.getMetadataBatch.selector, permissionBits: 0});
config.fallbackFunctions[4] = FallbackFunction({selector: this.nextTokenIdToMint.selector, permissionBits: 0});
config.fallbackFunctions[5] = FallbackFunction({selector: this.getBatchIndex.selector, permissionBits: 0});

config.requiredInterfaces = new bytes4[](1);
config.requiredInterfaces[0] = 0xd9b67a26; // ERC1155
Expand All @@ -44,7 +44,7 @@ contract BatchMetadataERC1155 is BatchMetadataERC721, UpdateMetadataCallbackERC1
if (_startTokenId < _batchMetadataStorage().nextTokenIdRangeStart) {
revert BatchMetadataMetadataAlreadySet();
}
_setMetadata(_quantity, _baseURI);
_setMetadata(_startTokenId, _quantity, _baseURI);
}

}
110 changes: 39 additions & 71 deletions src/module/token/metadata/BatchMetadataERC721.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,10 @@ library BatchMetadataStorage {
keccak256(abi.encode(uint256(keccak256("token.metadata.batch")) - 1)) & ~bytes32(uint256(0xff));

struct Data {
// tokenId range end
uint256[] tokenIdRangeEnd;
// next tokenId as range start
uint256 nextTokenIdRangeStart;
// tokenId range end => baseURI of range
mapping(uint256 => string) baseURIOfTokenIdRange;
// metadata batches
BatchMetadataERC721.MetadataBatch[] metadataBatches;
}

function data() internal pure returns (Data storage data_) {
Expand All @@ -42,7 +40,7 @@ contract BatchMetadataERC721 is Module, UpdateMetadataCallbackERC721 {
/**
* @notice MetadataBatch struct to store metadata for a range of tokenIds.
* @param startTokenIdInclusive The first tokenId in the range.
* @param endTokenIdNonInclusive The last tokenId in the range.
* @param endTokenIdInclusive The last tokenId in the range.
* @param baseURI The base URI for the range.
*/
struct MetadataBatch {
Expand All @@ -69,7 +67,7 @@ contract BatchMetadataERC721 is Module, UpdateMetadataCallbackERC721 {
//////////////////////////////////////////////////////////////*/

/// @dev ERC-4906 Metadata Update.
event BatchMetadataUpdate(uint256 _fromTokenId, uint256 _toTokenId);
event BatchMetadataUpdate(uint256 startTokenIdIncluside, uint256 endTokenIdInclusive, string baseURI);

/*//////////////////////////////////////////////////////////////
MODULE CONFIG
Expand All @@ -89,9 +87,9 @@ contract BatchMetadataERC721 is Module, UpdateMetadataCallbackERC721 {
FallbackFunction({selector: this.setBaseURI.selector, permissionBits: Role._MANAGER_ROLE});
config.fallbackFunctions[2] =
FallbackFunction({selector: this.getAllMetadataBatches.selector, permissionBits: 0});
config.fallbackFunctions[3] = FallbackFunction({selector: this.nextTokenIdToMint.selector, permissionBits: 0});
config.fallbackFunctions[4] = FallbackFunction({selector: this.getBatchId.selector, permissionBits: 0});
config.fallbackFunctions[5] = FallbackFunction({selector: this.getBatchRange.selector, permissionBits: 0});
config.fallbackFunctions[3] = FallbackFunction({selector: this.getMetadataBatch.selector, permissionBits: 0});
config.fallbackFunctions[4] = FallbackFunction({selector: this.nextTokenIdToMint.selector, permissionBits: 0});
config.fallbackFunctions[5] = FallbackFunction({selector: this.getBatchIndex.selector, permissionBits: 0});

config.requiredInterfaces = new bytes4[](1);
config.requiredInterfaces[0] = 0x80ac58cd; // ERC721.
Expand Down Expand Up @@ -121,7 +119,7 @@ contract BatchMetadataERC721 is Module, UpdateMetadataCallbackERC721 {
if (_startTokenId < _batchMetadataStorage().nextTokenIdRangeStart) {
revert BatchMetadataMetadataAlreadySet();
}
_setMetadata(_quantity, _baseURI);
_setMetadata(_startTokenId, _quantity, _baseURI);
}

/*//////////////////////////////////////////////////////////////
Expand All @@ -130,71 +128,42 @@ contract BatchMetadataERC721 is Module, UpdateMetadataCallbackERC721 {

/// @notice Returns all metadata batches for a token.
function getAllMetadataBatches() external view returns (MetadataBatch[] memory) {
uint256[] memory rangeEnds = _batchMetadataStorage().tokenIdRangeEnd;
uint256 numOfBatches = rangeEnds.length;

MetadataBatch[] memory batches = new MetadataBatch[](rangeEnds.length);

uint256 rangeStart = 0;
for (uint256 i = 0; i < numOfBatches; i += 1) {
batches[i] = MetadataBatch({
startTokenIdInclusive: rangeStart,
endTokenIdInclusive: rangeEnds[i] - 1,
baseURI: _batchMetadataStorage().baseURIOfTokenIdRange[rangeEnds[i]]
});
rangeStart = rangeEnds[i];
}
return _batchMetadataStorage().metadataBatches;
}

return batches;
/// @dev returns the metadata batch for a given batchIndex
function getMetadataBatch(uint256 _batchIndex) public view returns (MetadataBatch memory) {
return _batchMetadataStorage().metadataBatches[_batchIndex];
}

/// @notice Uploads metadata for a range of tokenIds.
function uploadMetadata(uint256 _amount, string calldata _baseURI) external virtual {
_setMetadata(_amount, _baseURI);
_setMetadata(_batchMetadataStorage().nextTokenIdRangeStart, _amount, _baseURI);
}

function nextTokenIdToMint() external view returns (uint256) {
return _batchMetadataStorage().nextTokenIdRangeStart;
}

/// @dev Returns the id for the batch of tokens the given tokenId belongs to.
function getBatchId(uint256 _tokenId) public view virtual returns (uint256 batchId, uint256 index) {
uint256[] memory rangeEnds = _batchMetadataStorage().tokenIdRangeEnd;
uint256 numOfBatches = rangeEnds.length;

for (uint256 i = 0; i < numOfBatches; i += 1) {
if (_tokenId < rangeEnds[i]) {
index = i;
batchId = rangeEnds[i];

return (batchId, index);
}
}
revert BatchMetadataNoMetadataForTokenId();
}

/// @dev returns the starting tokenId of a given batchId.
function getBatchRange(uint256 _batchID) public view returns (uint256, uint256) {
uint256[] memory rangeEnds = _batchMetadataStorage().tokenIdRangeEnd;
uint256 numOfBatches = rangeEnds.length;
/// @dev Returns the index for the batch of tokens the given tokenId belongs to.
function getBatchIndex(uint256 _tokenId) public view virtual returns (uint256) {
MetadataBatch[] memory batches = _batchMetadataStorage().metadataBatches;
uint256 numOfBatches = batches.length;

for (uint256 i = 0; i < numOfBatches; i += 1) {
if (_batchID == rangeEnds[i]) {
if (i > 0) {
return (rangeEnds[i - 1], rangeEnds[i] - 1);
}
return (0, rangeEnds[i] - 1);
if (_tokenId >= batches[i].startTokenIdInclusive && _tokenId <= batches[i].endTokenIdInclusive) {
return i;
}
}

revert BatchMetadataNoMetadataForTokenId();
}

/// @dev Sets the base URI for the batch of tokens with the given batchId.
function setBaseURI(uint256 _batchId, string memory _baseURI) external virtual {
_batchMetadataStorage().baseURIOfTokenIdRange[_batchId] = _baseURI;
(uint256 startTokenId,) = getBatchRange(_batchId);
emit BatchMetadataUpdate(startTokenId, _batchId);
/// @dev Sets the base URI for the batch based on the batchIndex.
function setBaseURI(uint256 _batchIndex, string memory _baseURI) external virtual {
MetadataBatch memory batch = _batchMetadataStorage().metadataBatches[_batchIndex];
batch.baseURI = _baseURI;
_batchMetadataStorage().metadataBatches[_batchIndex] = batch;
emit BatchMetadataUpdate(batch.startTokenIdInclusive, batch.endTokenIdInclusive, batch.baseURI);
}

/*//////////////////////////////////////////////////////////////
Expand All @@ -203,35 +172,34 @@ contract BatchMetadataERC721 is Module, UpdateMetadataCallbackERC721 {

/// @dev Returns the baseURI for a token. The intended metadata URI for the token is baseURI + indexInBatch.
function _getBaseURI(uint256 _tokenId) internal view returns (string memory baseUri, uint256 indexInBatch) {
uint256[] memory rangeEnds = _batchMetadataStorage().tokenIdRangeEnd;
uint256 numOfBatches = rangeEnds.length;
BatchMetadataERC721.MetadataBatch[] memory batches = _batchMetadataStorage().metadataBatches;
uint256 numOfBatches = batches.length;

for (uint256 i = 0; i < numOfBatches; i += 1) {
if (_tokenId < rangeEnds[i]) {
uint256 rangeStart = 0;
if (i > 0) {
rangeStart = rangeEnds[i - 1];
}
return (_batchMetadataStorage().baseURIOfTokenIdRange[rangeEnds[i]], _tokenId - rangeStart);
if (_tokenId >= batches[i].startTokenIdInclusive && _tokenId <= batches[i].endTokenIdInclusive) {
return (batches[i].baseURI, _tokenId - batches[i].startTokenIdInclusive);
}
}
revert BatchMetadataNoMetadataForTokenId();
}

/// @notice sets the metadata for a range of tokenIds.
function _setMetadata(uint256 _amount, string calldata _baseURI) internal virtual {
function _setMetadata(uint256 _rangeStart, uint256 _amount, string calldata _baseURI) internal virtual {
if (_amount == 0) {
revert BatchMetadataZeroAmount();
}

uint256 rangeStart = _batchMetadataStorage().nextTokenIdRangeStart;
uint256 rangeEndNonInclusive = rangeStart + _amount;
uint256 rangeEndNonInclusive = _rangeStart + _amount;

MetadataBatch memory batch = MetadataBatch({
startTokenIdInclusive: _rangeStart,
endTokenIdInclusive: rangeEndNonInclusive - 1,
baseURI: _baseURI
});
_batchMetadataStorage().metadataBatches.push(batch);
_batchMetadataStorage().nextTokenIdRangeStart = rangeEndNonInclusive;
_batchMetadataStorage().tokenIdRangeEnd.push(rangeEndNonInclusive);
_batchMetadataStorage().baseURIOfTokenIdRange[rangeEndNonInclusive] = _baseURI;

emit BatchMetadataUpdate(rangeStart, rangeEndNonInclusive - 1);
emit BatchMetadataUpdate(_rangeStart, rangeEndNonInclusive - 1, _baseURI);
}

function _batchMetadataStorage() internal pure returns (BatchMetadataStorage.Data storage) {
Expand Down
10 changes: 5 additions & 5 deletions src/module/token/minting/ClaimableERC1155.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ library ClaimableStorage {
keccak256(abi.encode(uint256(keccak256("token.minting.claimable.erc1155")) - 1)) & ~bytes32(uint256(0xff));

struct Data {
// sale config: primary sale recipient, and platform fee recipient + BPS.
ClaimableERC1155.SaleConfig saleConfig;
// token ID => claim condition
mapping(uint256 => ClaimableERC1155.ClaimCondition) claimConditionByTokenId;
// UID => whether it has been used
mapping(bytes32 => bool) uidUsed;
// address => uint256 => how many tokens have been minted
mapping(address => mapping(uint256 => uint256)) totalMinted;
// sale config: primary sale recipient, and platform fee recipient + BPS.
ClaimableERC1155.SaleConfig saleConfig;
// token ID => claim condition
mapping(uint256 => ClaimableERC1155.ClaimCondition) claimConditionByTokenId;
}

function data() internal pure returns (Data storage data_) {
Expand Down Expand Up @@ -161,7 +161,7 @@ contract ClaimableERC1155 is
/// @notice Returns all implemented callback and fallback functions.
function getModuleConfig() external pure override returns (ModuleConfig memory config) {
config.callbackFunctions = new CallbackFunction[](2);
config.fallbackFunctions = new FallbackFunction[](5);
config.fallbackFunctions = new FallbackFunction[](4);

config.callbackFunctions[0] = CallbackFunction(this.beforeMintERC1155.selector);
config.callbackFunctions[1] = CallbackFunction(this.beforeMintWithSignatureERC1155.selector);
Expand Down
35 changes: 23 additions & 12 deletions src/module/token/minting/ClaimableERC20.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pragma solidity ^0.8.20;
import {Module} from "../../../Module.sol";

import {Role} from "../../../Role.sol";

import {IInstallationCallback} from "../../../interface/IInstallationCallback.sol";
import {OwnableRoles} from "@solady/auth/OwnableRoles.sol";
import {MerkleProofLib} from "@solady/utils/MerkleProofLib.sol";
Expand All @@ -12,21 +13,27 @@ import {SafeTransferLib} from "@solady/utils/SafeTransferLib.sol";
import {BeforeMintCallbackERC20} from "../../../callback/BeforeMintCallbackERC20.sol";
import {BeforeMintWithSignatureCallbackERC20} from "../../../callback/BeforeMintWithSignatureCallbackERC20.sol";

interface IERC20 {

function decimals() external view returns (uint8);

}

library ClaimableStorage {

/// @custom:storage-location erc7201:token.minting.claimable.erc20
bytes32 public constant CLAIMABLE_STORAGE_POSITION =
keccak256(abi.encode(uint256(keccak256("token.minting.claimable.erc20")) - 1)) & ~bytes32(uint256(0xff));

struct Data {
// sale config: primary sale recipient, and platform fee recipient + BPS.
ClaimableERC20.SaleConfig saleConfig;
// claim condition
ClaimableERC20.ClaimCondition claimCondition;
// UID => whether it has been used
mapping(bytes32 => bool) uidUsed;
// address => how many tokens have been minted
mapping(address => uint256) totalMinted;
// sale config: primary sale recipient, and platform fee recipient + BPS.
ClaimableERC20.SaleConfig saleConfig;
// claim condition
ClaimableERC20.ClaimCondition claimCondition;
}

function data() internal pure returns (Data storage data_) {
Expand Down Expand Up @@ -83,9 +90,8 @@ contract ClaimableERC20 is
*
* @param startTimestamp The timestamp at which the minting request is valid.
* @param endTimestamp The timestamp at which the minting request expires.
* @param recipient The address that will receive the minted tokens.
* @param amount The amount of tokens to mint.
* @param currency The address of the currency used to pay for the minted tokens.
* @param maxMintPerWallet The maximum number of tokens that can be minted per wallet.
* @param pricePerUnit The price per unit of the minted tokens.
* @param uid A unique identifier for the minting request.
*/
Expand All @@ -101,8 +107,9 @@ contract ClaimableERC20 is
/**
* @notice The parameters sent to the `beforeMintERC20` callback function.
*
* @param request The minting request.
* @param signature The signature produced from signing the minting request.
* @param currency The address of the currency used to pay for the minted tokens.
* @param pricePerUnit The price per unit of the minted tokens.
* @param recipientAllowlistProof The proof of the recipient's address in the allowlist.
*/
struct ClaimParamsERC20 {
address currency;
Expand Down Expand Up @@ -160,7 +167,7 @@ contract ClaimableERC20 is
/// @notice Returns all implemented callback and fallback functions.
function getModuleConfig() external pure override returns (ModuleConfig memory config) {
config.callbackFunctions = new CallbackFunction[](2);
config.fallbackFunctions = new FallbackFunction[](5);
config.fallbackFunctions = new FallbackFunction[](4);

config.callbackFunctions[0] = CallbackFunction(this.beforeMintERC20.selector);
config.callbackFunctions[1] = CallbackFunction(this.beforeMintWithSignatureERC20.selector);
Expand Down Expand Up @@ -194,10 +201,12 @@ contract ClaimableERC20 is

_validateClaimCondition(_to, _amount, _params.currency, _params.pricePerUnit, _params.recipientAllowlistProof);

_distributeMintPrice(msg.sender, _params.currency, (_amount * _params.pricePerUnit) / 1e18);
_distributeMintPrice(
msg.sender, _params.currency, (_amount * _params.pricePerUnit) / (10 ** IERC20(address(this)).decimals())
);
}

/// @notice Callback function for the ERC20Core.mint function.
/// @notice Callback function for the ERC20Core.mintWithSignature function.
function beforeMintWithSignatureERC20(address _to, uint256 _amount, bytes memory _data, address _signer)
external
payable
Expand All @@ -213,7 +222,9 @@ contract ClaimableERC20 is

_validateClaimSignatureParams(_params, _to, _amount);

_distributeMintPrice(msg.sender, _params.currency, (_amount * _params.pricePerUnit) / 1e18);
_distributeMintPrice(
msg.sender, _params.currency, (_amount * _params.pricePerUnit) / (10 ** IERC20(address(this)).decimals())
);
}

/// @dev Called by a Core into an Module during the installation of the Module.
Expand Down
Loading

0 comments on commit be6f40a

Please sign in to comment.