Open sherlock-admin4 opened 2 months ago
The pool creator can simply redeploy the pool, this is not an issue of significance. Furthermore, that exact pool can be created as the nonce increases even upon a transaction reverting (source: https://ethereum.stackexchange.com/a/77049), thus a different address will be generated.
Escalate, Per above comment
Escalate, Per above comment
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
The pool creator can simply redeploy the pool, this is not an issue of significance. Furthermore, that exact pool can be created as the nonce increases even upon a transaction reverting (source: https://ethereum.stackexchange.com/a/77049), thus a different address will be generated.
Still doesn't change the fact that a user can frontrun the deploySuperPool
transactions by donating a small amount of the token to the future address and DOS all the pool creation attempts.
I agree with @Kalogerone comment. Even if the pool creator redeploys the pool, the malicious user can DoS also the new pool.
Planning to reject the escalation and leave the issue as is.
Result: Medium Has Duplicates
@cvetanovv Sorry for the late reply
Sherlock has clear rules on DOS issues that can be considered valid:
Could Denial-of-Service (DOS), griefing, or locking of contracts count as a Medium (or High) issue? DoS has two separate scores on which it can become an issue:
The issue causes locking of funds for users for more than a week.
The issue impacts the availability of time-sensitive functions (cutoff functions are not considered time-sensitive). If at least one of these are describing the case, the issue can be a Medium. If both apply, the issue can be considered of High severity. Additional constraints related to the issue may decrease its severity accordingly.
Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.
Both of this is not the case here. Similar is the case for https://github.com/sherlock-audit/2024-08-sentiment-v2-judging/issues/400
V. How to identify a medium issue:
- Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.
- Breaks core contract functionality, rendering the contract useless or leading to loss of funds.
It's pretty self explanatory that this issue "Breaks core contract functionality, rendering the contract useless". I don't think it is appropriate to play with the words here and try to invalidate an issue that denies the ability to create a pool in a protocol based on user created pools.
With this attack that requires minimal funds, the malicious user breaks the contract and enters the category:
Breaks core contract functionality, rendering the contract useless or leading to loss of funds.
The protocol team fixed this issue in the following PRs/commits: https://github.com/sentimentxyz/protocol-v2/pull/333
Kalogerone
High
Griefer can DOS the
SuperPool
creation and make it very expensive for other usersSummary
The
SuperPoolFactory.sol
contract creates newSuperPool
instances using thenew
keyword, which is essentially using theCREATE
opcode. This means that the address of the nextSuperPool
instance can be known by any user. To create a newSuperPool
, it's essential to deposit and burn a minimum of 1000 shares. A griefer can frontrunSuperPool
creation transactions andtransfer
small amounts of tokens to the knownSuperPool
address to make shares expensive and prevent the creation of theSuperPool
.Root Cause
When using the
CREATE
opcode, the new contract address depends on the deployer address (theSuperPoolFactory.sol
address which is known) and its nonce (which can be calculated by simply looking atSuperPoolFactory
's etherscan). Even ethers has a function to calculate the next address. This means that the nextSuperPool
address that will be created is known and can't be changed.SuperPool
creation requires the user to deposit and burn a minimum of1000 shares
, otherwise the transaction will revert.deploySuperPool:
Note that
uint256 public constant MIN_BURNED_SHARES = 1000;
An attacker can frontrun this transaction from a regular user and donate to the already known
address
a small amount of theSuperPool
's selected asset to inflate the shares and make them very expensive for the user to create theSuperPool
(exact numbers shown in the coded PoC).The shares inflation happens because of the
_convertToShares
function used in thedeposit
function:Normally a user would only need
1000 assets
to mint1000 shares
(1000 1 / 1 = 1000 shares using the_convertToShares
formula above). Imagine a donation of1000000 assets
before the transaction. Now1000 assets
would give0 shares
(1000 1 / 1000001 = 0 shares). With a token likeUSDC
which has 6 decimals and is in scope, a user would need $1000 to overcome a $1 donation and mint1000 shares
.Internal pre-conditions
No response
External pre-conditions
No response
Attack Path
SuperPool
.SuperPool
.SuperPool
asset.SuperPool
.Impact
It will become very expensive to create a
SuperPool
, many users won't want to do it andSuperPools
will stop getting created.PoC
Paste the following code in the
test/core/Superpool.t.sol
test file and follow the comments:Mitigation
Don't require from the user to deposit and actually mint the dead shares. You can hardcode them in the
SuperPool
contract by making for e.g.:totalAssets
function to return the actual total assets + 1000totalSupply
function to return the actual total supply + 1000