Platform: Code4rena
Start Date: 30/03/2022
Pot Size: $30,000 USDC
Total HM: 21
Participants: 38
Period: 3 days
Judge: Michael De Luca
Total Solo HM: 10
Id: 104
League: ETH
Rank: 7/38
Findings: 5
Award: $1,386.49
🌟 Selected for report: 2
🚀 Solo Findings: 0
🌟 Selected for report: kirk-baird
724.5042 USDC - $724.50
https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L31 https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L165-L166
Splitter.currentWindow can be made large enough so that claimForAllWindows() run out of gas on the each run, making it unavailable.
Furthermore, as balanceForWindow array will become large enough, finding a window with any substantial amount to claim becomes a task too hard to be solved manually, so it will require some automated solutions from all the users to locate any meaningful amounts to claim via claim(window, allocation, proof), which become the only way to claim.
Running claim(i, ...) for all i will involve too much transaction costs to be economically feasible for the most users.
This way cost of usage will increase and reputational risk emerges.
Setting severity to be medium as core functionality availability is substantially diminished as the result of the attack.
Suppose platformFee is 100, i.e. 1%.
Bob the attacker can do say 10^3 repetitions of the steps (1-2):
send 100 wei of royaltyAsset to the royaltyVault balance
run sendToSplitter(), which have no access controls, distributing the whole current royaltyAsset balance of the vault to platform (1 wei) and splitter (99 wei):
https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L31
On each run of (2) currentWindow will be incremented and balanceForWindow array expanded by one element:
https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L165-L166
One simple solution is to check in sendToSplitter() that getVaultBalance() is greater than MIN_ROYALTY_AMOUNT, which is a new additional configuration threshold variable, proceeding with splitting, sending and window increase only when this threshold is met.
This will substantially increase the cost of the attack and also reduce overall transaction overhead of the system by removing the low amount actions.
#0 - sofianeOuafir
2022-04-14T19:18:39Z
In my opinion, the severity level should be 3 (High Risk) instead of 2 (Med Risk) duplicate of #3
#1 - deluca-mike
2022-04-22T02:19:16Z
This is more a duplicate of #6, since it pertains to the possibility of claimForAllWindows being made prohibitively expensive, regardless if its from an attacker abusing incrementWindow, or natural growth of currentWindow without claims.
🌟 Selected for report: hyh
Also found by: Ruhum, WatchPug, hubble, kirk-baird, leastwood, pedroais, rayn, saian, securerodd
70.1719 USDC - $70.17
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:
https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L87
Add onlyUnInitialized modifier to the initialize function:
#0 - sofianeOuafir
2022-04-14T19:42:04Z
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)
#1 - deluca-mike
2022-04-22T02:30:08Z
Not convinced this is a high severity issue, since erroneously changing payableToken via 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 behind onlyOwner.
However, if it can be shown that users other than the owner can end up losing value due to the owner abusing or erroneously using initialize, 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.
🌟 Selected for report: kirk-baird
Also found by: 0xDjango, Dravee, Ruhum, TomFrenchBlockchain, WatchPug, defsec, hubble, hyh, leastwood, minhquanym
85.0569 USDC - $85.06
Placing severity to medium here as setting out of bounds platformFee is relatively easy (different systems have different fee decimals, there is no standard at the moment, so it's not hard to mix things up), while the impact can be strong enough.
For example, if platformFee > 10000 all CoreCollection's transfers will be blocked as _beforeTokenTransfer -> sendToSplitter call will always revert:
Another example, if platformFee == 10000 the same call will always revert, just a little bit later as splitterShare be zero, while incrementWindow requires it to be positive:
https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L164
platformFee is supposed to be in (0, 10000) range, but it is not controlled by RoyaltyVault and RoyaltyVaultFactory:
Consider adding range check for platformFee in RoyaltyVault's setPlatformFee
#0 - sofianeOuafir
2022-04-15T12:48:40Z
duplicate of #9
362.2521 USDC - $362.25
https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L51-L57 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 wei of 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 to platformFeeRecipient
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:
https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L41
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(...); } }
144.508 USDC - $144.51
Fees will be lost after FeeRecipient be set to zero by mistake or as a part of griefing attack.
platformFeeRecipient isn't controlled to be non-zero address:
https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L81
It is, however, controlled on the factory level:
Consider unifying the approach and add the check to the RoyaltyVault
Having different fraction/percentage conventions within one system is error prone and can easily lead to wrong amount calculations and end up with fund losses.
amountFromPercent use 10^2:
https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L223
scaleAmountByPercentage use 10^4:
https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L103
Consider unifying the fraction/percentage decimals convention by making it 10^4 everywhere
A range of malfunctions is possible if core configuration parameters are set by mistake to any values that make little sense. It is also not always right away evident that such a mistake took place
CoreCollection initialize() doesn't check the parameters supplied:
initialize() is run by end users via CoreFactory, that doesn't check the parameters either:
Consider adding a layer of control to the CoreCollection initialization
Amount calculations can become incorrect with a range of impacts up to fund loss and insolvency if contract dedicated constant value and the values hard coded across the implementation differ and are used in various parts of the code with the same intent.
Now PERCENTAGE_SCALE is set to be 10^5 and is not used:
https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L14
While hard coded 10^4 is used in scaleAmountByPercentage:
https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L103
scaleAmountByPercentage is involved in calculations (while PERCENTAGE_SCALE is not):
https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L54
https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L137
Consider unifying the fraction/percentage decimals convention by making it 10^4 everywhere, and removing hard code, utilizing PERCENTAGE_SCALE in all related calculations
Filtering on unindexed events is disabled, which makes it harder to programmatically use and analyse the system.
CoreCollection events aren't indexed at all:
Splitter's WindowIncremented one too:
https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L28
CoreCollection's events also:
Consider adding indexes to ids and addresses in the all important events to improve their usability
transferSplitAsset comments state that it's ETH transfers, while it's not:
https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L235
Update comments to reflect ERC20 token transfer
If there be any emergency with system contracts or outside infrastructure, there is no way to temporary stop the operations.
All core user-facing contracts do not have pausing functionality for new operation initiation.
For example, both CoreFactory’s user facing functions cannot be temporary paused:
https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreFactory.sol#L70
https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreFactory.sol#L108
sendToSplitter also can benefit from having pausing functionality:
https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L31
Consider making all new actions linked user facing functions pausable.
For example, by using OpenZeppelin's approach:
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/Pausable.sol
As different compiler versions have critical behavior specifics if the contracts get accidentally deployed using another compiler version compared to the one they were tested with, various types of undesired behavior can be introduced
pragma solidity ^0.8.4 and pragma solidity ^0.8.0 is used in most contracts, for example:
https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L2
https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreFactory.sol#L2
Consider fixing the version to 0.8.x across all the codebase, for example set x to 10
_createCollection's initialize call will revert with low level message on deploy failure.
_createCollection doesn't check new CoreProxy call result, invokes initialize in all the cases:
Require non zero coreCollection address before proceeding with initialize.