| title | sponsor | slug | date | findings | contest |
|---|---|---|---|---|---|
Joyn contest |
Joyn |
2022-03-joyn |
2022-07-25 |
104 |
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit contest is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.
During the audit contest outlined in this document, C4 conducted an analysis of the Joyn smart contract system written in Solidity. The audit contest took place between March 30—April 1 2022.
45 Wardens contributed reports to the Joyn contest:
- leastwood
- kirk-baird
- peritoflores
- hickuphh3
- ych18
- wuwe1
- hyh
- rayn
- WatchPug (jtp and ming)
- Dravee
- Ruhum
- hubble (ksk2345 and shri4net)
- TomFrenchBlockchain
- robee
- defsec
- CertoraInc (egjlmn1, OriDabush, ItayG, and shakedwinder)
- saian
- IllIllI
- 0x (Czar102 and pmerkleplant)
- 0xDjango
- pedroais
- minhquanym
- m9800
- rfa
- 0xkatana
- kenta
- BouSalman
- z3s
- securerodd
- cccz
- 0v3rf10w
- 0x1f8b
- hake
- Hawkeye (0xwags and 0xmint)
- Tomio
- Funen
- blackswordsman
- 0xNazgul
This contest was judged by Michael De Luca.
Final report assembled by itsmetechjay.
The C4 analysis yielded an aggregated total of 21 unique vulnerabilities. Of these vulnerabilities, 9 received a risk rating in the category of HIGH severity and 12 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 25 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 21 reports recommending gas optimizations.
All of the issues presented here are linked back to their original finding.
The code under review can be found within the C4 Joyn contest repository, and is composed of 10 smart contracts written in the Solidity programming language and includes 1,294 lines of Solidity code.
C4 assesses the severity of disclosed vulnerabilities according to a methodology based on OWASP standards.
Vulnerabilities are divided into three primary risk categories: high, medium, and low/non-critical.
High-level considerations for vulnerabilities span the following key areas when conducting assessments:
- Malicious Input Handling
- Escalation of privileges
- Arithmetic
- Gas use
Further information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website.
Submitted by hickuphh3, also found by 0xDjango, kirk-baird, leastwood, m9800, minhquanym, and pedroais
The transferFrom() function returns a boolean value indicating success. This parameter needs to be checked to see if the transfer has been successful. Oddly, transfer() function calls were checked.
Some tokens like EURS and BAT will not revert if the transfer failed but return false instead. Tokens that don't actually perform the transfer and return false are still counted as a correct transfer.
Users would be able to mint NFTs for free regardless of mint fee if tokens that don’t revert on failed transfers were used.
Check the success boolean of all transferFrom() calls. Alternatively, use OZ’s SafeERC20’s safeTransferFrom() function.
sofianeOuafir (Joyn) confirmed, disagreed with severity and commented:
In my opinion, the severity level should be 3 (High Risk) instead of 2 (Med Risk)
This is clearly an issue that needs to be fixed and represents a high risk. Currently, the current state of the code would allow users to mint tokens even if the payment isn't successful.
deluca-mike (judge) increased severity to High and commented:
payableTokenseems to be defined by whomever defines theCollectionincreateProject, so it would be possible for that person to define a payable token that, unbeknownst to them, behaves unexpectedly. I agree with high risk (unless there is some person/committee that is curates and validates thepaybaleTokens ahead of time). Need to handle return fromtransferandtransferFrom, as well as erc20s that do not return anything from fromtransferandtransferFrom.
Submitted by cccz, also found by hickuphh3, kirk-baird, leastwood, pedroais, rayn, Ruhum, saian, WatchPug, and wuwe1
In general, the Splitter contract's incrementWindow function is only called when tokens are transfer to the contract, ensuring that the number of tokens stored in balanceForWindow is equal to the contract balance. However, anyone can use a fake RoyaltyVault contract to call the incrementWindow function of the Splitter contract, so that the amount of tokens stored in balanceForWindow is greater than the contract balance, after which the verified user can call the claim or claimForAllWindows functions to steal the tokens in the contract.
function incrementWindow(uint256 royaltyAmount) public returns (bool) {
uint256 wethBalance;
require(
IRoyaltyVault(msg.sender).supportsInterface(IID_IROYALTY),
"Royalty Vault not supported"
);
require(
IRoyaltyVault(msg.sender).getSplitter() == address(this),
"Unauthorised to increment window"
);
wethBalance = IERC20(splitAsset).balanceOf(address(this));
require(wethBalance >= royaltyAmount, "Insufficient funds");
require(royaltyAmount > 0, "No additional funds for window");
balanceForWindow.push(royaltyAmount);
currentWindow += 1;
emit WindowIncremented(currentWindow, royaltyAmount);
return true;
}
Add the onlyRoyaltyVault modifier to the incrementWindow function of the Splitter contract to ensure that only RoyaltyVault contracts with a specific address can call this function.
sofianeOuafir (Joyn) confirmed and commented:
This is a high-risk issue and we intend to solve it. The mitigation provided looks good too and will be considered when fixing this issue 👍
deluca-mike (judge) commented:
See a detailed exploit and recommended solution at #21
Submitted by kirk-baird, also found by hyh and Ruhum
When the value of currentWindow is raised sufficiently high Splitter.claimForAllWindows() will not be able to be called due to the block gas limit.
currentWindow can only ever be incremented and thus will always increase. This value will naturally increase as royalties are paid into the contract.
Furthermore, an attacker can continually increment currentWindow by calling incrementWindow(). An attacker can impersonate a IRoyaltyVault and send 1 WEI worth of WETH to pass the required checks.
Excerpt from Splitter.claimForAllWindows() demonstrating the for loop over currentWindow that will grow indefinitely.
for (uint256 i = 0; i < currentWindow; i++) {
if (!isClaimed(msg.sender, i)) {
setClaimed(msg.sender, i);
amount += scaleAmountByPercentage(
balanceForWindow[i],
percentageAllocation
);
}
}
Splitter.incrementWindow() may be called by an attacker increasing currentWindow.
function incrementWindow(uint256 royaltyAmount) public returns (bool) {
uint256 wethBalance;
require(
IRoyaltyVault(msg.sender).supportsInterface(IID_IROYALTY),
"Royalty Vault not supported"
);
require(
IRoyaltyVault(msg.sender).getSplitter() == address(this),
"Unauthorised to increment window"
);
wethBalance = IERC20(splitAsset).balanceOf(address(this));
require(wethBalance >= royaltyAmount, "Insufficient funds");
require(royaltyAmount > 0, "No additional funds for window");
balanceForWindow.push(royaltyAmount);
currentWindow += 1;
emit WindowIncremented(currentWindow, royaltyAmount);
return true;
}
Consider modifying the function claimForAllWindows() to instead claim for range of windows. Pass the function a startWindow and endWindow and only iterate through windows in that range. Ensure that endWindow < currentWindow.
sofianeOuafir (Joyn) confirmed, disagreed with severity and commented:
In my opinion, the severity level should be 3 (High Risk) instead of 2 (Med Risk) duplicate of #3
deluca-mike (judge) increased severity to High and commented:
While similar, I believe these issues are separate.
Issue 3 indicates that the check that
msg.senderis an authorizedRoyaltyVaultis faulty, since any contract can implement the interface and return theSplitterfromgetSplitter. While this should be fixed, as the warden suggested in the Recommended Mitigation Steps in #3, the issue raised in this issue can still occur when enough authorizedRoyaltyVaultcontracts callincrementWindow.
claimForAllWindowscan remain, but as this warden suggests, aclaimForWindows(uint256 startWindow, uint256 endWindow, uint256 percentageAllocation, bytes32[] calldata merkleProof)should exist, in caseclaimForAllWindowsbecomes prohibitively expensive, even organically (i.e.currentWindowis made very high due to sufficient authorizedincrementWindowcalls).
Submitted by hyh, also found by 0xkatana, hubble, kirk-baird, leastwood, pedroais, rayn, rfa, Ruhum, saian, securerodd, and WatchPug
Reinitialization is possible for CoreCollection as initialize function sets initialized flag, but doesn't control for it, so the function can be rerun multiple times.
Such types of issues tend to be critical as all core variables can be reset this way, for example payableToken, which provides a way to retrieve all the contract funds.
However, setting priority to be medium as initialize is onlyOwner. A run by an external attacker this way is prohibited, but the possibility of owner initiated reset either by mistake or with a malicious intent remains with the same range of system breaking consequences.
initialize doesn't control for repetitive runs:
Add onlyUnInitialized modifier to the initialize function:
sofianeOuafir (Joyn) confirmed, disagreed with severity and commented:
This is a high severity issue and we intend to fix it. The mitigation step looks great and will be considered to fix the issue.
In my opinion, the severity level should be 3 (High Risk) instead of 2 (Med Risk)
deluca-mike (judge) increased severity to High and commented:
Not convinced this is a high severity issue, since erroneously changingpayableTokenvia a re-initialization can simply be corrected by a re-re-initialization to set it back correctly. Further, as the warden mentioned, the initialize function is behindonlyOwner.
However, if it can be shown that users other than the owner can end up losing value due to the owner abusing or erroneously usinginitialize, then it can be promoted to High Severity.And just as I say that, #17 points that out clearly. So, yes, agreed, this is a High Severity issue.
Submitted by kirk-baird, also found by 0xDjango, defsec, Dravee, hubble, hyh, leastwood, minhquanym, Ruhum, TomFrenchBlockchain, and WatchPug
The owner of RoyaltyVault can set _platformFee to any arbitrary value (e.g. 100% = 10000) and that share of the contracts balance and future balances will be set to the platformFeeRecipient (which is in the owners control) rather than the splitter contract.
As a result the owner can steal the entire contract balance and any future balances avoiding the splitter.
function setPlatformFee(uint256 _platformFee) external override onlyOwner {
platformFee = _platformFee;
emit NewRoyaltyVaultPlatformFee(_platformFee);
}
This issue may be mitigated by add a maximum value for the _platformFee say 5% (or some reasonable value based on the needs of the platform).
Also consider calling sendToSplitter() before adjusting the platformFee. This will only allow the owner to change the fee for future value excluding the current contract balance.
Consider the following code.
function setPlatformFee(uint256 _platformFee) external override onlyOwner {
require(_platformFee < MAX_FEE);
sendToSplitter(); // @audit this will need to be public rather than external
platformFee = _platformFee;
emit NewRoyaltyVaultPlatformFee(_platformFee);
}
sofianeOuafir (Joyn) confirmed and commented:
This is an issue and we intend to fix it. The recommended mitigation looks good and will be considered.
We also agree that this is a med risk as this can currently only be done by the contract owner which is us at Joyn
deluca-mike (judge) commented:
Instead of having to come up with a "reasonable"
MAX_FEE, consider instead just preventing the fee from ever being raised, and only allowing it to be lowered.
deluca-mike (judge) increased severity to High and commented:
While I was originally leaning Medium Risk, after taking the arguments made by the duplicate issues into account, I am now leaning High Risk. The rationale is that, a DOS of
sendToSplittervia a highplatformFeenot only harms stakeholders of theRoyaltyVaultthat would get the remainder of the balance, split, but may also prevent all NFT transfers ifsendToSplitteris hooked into as part of all token transfer, via royalty payments. A malicious or disgruntledRoyaltyVaultowner can hold all the NFTs hostage that callsendToSplitteratomically on transfers.So there are 2 issues that need to be solved here:
- protect NFT holders by ensuring
platformFee(or any other values) cannot be set to a value that would causesendToSplitterto fail (splitterShare = 0orplatformShare > balanceOfVault), or don't havesendToSplitterbe called on NFT transfers- protect royalty split recipients by putting an arbitrary max to the fee, or only allowing the fee to be reduced
Submitted by peritoflores
Storage collision because of lack of EIP1967 could cause conflicts and override sensible variables
contract CoreProxy is Ownable {
address private immutable _implement;
When you implement proxies, logic and implementation share the same storage layout. In order to avoid storage conflicts EIP1967 was proposed.(https://eips.ethereum.org/EIPS/eip-1967) The idea is to set proxy variables at fixed positions (like impl and admin ).
For example, according to the standard, the slot for for logic address should be
0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc (obtained as bytes32(uint256(keccak256('eip1967.proxy.implementation')) - 1) ).
In this case, for example, as you inherits from Ownable the variable _owner is at the first slot and can be overwritten in the implementation. There is a table at OZ site that explains this scenario more in detail
https://docs.openzeppelin.com/upgrades-plugins/1.x/proxies
section "Unstructured Storaged Proxies"
Consider using EIP1967
sofianeOuafir (Joyn) confirmed and commented:
This is an issue we want to investigate and fix if our investigation suggests we indeed need to make improvement on that end.
At the same time, I have little idea of what is the impact of this issue. I'm not sure if it's a high risk item
deluca-mike (judge) commented:
Impact would be that an upgrade could brick a contract by simply rearranging inheritance order, or adding variables to an inherited contract, since the implantation slot will not be where it is expected. As the warden suggests, its critical that the implementation slot be fixed at an explicit location, and not an implicit location derived purely from inheritance and declaration order.
Submitted by leastwood
The mintToken() function is called to mint unique tokens from an ERC721 collection. This function will either require users to provide a merkle proof to claim an airdropped token or pay a fee in the form of a payableToken. However, because the payableToken is paid before a token is minted, it may be possible to reenter the mintToken() function if there is a callback attached before or after the token transfer. Because totalSupply() has not been updated for the new token, a user is able to bypass the totalSupply() + amount <= maxSupply check. As a result, if the user mints the last token, they can reenter and mint duplicate NFTs as the way tokenId is generated will wrap around to the start again.
For the sake of this example, let's say startingIndex = 0 and maxSupply = 100. tokenId is minted according to ((startingIndex + totalSupply()) % maxSupply) + 1. If we see that a user mints a token where totalSupply() = maxSupply - 1 = 99 and they reenter the function, then the next token to mint will actually be of index 1 as totalSupply() % maxSupply = 0. Calculating the first tokenId, we get ((0 + 0) % maxSupply) + 1 = 1 which is a duplicate of our example.
Consider adding reentrancy protections to prevent users from abusing this behaviour. It may also be useful to follow the checks-effects pattern such that all external/state changing calls are made at the end.
sofianeOuafir (Joyn) confirmed and commented:
This is an issue we intend to investigate and fix if indeed it is an issue
deluca-mike (judge) commented:
This is a valid high risk issue. Also, for reference, the checks-effects-interactions (CEI) pattern suggests you, in this order:
- perform checks that something can be done
- perform the effects (update storage and emit events)
- interact with other functions/contracts (since you may not be sure they will call out and re-enter)
Submitted by ych18, also found by hickuphh3 and WatchPug
The CoreCollection.withdraw function uses payableToken.transferFrom(address(this), msg.sender, amount) to transfer tokens from the CoreCollection contract to the msg.sender ( who is the owner of the contract). The usage of transferFrom can result in serious issues. In fact, many ERC20 always require that in transferFrom allowance[from][msg.sender] >= amount, so in this case the call to the withdraw function will revert as the allowance[CoreCollection][CoreCollection] == 0 and therefore the funds cannot ben withdrawn and will be locked forever in the contract.
Replace transferFrom with transfer
sofianeOuafir (Joyn) confirmed and commented:
duplicate of #52
deluca-mike (judge) commented:
This is not a duplicate, as it pertains to the wrong use of transfer vs transferFrom, which can have implications regarding required allowances.
Submitted by ych18, also found by wuwe1
Although the ERC20 standard suggests that a transfer should return true on success, many tokens are non-compliant in this regard (including high profile, like USDT) . In that case, the .transfer() call here will revert even if the transfer is successful, because solidity will check that the RETURNDATASIZE matches the ERC20 interface.
Consider using OpenZeppelin’s SafeERC20
sofianeOuafir (Joyn) confirmed and commented:
duplicate of #52
deluca-mike (judge) commented:
Actually not a duplicate of #52, since it pertains to return data size handling causing an issue, rather than failure to handle a true/false return at all. Still, same solution (use SafeERC20).
[M-01] DoS: Attacker May Front-Run createSplit() With A merkleRoot Causing Future Transactions With The Same merkleRoot to Revert
Submitted by kirk-baird
A merkleRoot may only be used once in createSplit() since it is used as salt to the deployment of a SplitProxy.
The result is an attacker may front-run any createSplit() transaction in the mem pool and create another createSplit() transaction with a higher gas price that uses the same merkleRoot but changes the other fields such as the _collectionContract or _splitAsset(). The original transaction will revert and the user will not be able to send any more transaction with this merkleRoot.
The user would therefore have to generate a new merkle tree with different address, different allocations or a different order of leaves in the tree to create a new merkle root. However, the attack is repeateable and there is no guarantee this new merkle root will be successfully added to a split without the attacker front-running the transaction again.
The excerpt from createSplitProxy() shows the merkleRoot() being used as a salt.
splitProxy = address(
new SplitProxy{salt: keccak256(abi.encode(merkleRoot))}()
);
As seems to be the case here if the transaction address does NOT need to be known ahead of time consider removing the salt parameter from the contract deployment.
Otherwise, if the transaction address does need to be known ahead of time then consider concatenating msg.sender to the merkleRoot. e.g.
splitProxy = address(
new SplitProxy{salt: keccak256(abi.encode(msg.sender, merkleRoot))}()
)
sofianeOuafir (Joyn) confirmed and commented:
This is an issue and intend to fix it
Submitted by kirk-baird
The function attemptETHTransfer() makes a call with a fixed amount of gas, 30,000. If the receiver is a contract this may be insufficient to process the receive() function. As a result the user would be unable to receive funds from this function.
function attemptETHTransfer(address to, uint256 value)
private
returns (bool)
{
// Here increase the gas limit a reasonable amount above the default, and try
// to send ETH to the recipient.
// NOTE: This might allow the recipient to attempt a limited reentrancy attack.
(bool success, ) = to.call{value: value, gas: 30000}("");
return success;
}
Consider removing the gas field to use the default amount and protect from reentrancy by using reentrancy guards and the check-effects-interaction pattern. Note this pattern is already applied correctly.
sofianeOuafir (Joyn) confirmed and commented:
This is an issue we want to investigate and fix if needed
deluca-mike (judge) commented:
Consider that changes to the network could result in re-pricing of opcodes which can make:
- opcodes less expensive, which would re-introduce the opportunity for re-entrancy that the old costs prevented
- opcodes more expensive, which would break this function altogether
If
attemptETHTransferis the only way to extract ETH stored in this contract, then an Ethereum upgrade could result in lost funds. Luckily, as long as access lists remain in the protocol, a route would exist to recover and/or restore functionality.
Submitted by leastwood
The Joyn documentation mentions that Joyn royalty vaults should be equipped to handle revenue generated on a collection's primary and secondary sales. Currently, CoreCollection.sol allows the collection owner to receive a fee on each token mint, however, there is no existing implementation which allows the owner of a collection to receive fees on secondary sales.
After discussion with the Joyn team, it appears that this will be gathered from Opensea which does not have an on-chain royalty mechanism. As such, each collection will need to be added manually on Opensea, introducing further centralisation risk. It is also possible for users to avoid paying the secondary fee by using other marketplaces such as Foundation.
Consider implementing the necessary functionality to allow for the collection of fees through an on-chain mechanism. ERC2981 outlines the appropriate behaviour for this.
sofianeOuafir (Joyn) confirmed and commented:
This is a great observation. Something we are aware of and intend to fix as well. 👍
Submitted by wuwe1, also found by defsec, and kirk-baird
This is dangerous in scam senario because the malicious user can frontrun and become the owner of the collection. As owner, one can withdraw paymentToken. (note that _collections.isForSale can be change by frontrunner)
- Anyone can call
createProject.
function createProject(
string memory _projectId,
Collection[] memory _collections
) external onlyAvailableProject(_projectId) {
require(
_collections.length > 0,
'CoreFactory: should have more at least one collection'
);Two ways to mitigate.
- Consider use white list on project creation.
- Ask user to sign their address and check the signature against
msg.sender. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L102
sofianeOuafir (Joyn) confirmed and commented:
This is an issue and we intend to fix it!
deluca-mike (judge) commented:
The solutions listed in #34 and #35 are better.
[M-05] Gas costs will likely result in any fees sent to the Splitter being economically unviable to recover.
Submitted by TomFrenchBlockchain
Collection owners will likely lose money by claiming fees unless the fees from a single NFT sale outweighs the cost of claiming it (not guaranteed).
Consider a new Collection with a RoyaltyVault and Splitter set and a nonzero mint fee.
When calling mintToken, the _handlePayment function is called
https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/core-contracts/contracts/CoreCollection.sol#L161-L163
This will transfer the minting fee to the RoyaltyVault contract.
On each transfer of an NFT within the collection (for instance in the _mint call which occurs directly after calling _handlePayment), the Collection contract will call sendToSplitter on the RoyaltyVault:
This function will forward the collection owners' portion of the minting on to the Splitter contract but another important thing to note is that we call Splitter.incrementWindow.
This results in the fees newly deposited into the Splitter contract being held in a separate "window" to the fees from previous or later mints and need to be claimed separately. Remember that this process happens on every NFT sale so the only funds which will be held in this window will be the minting fees for this particular mint.
From this we can see that the claim function will only claim the fraction of the fees which are owed to the caller from a single NFT mint.
Note that we can attempt to claim from multiple windows in a single transaction using claimForAllWindow but as the name suggests it performs an unbounded loop trying to claim all previous windows (even ones which have already been claimed!) and it is likely that with a new window for every NFT sold this function will exceed the gas limit (consider an 10k token collection resulting in trying to do 10k SSTOREs at 20k gas each.), leaving us to claim each window individually with claim.
We're then forced to claim the royalties from each NFT sold one by one, having to send huge numbers of calls to claim incurring the base transaction cost many times over and performing many ERC20 transfers when we could have just performed one.
Compound on this that this needs to be repeated by everyone included in the split, multiplying the costs of claiming.
Medium risk as it's gas inefficiency to the point of significant value leakage where collection owners will lose a large fraction of their royalties.
It doesn't seem like the "window" mechanism does anything except raise gas costs to the extent that it will be very difficult to withdraw fees so it should be removed.
sofianeOuafir (Joyn) confirmed and commented:
This is a very fair point and we'll consider fixing this issue.
deluca-mike (judge) commented:
Aside from the very valid points made by the warden, it seems that the heavy functions called from the
_beforeTokenTransferalso create a lot of friction for the NFT owners. Might make more sense to have royalty splitting happen asynchronously from NFT transfers (i.e. let the cost of splitting be the burden of royalty stakeholders, not NFT holders).If NFT transferring becomes too costly, someone could make a "de-joyn" contract which can "re-tokenize" NFTs sent to it, so that they can be transferred without having to worry about
_beforeTokenTransfer(or royalties, for that matter).See Recommended Mitigation Steps in #37 for more info.
Submitted by hyh, also found by robee
https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L164
When royaltyAsset is an ERC20 that doesn't allow zero amount transfers, the following griefing attack is possible, entirely disabling CoreCollection token transfer by precision degradation as both reward distribution and vault balance can be manipulated.
Suppose splitterProxy is set, all addresses and fees are configured correctly, system is in normal operating state.
POC:
Bob the attacker setup a bot which every time it observes positive royaltyVault balance:
-
runs
sendToSplitter(), distributing the whole current royaltyAsset balance of the vault to splitter and platform, so vault balance becomes zero -
sends
1 weiof royaltyAsset to the royaltyVault balance -
each next CoreCollection token transfer will calculate
platformShare = (balanceOfVault * platformFee) / 10000, which will be 0 as platformFee is supposed to be less than 100%, and then there will be an attempt to transfer it toplatformFeeRecipient
If royaltyAsset reverts on zero amount transfers, the whole operation will fail as the success of IERC20(royaltyAsset).transfer(platformFeeRecipient, platformShare) is required for each CoreCollection token transfer, which invokes sendToSplitter() in _beforeTokenTransfer() as vault balance is positive in (3).
Notice, that Bob needn't to front run the transfer, it is enough to empty the balance in a lazy way, so cumulative gas cost of the attack can be kept moderate.
Setting severity to medium as on one hand, the attack is easy to setup and completely blocks token transfers, making the system inoperable, and it looks like system has to be redeployed on such type of attack with some manual management of user funds, which means additional operational costs and reputational damage. On the another, it is limited to the zero amount reverting royaltyAsset case or the case when platformFee is set to 100%.
That is, as an another corner case, if platformFee is set to 100%, platformShare will be 1 wei and splitterShare be zero in (3), so this attack be valid for any royaltyAsset as it is required in Splitter's incrementWindow that splitterShare be positive.
As royaltyAsset can be an arbitrary ERC20 it can be reverting on zero value transfers:
https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers
_beforeTokenTransfer runs IRoyaltyVault(royaltyVault).sendToSplitter() whenever royaltyVault is set and have positive balance:
sendToSplitter() leaves vault balance as exactly zero as splitterShare = balanceOfVault - platformShare, i.e. no dust is left behind:
This way the balance opens up for the tiny amount manipulation.
One require that can fail the whole operation is platformShare transfer:
Another is positive royaltyAmount = splitterShare requirement:
https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L164
The issue is that token transfer, which is core system operation, require fee splitting to be done on the spot. More failsafe design is to try to send the fees and record the amounts not yet distributed, not requiring immediate success. The logic here is that transfer itself is more important than fee distribution, which is simple enough and can be performed in a variety of ways later.
Another issue is a combination of direct balance usage and the lack of access controls of the sendToSplitter function, but it only affects fee splitting and is somewhat harder to address.
As one approach consider trying, but not requiring IRoyaltyVault(royaltyVault).sendToSplitter() to run successfully as it can be executed later with the same result.
Another, a simpler one (the same is in Griefing attack is possible making Splitter's claimForAllWindows inaccessible issue), is to introduce action threshold, MIN_ROYALTY_AMOUNT, to sendToSplitter(), for example:
Now:
/**
* @dev Send accumulated royalty to splitter.
*/
function sendToSplitter() external override {
uint256 balanceOfVault = getVaultBalance();
require(
balanceOfVault > 0,
"Vault does not have enough royalty Asset to send"
);
...
emit RoyaltySentToSplitter(...);
emit FeeSentToPlatform(...);
}
To be:
/**
* @dev Send accumulated royalty to splitter if it's above MIN_ROYALTY_AMOUNT threshold.
*/
function sendToSplitter() external override {
uint256 balanceOfVault = getVaultBalance();
if (balanceOfVault > MIN_ROYALTY_AMOUNT) {
...
emit RoyaltySentToSplitter(...);
emit FeeSentToPlatform(...);
}
}
sofianeOuafir (Joyn) confirmed
Submitted by kirk-baird, also found by defsec, hickuphh3, and leastwood
Certain ERC20 tokens may change user's balances over time (positively or negatively) or charge a fee when a transfer is called (FoT tokens). The accounting of these tokens is not handled by RoyaltyVault.sol or Splitter.sol and may result in tokens being stuck in Splitter or overstating the balance of a user
Thus, for FoT tokens if all users tried to claim from the Splitter there would be insufficient funds and the last user could not withdraw their tokens.
The function RoyaltyVault.sendToSplitter() will transfer splitterShare tokens to the Splitter and then call incrementWindow(splitterShare) which tells the contract to split splitterShare between each of the users.
require(
IERC20(royaltyAsset).transfer(splitterProxy, splitterShare) == true,
"Failed to transfer royalty Asset to splitter"
);
require(
ISplitter(splitterProxy).incrementWindow(splitterShare) == true,
"Failed to increment splitter window"
);
Since the Splitter may receive less than splitterShare tokens if there is a fee on transfer the Splitter will overstate the amount split and each user can claim more than their value (except the last user who claims nothing as the contract will have insufficient funds to transfer them the full amount).
Furthermore, if the token rebase their value of the tokens down while they are sitting in the Splitter the same issue will occur. If the tokens rebase their value up then this will not be accounted for in the protocol.
It is recommend documenting clearly that rebasing token should not be used in the protocol.
Alternatively, if it is a requirement to handle rebasing tokens balance checks should be done before and after the transfer to ensure accurate accounting. Note: this makes the contract vulnerable to reentrancy and so a reentrancy guard must be placed over the function sendToSplitter().
uint256 balanceBefore = IERC20(royaltyAsset).balanceOf(splitterProxy);
require(
IERC20(royaltyAsset).transfer(splitterProxy, splitterShare) == true,
"Failed to transfer royalty Asset to splitter"
);
uint256 balanceAfter = IERC20(royaltyAsset).balanceOf(splitterProxy);
require(
ISplitter(splitterProxy).incrementWindow(balanceAfter - balanceBefore) == true,
"Failed to increment splitter window"
);
sofianeOuafir (Joyn) confirmed
[M-08] CoreCollection: Starting index is pseudo-randomly generated, allowing for gameable NFT launches
Submitted by hickuphh3
In Paradigm’s article “A Guide to Designing Effective NFT Launches”, one of the desirable properties of an NFT launch is unexploitable fairness: Launches must have true randomness to ensure that predatory users cannot snipe the rarest items at the expense of less sophisticated users.
It is therefore highly recommended to find a good source of entropy for the generation of the starting index. The block.number isn’t random at all; it only incrementally increases, allowing anyone to easily compute the starting indexes of the next 10,000 blocks for instance.
contract FortuneTeller {
function predictStartingIndexes(uint256 maxSupply, uint256 numBlocks)
external
view
returns
(uint256[] memory startingIndexes) {
startingIndexes = new uint[](numBlocks);
for (uint256 i = 0; i < numBlocks; ++i) {
startingIndexes[i] = (uint256(
keccak256(abi.encodePacked("CoreCollection", block.number + i))
) % maxSupply) +
1;
}
}
}Coupled with the fact that the _baseUri is set upon initialization, the metadata could be scrapped beforehand to determine the rare NFTs.
Thus, NFT mints can be gamed / exploited.
Consider exploring the use of commit-reveal schemes (eg. blockhash of a future block, less gameable but not foolproof) or VRFs.
sofianeOuafir (Joyn) acknowledged and commented:
This is a known issue and for now, we're not going to solve it
deluca-mike (judge) commented:
I'd reconsider not completely it, but rather doing
keccak256(abi.encodePacked("CoreCollection", blockhash(block.number - 1), block.coinbase, msg.sender, i))so at least there is a much smaller set of users that will know what the previous blockhash and the current miner will be by the time the tx mines, and it will be salted with the sender.It's not perfect, but it's significantly better.
Submitted by hickuphh3, also found by 0x
https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L14
https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L103
There is a PERCENTAGE_SCALE = 10e5 defined, but the actual denominator used is 10000. This is aggravated by the following factors:
- Split contracts are created by collection owners, not the factory owner. Hence, there is a likelihood for someone to mistakenly use
PERCENTAGE_SCALEinstead of10000. - The merkle root for split distribution can only be set once, and a collection’s split and royalty vault can’t be changed once created.
Thus, if an incorrect denominator is used, the calculated claimable amount could exceed the actual available funds in the contract, causing claims to fail and funds to be permanently locked.
Remove PERCENTAGE_SCALE because it is unused, or replace its value with 10_000 and use that instead.
P.S: there is an issue with the example scaled percentage given for platform fees (5% = 200). Should be 500 instead of 200.
sofianeOuafir (Joyn) confirmed and commented:
This is an issue and we intend to fix it
Submitted by Dravee
It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate.
Here, no timelock capabilities seem to be used
I believe this impacts multiple users enough to make them want to react / be notified ahead of time.
Consider adding a timelock to setPlatformFee()
sofianeOuafir (Joyn) acknowledged and commented:
This is a good idea. We will consider mitigating this but at the same time it might not be something we will solve
Submitted by hubble
The below transferFrom command is called at two places in the core contracts, followed by an emit event
payableToken.transferFrom(msg.sender,recipient,_amount)
emit ...(...);
The return value is not checked during the payableToken.transferFrom
In the event of failure of payableToken.transferFrom(...), the emit event is still generated causing the downstream applications to capture wrong transaction / state of the protocol.
-
Contract CoreCollection.sol
function withdraw() -
Contract ERC721Payable.sol function _handlePayment
Add a require statement as being used in the RoyaltyVault.sol
require( payableToken.transferFrom(msg.sender,recipient,_amount) == true,
"Failed to transfer amount to recipient" );
sofianeOuafir (Joyn) confirmed, disagreed with severity and commented:
In my opinion, the severity level should be 3 (High Risk) instead of 2 (Med Risk) duplicate of #52
deluca-mike (judge) commented:
No longer a duplicate because this issue pertains specifically to the false emission of events when an underlying call would have failed.
[M-12] CoreCollection.setRoyaltyVault doesn't check royaltyVault.royaltyAsset against payableToken, resulting in potential permanent lock of payableTokens in royaltyVault
Submitted by rayn
Each CoreProxy is allowed to be associated with a RoyaltyVault, the latter which would be responsible for collecting minting fees and distributing to beneficiaries. Potential mismatch between token used in CoreProxy and RoyaltyVault might result in minting tokens being permanently stuck in RoyaltyVault.
Each RoyaltyVault can only handle the royaltyVault.royaltyAsset token assigned upon creation, if any other kind of tokens are sent to the vault, it would get stuck inside the vault forever.
function sendToSplitter() external override {
...
require(
IERC20(royaltyAsset).transfer(splitterProxy, splitterShare) == true,
"Failed to transfer royalty Asset to splitter"
);
...
require(
IERC20(royaltyAsset).transfer(
platformFeeRecipient,
platformShare
) == true,
"Failed to transfer royalty Asset to platform fee recipient"
);
...
}
Considering that pairing of CoreProxy and RoyaltyVault is not necessarily handled automatically, and can sometimes be manually assigned, and further combined with the fact that once assigned, CoreProxy does not allow modifications of the pairing RoyaltyVault. We can easily conclude that if a CoreProxy is paired with an incompatible RoyaltyVault, the payableToken minting fees automatically transferred to RoyaltyVault by _handlePayment will get permanently stuck.
function setRoyaltyVault(address _royaltyVault)
external
onlyVaultUninitialized
{
...
royaltyVault = _royaltyVault;
...
}
function _handlePayment(uint256 _amount) internal {
address recipient = royaltyVaultInitialized()
? royaltyVault
: address(this);
payableToken.transferFrom(msg.sender, recipient, _amount);
...
}
vim, ganache-cli
While assigning vaults to CoreProxy, check if payableToken is the same as royaltyVault.royaltyAsset
function setRoyaltyVault(address _royaltyVault)
external
onlyVaultUninitialized
{
require(
payableToken == _royaltyVault.royaltyAsset(),
"CoreCollection : payableToken must be same as royaltyAsset."
);
...
royaltyVault = _royaltyVault;
...
}
sofianeOuafir (Joyn) confirmed
deluca-mike (judge) decreased severity to Medium and commented:
Downgraded to medium because, while a more automated and validated way of assigning a compatible royalty vault would prevent this issue, in the current framework you'd need to make a user error (albeit one that is not easy to spot), to lose funds.
For this contest, 25 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by CertoraInc received the top score from the judge.
The following wardens also submitted reports: robee, rayn, defsec, hyh, BouSalman, IllIllI, 0xDjango, kenta, hubble, Dravee, z3s, saian, Hawkeye, kirk-baird, 0v3rf10w, wuwe1, hake, 0x, Ruhum, 0x1f8b, leastwood, ych18, WatchPug, and 0xkatana.
-
Different pragma versions - the core-contracts use
pragma solidity ^0.8.0and the rest of the contracts usepragma solidity ^0.8.4 -
Use a specific solidity version instead of using
^, to prevent future solidity versions impacting your code and creating issues. -
In the comments and variable names you wrote ETH instead of wETH, which is un-correct (that's an ERC20 so it must be wETH)
sol function transferSplitAsset(address to, uint256 value) private returns (bool didSucceed) { // Try to transfer ETH to the given recipient. didSucceed = IERC20(splitAsset).transfer(to, value); require(didSucceed, "Failed to transfer ETH"); emit TransferETH(to, value, didSucceed); } -
In the comment before the function, you wrote returns instead of the known
@returntagsol /** * @notice Mint token * @dev A starting index is calculated at the time of first mint * returns a tokenId * @param _to Token recipient */ function mint(address _to) private returns (uint256 tokenId) { if (startingIndex == 0) { setStartingIndex(); } tokenId = ((startingIndex + totalSupply()) % maxSupply) + 1; _mint(_to, tokenId); } -
Low level calls (call, delegate call and static call) return success if the called contract doesn’t exist (not deployed or destructed). This can be seen here https://github.com/Uniswap/v3-core/blob/main/audits/tob/audit.pdf (report #9) and here https://docs.soliditylang.org/en/develop/control-structures.html#error-handling-assert-require-revert-and-exceptions.
That means that in `attemptETHTransfer`, if `to` doesn't exist the call will fail but success will be set to true, which will act like the call was successful. ```sol function attemptETHTransfer(address to, uint256 value) private returns (bool) { // Here increase the gas limit a reasonable amount above the default, and try // to send ETH to the recipient. // NOTE: This might allow the recipient to attempt a limited reentrancy attack. (bool success, ) = to.call{value: value, gas: 30000}(""); return success; } ``` -
Add
onlyUnInitializedmodifier to theinitializefunction, otherwise the owner can initialize the contract more than one time -
HASHED_PROOF- upper case variable name that is not constant -
If
startingIndex + totalSupply()will reachtype(uint256).maxthe system will be in a stuck state, that's because the calculation in the _mint function will overflow -
Contracts not declaring that they implement their interfaces - for example
CoreCollectionandCoreFactorydon't declare that they implementICoreCollectionandICoreFactory -
ICoreFactoryis imported but not used inCoreProxy -
Didn't check that the address of the given vault is not zero in the
setPlatformFeefunction -
Wrong comment in
RoyaltyVaultFactoryandSplitFactory```sol /** * @dev Set Platform fee for collection contract. * @param _platformFee Platform fee in scaled percentage. (5% = 200) * @param _vault vault address. */ function setPlatformFee(address _vault, uint256 _platformFee) external { IRoyaltyVault(_vault).setPlatformFee(_platformFee); }/** * @dev Set Platform fee recipient for collection contract. * @param _vault vault address. * @param _platformFeeRecipient Platform fee recipient. */ function setPlatformFeeRecipient( address _vault, address _platformFeeRecipient ) external { require(_vault != address(0), "Invalid vault"); require( _platformFeeRecipient != address(0), "Invalid platform fee recipient" ); IRoyaltyVault(_vault).setPlatformFeeRecipient(_platformFeeRecipient); } ```
sofianeOuafir (Joyn) commented:
high quality report
deluca-mike (judge) commented:
add onlyUnInitialized modifier to the initialize function, otherwise the owner can initialize the contract more than one time
For this contest, 21 reports were submitted by wardens detailing gas optimizations. The report highlighted below by IllIllI received the top score from the judge.
The following wardens also submitted reports: saian, rfa, Dravee, robee, 0xkatana, kenta, WatchPug, defsec, Tomio, z3s, Funen, 0x1f8b, 0v3rf10w, minhquanym, hake, ych18, securerodd, rayn, blackswordsman, and 0xNazgul.
- File: core-contracts/contracts/ERC721Payable.sol (lines 21-24)
require(
!royaltyVaultInitialized(),
'CoreCollection: Royalty Vault already initialized'
);- File: core-contracts/contracts/ERC721Payable.sol (lines 29-32)
require(
royaltyVaultInitialized(),
'CoreCollection: Royalty Vault not initialized'
);- File: core-contracts/contracts/ERC721Claimable.sol (line 23)
require(!claimableSet(), 'ERC721Claimable: Claimable is already set');- File: core-contracts/contracts/CoreCollection.sol (line 47)
require(!initialized, "CoreCollection: Already initialized");- File: core-contracts/contracts/CoreCollection.sol (lines 52-55)
require(
_maxSupply > 0,
"CoreCollection: Max supply should be greater than 0"
);- File: core-contracts/contracts/CoreCollection.sol (line 146)
require(amount > 0, "CoreCollection: Amount should be greater than 0");- File: core-contracts/contracts/CoreCollection.sol (lines 189-192)
require(
msg.sender == splitFactory || msg.sender == owner(),
"CoreCollection: Only Split Factory or owner can initialize vault."
);- File: core-contracts/contracts/CoreCollection.sol (lines 204-207)
require(
bytes(HASHED_PROOF).length == 0,
"CoreCollection: Hashed Proof is set"
);- File: core-contracts/contracts/CoreCollection.sol (lines 220-223)
require(
startingIndex == 0,
"CoreCollection: Starting index is already set"
);- File: core-contracts/contracts/CoreFactory.sol (lines 35-38)
require(
projects[_projectId].creator == address(0),
'CoreFactory: Unavailable project id'
);- File: core-contracts/contracts/CoreFactory.sol (lines 43-46)
require(
projects[_projectId].creator == msg.sender,
'CoreFactory: Not an owner of the project'
);- File: core-contracts/contracts/CoreFactory.sol (lines 51-54)
require(
collections[_collectionId] == address(0),
'CoreFactory: Unavailable collection id'
);- File: core-contracts/contracts/CoreFactory.sol (lines 74-77)
require(
_collections.length > 0,
'CoreFactory: should have more at least one collection'
);- File: royalty-vault/contracts/RoyaltyVault.sol (lines 34-37)
require(
balanceOfVault > 0,
"Vault does not have enough royalty Asset to send"
);- File: royalty-vault/contracts/RoyaltyVault.sol (lines 43-46)
require(
IERC20(royaltyAsset).transfer(splitterProxy, splitterShare) == true,
"Failed to transfer royalty Asset to splitter"
);- File: royalty-vault/contracts/RoyaltyVault.sol (lines 47-50)
require(
ISplitter(splitterProxy).incrementWindow(splitterShare) == true,
"Failed to increment splitter window"
);- File: royalty-vault/contracts/RoyaltyVault.sol (lines 51-57)
require(
IERC20(royaltyAsset).transfer(
platformFeeRecipient,
platformShare
) == true,
"Failed to transfer royalty Asset to platform fee recipient"
);- File: splits/contracts/SplitFactory.sol (lines 48-51)
require(
splits[_splitId] == address(0),
'SplitFactory : Split ID already in use'
);- File: splits/contracts/SplitFactory.sol (lines 81-84)
require(
ICoreCollection(_collectionContract).owner() == msg.sender,
'Transaction sender is not collection owner'
);- File: splits/contracts/Splitter.sol (lines 118-121)
require(
!isClaimed(msg.sender, window),
"NFT has already claimed the given window"
);Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
- File: royalty-vault/contracts/RoyaltyVault.sol (line 2)
pragma solidity ^0.8.4;- File: royalty-vault/contracts/ProxyVault.sol (line 2)
pragma solidity ^0.8.4;- File: splits/contracts/SplitFactory.sol (line 2)
pragma solidity ^0.8.4;- File: splits/contracts/SplitProxy.sol (line 2)
pragma solidity ^0.8.4;- File: splits/contracts/Splitter.sol (line 2)
pragma solidity ^0.8.4;Use a solidity version of at least 0.8.2 to get compiler automatic inlining
Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads
Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings
Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
- File: core-contracts/contracts/ERC721Payable.sol (line 2)
pragma solidity ^0.8.0;- File: core-contracts/contracts/CoreProxy.sol (line 2)
pragma solidity ^0.8.0;- File: core-contracts/contracts/ERC721Claimable.sol (line 2)
pragma solidity ^0.8.0;- File: core-contracts/contracts/CoreCollection.sol (line 2)
pragma solidity ^0.8.0;- File: core-contracts/contracts/CoreFactory.sol (line 2)
pragma solidity ^0.8.0; // Booleans are more expensive than uint256 or any type that takes up a full
// word because each write operation emits an extra SLOAD to first read the
// slot's contents, replace the bits taken up by the boolean, and then write
// back. This is the compiler's defense against contract upgrades and
// pointer aliasing, and it cannot be disabled.- File: core-contracts/contracts/ERC721Payable.sol (line 8)
bool public isForSale;- File: core-contracts/contracts/CoreCollection.sol (line 20)
bool public initialized;- File: core-contracts/contracts/CoreCollection.sol (lines 52-55)
require(
_maxSupply > 0,
"CoreCollection: Max supply should be greater than 0"
);- File: core-contracts/contracts/CoreCollection.sol (line 146)
require(amount > 0, "CoreCollection: Amount should be greater than 0");- File: royalty-vault/contracts/RoyaltyVault.sol (lines 34-37)
require(
balanceOfVault > 0,
"Vault does not have enough royalty Asset to send"
);- File: splits/contracts/Splitter.sol (line 164)
require(royaltyAmount > 0, "No additional funds for window");Even memory arrays incur the overhead of bit tests and bit shifts to calculate the array length
- File: core-contracts/contracts/CoreFactory.sol (line 79)
for (uint256 i; i < _collections.length; i++) {- File: splits/contracts/Splitter.sol (line 274)
for (uint256 i = 0; i < proof.length; i++) {- File: core-contracts/contracts/CoreCollection.sol (line 279)
for (uint256 i = 0; i < _amount; i++) {- File: splits/contracts/Splitter.sol (line 49)
uint256 amount = 0;- File: splits/contracts/Splitter.sol (line 50)
for (uint256 i = 0; i < currentWindow; i++) {- File: splits/contracts/Splitter.sol (line 274)
for (uint256 i = 0; i < proof.length; i++) {The instances below point to the second access of a state variable within a function. Less obvious optimizations include having local storage variables of mappings within state variable mappings or mappings within state variable structs, having local storage variables of structs within mappings, or having local caches of state variable contracts/addresses.
- File: core-contracts/contracts/CoreCollection.sol (line 231)
emit StartingIndexSet(startingIndex);- File: core-contracts/contracts/CoreCollection.sol (line 264)
tokenId = ((startingIndex + totalSupply()) % maxSupply) + 1;- File: splits/contracts/SplitFactory.sol (line 161)
delete merkleRoot;- File: splits/contracts/SplitFactory.sol (line 171)
delete splitterProxy;- File: core-contracts/contracts/CoreCollection.sol (line 79)
string memory _collectionName,- File: core-contracts/contracts/CoreCollection.sol (line 80)
string memory _collectionSymbol,- File: core-contracts/contracts/CoreCollection.sol (line 81)
string memory _collectionURI,- File: core-contracts/contracts/CoreCollection.sol (line 122)
string memory _collectionName,- File: core-contracts/contracts/CoreCollection.sol (line 123)
string memory _collectionSymbol- File: core-contracts/contracts/CoreFactory.sol (line 71)
string memory _projectId,- File: core-contracts/contracts/CoreFactory.sol (line 72)
Collection[] memory _collections- File: core-contracts/contracts/CoreFactory.sol (line 109)
string memory _projectId,- File: core-contracts/contracts/CoreFactory.sol (line 110)
Collection memory _collection- File: core-contracts/contracts/CoreFactory.sol (line 128)
function getProject(string memory _projectId)- File: splits/contracts/SplitFactory.sol (line 79)
string memory _splitId- File: splits/contracts/SplitFactory.sol (line 105)
string memory _splitId[G-10] ++i/i++ should be unchecked{++i}/unchecked{++i} when it is not possible for them to overflow, as is the case when used in for- and while-loops
- File: core-contracts/contracts/CoreCollection.sol (line 279)
for (uint256 i = 0; i < _amount; i++) {- File: core-contracts/contracts/CoreFactory.sol (line 79)
for (uint256 i; i < _collections.length; i++) {- File: splits/contracts/Splitter.sol (line 50)
for (uint256 i = 0; i < currentWindow; i++) {- File: splits/contracts/Splitter.sol (line 274)
for (uint256 i = 0; i < proof.length; i++) {- File: core-contracts/contracts/CoreCollection.sol (line 279)
for (uint256 i = 0; i < _amount; i++) {- File: core-contracts/contracts/CoreFactory.sol (line 79)
for (uint256 i; i < _collections.length; i++) {- File: splits/contracts/Splitter.sol (line 50)
for (uint256 i = 0; i < currentWindow; i++) {- File: splits/contracts/Splitter.sol (line 274)
for (uint256 i = 0; i < proof.length; i++) {When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed
- File: splits/contracts/Splitter.sol (line 217)
function amountFromPercent(uint256 amount, uint32 percent)If needed, the value can be read from the verified contract source code
- File: splits/contracts/Splitter.sol (line 14)
uint256 public constant PERCENTAGE_SCALE = 10e5;- File: splits/contracts/Splitter.sol (line 15)
bytes4 public constant IID_IROYALTY = type(IRoyaltyVault).interfaceId;if (<x> == true) => if (<x>), if (<x> == false) => if (!<x>)
- File: royalty-vault/contracts/RoyaltyVault.sol (line 44)
IERC20(royaltyAsset).transfer(splitterProxy, splitterShare) == true,- File: royalty-vault/contracts/RoyaltyVault.sol (line 48)
ISplitter(splitterProxy).incrementWindow(splitterShare) == true,- File: royalty-vault/contracts/RoyaltyVault.sol (lines 52-55)
IERC20(royaltyAsset).transfer(
platformFeeRecipient,
platformShare
) == true,- File: splits/contracts/Splitter.sol (line 14)
uint256 public constant PERCENTAGE_SCALE = 10e5;- File: royalty-vault/contracts/ProxyVault.sol (line 9)
address internal royaltyVault;[G-17] require() or revert() statements that check input arguments should be at the top of the function
- File: splits/contracts/Splitter.sol (line 164)
require(royaltyAmount > 0, "No additional funds for window");- File: splits/contracts/Splitter.sol (lines 217-220)
function amountFromPercent(uint256 amount, uint32 percent)
private
pure
returns (uint256)- File: splits/contracts/Splitter.sol (lines 248-250)
function attemptETHTransfer(address to, uint256 value)
private
returns (bool)Contracts are allowed to override their parents' functions and change the visibility from external to public.
- File: royalty-vault/contracts/RoyaltyVault.sol (lines 95-100)
function supportsInterface(bytes4 interfaceId)
public
view
virtual
override(IRoyaltyVault, ERC165)
returns (bool)- File: royalty-vault/contracts/RoyaltyVault.sol (line 88)
function getSplitter() public view override returns (address) {- File: royalty-vault/contracts/RoyaltyVault.sol (Various lines throughout the file)
- File: splits/contracts/SplitFactory.sol (Various lines throughout the file)
- File: splits/contracts/Splitter.sol (Various lines throughout the file)
If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.
- File: core-contracts/contracts/CoreCollection.sol (lines 78-87)
function initialize(
string memory _collectionName,
string memory _collectionSymbol,
string memory _collectionURI,
uint256 _maxSupply,
uint256 _mintFee,
address _payableToken,
bool _isForSale,
address _splitFactory
) external onlyOwner onlyValidSupply(_maxSupply) {- File: core-contracts/contracts/CoreCollection.sol (lines 78-87)
function initialize(
string memory _collectionName,
string memory _collectionSymbol,
string memory _collectionURI,
uint256 _maxSupply,
uint256 _mintFee,
address _payableToken,
bool _isForSale,
address _splitFactory
) external onlyOwner onlyValidSupply(_maxSupply) {- File: core-contracts/contracts/CoreCollection.sol (lines 105-109)
function initializeClaims(bytes32 _root)
external
onlyOwner
onlyNotClaimableSet
onlyValidRoot(_root)- File: core-contracts/contracts/CoreCollection.sol (lines 105-109)
function initializeClaims(bytes32 _root)
external
onlyOwner
onlyNotClaimableSet
onlyValidRoot(_root)- File: core-contracts/contracts/CoreCollection.sol (lines 105-109)
function initializeClaims(bytes32 _root)
external
onlyOwner
onlyNotClaimableSet
onlyValidRoot(_root)- File: core-contracts/contracts/CoreCollection.sol (lines 121-124)
function setCollectionMeta(
string memory _collectionName,
string memory _collectionSymbol
) external onlyOwner {- File: core-contracts/contracts/CoreCollection.sol (lines 139-145)
function mintToken(
address to,
bool isClaim,
uint256 claimableAmount,
uint256 amount,
bytes32[] calldata merkleProof
) external onlyInitialized {- File: core-contracts/contracts/CoreCollection.sol (line 173)
function withdraw() external onlyOwner {- File: core-contracts/contracts/CoreCollection.sol (lines 185-187)
function setRoyaltyVault(address _royaltyVault)
external
onlyVaultUninitialized- File: core-contracts/contracts/CoreCollection.sol (line 203)
function setHashedProof(string calldata _proof) external onlyOwner {- File: core-contracts/contracts/CoreFactory.sol (lines 70-73)
function createProject(
string memory _projectId,
Collection[] memory _collections
) external onlyAvailableProject(_projectId) {- File: core-contracts/contracts/CoreFactory.sol (lines 108-111)
function addCollection(
string memory _projectId,
Collection memory _collection
) external onlyProjectOwner(_projectId) returns (address) {- File: core-contracts/contracts/CoreFactory.sol (lines 142-145)
function _createCollection(Collection memory _collection)
private
onlyAvailableCollection(_collection.id)
returns (address)- File: royalty-vault/contracts/RoyaltyVault.sol (line 67)
function setPlatformFee(uint256 _platformFee) external override onlyOwner {- File: royalty-vault/contracts/RoyaltyVault.sol (lines 76-79)
function setPlatformFeeRecipient(address _platformFeeRecipient)
external
override
onlyOwner- File: splits/contracts/SplitFactory.sol (lines 75-80)
function createSplit(
bytes32 _merkleRoot,
address _splitAsset,
address _collectionContract,
string memory _splitId
) external onlyAvailableSplit(_splitId) returns (address splitProxy) {- File: splits/contracts/SplitFactory.sol (lines 102-106)
function createSplit(
bytes32 _merkleRoot,
address _splitAsset,
string memory _splitId
) external onlyAvailableSplit(_splitId) returns (address splitProxy) {- File: splits/contracts/SplitFactory.sol (lines 120-122)
function setPlatformFee(address _vault, uint256 _platformFee)
external
onlyOwner- File: splits/contracts/SplitFactory.sol (lines 132-135)
function setPlatformFeeRecipient(
address _vault,
address _platformFeeRecipient
) external onlyOwner {sofianeOuafir (Joyn) confirmed and commented:
high quality report
C4 is an open organization governed by participants in the community.
C4 Contests incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Contest submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.
C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.