cawfree - `SuperPoolFactory` Burns The Entirety Of The Initial Deposit

[sherlock-admin4]

cawfree

Medium

SuperPoolFactory Burns The Entirety Of The Initial Deposit

Summary

The permissionless SuperPoolFactory implements defensive mechanisms to protect against vault inflation attacks by burning the initial depositor's shares to the dead address. This combats the instantiation of pools with illiquid share supplies.

However, it burns the entirety of their initial deposit instead of just the burn amount, resulting in potentially significant loss for the deployer.

Vulnerability Detail

When creating a new SuperPool, the SuperPoolFactory ensures that the deployer burns a minimum of MIN_BURNED_SHARES to ensure a sufficiently liquid share supply upon construction:

function deploySuperPool(
    address owner,
    address asset,
    address feeRecipient,
    uint256 fee,
    uint256 superPoolCap,
    uint256 initialDepositAmt,
    string calldata name,
    string calldata symbol
) external returns (address) {
    if (fee != 0 && feeRecipient == address(0)) revert SuperPoolFactory_ZeroFeeRecipient();
    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);

    emit SuperPoolDeployed(owner, address(superPool), asset, name, symbol);
    return address(superPool);
}

This is a common strategy to protect against inflation attacks. However, Sentiment's approach is implemented incorrectly:

uint256 shares = superPool.deposit(initialDepositAmt, address(this));

if (shares < MIN_BURNED_SHARES) revert SuperPoolFactory_TooFewInitialShares(shares); 
IERC20(superPool).transfer(DEAD_ADDRESS, shares); /// @audit Burns *all* minted shares to the `DEAD_ADDRESS`.

Notice that when the sender makes a sufficiently liquid deposit resulting in the minting of a number of shares greater than or equal to MIN_BURNED_SHARES, all of the shares will be burned to the DEAD_ADDRESS.

This results in an entire loss for the initial depositor's deposit, since instead of sending only the necessary MIN_BURNED_SHARES to the DEAD_ADDRESS, all minted shares for the initial deposit are burned instead.

Impact

The first depositor loses their entire deposit to the DEAD_ADDRESS.

Code Snippet

https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/25a0c8aeaddec273c5318540059165696591ecfb/protocol-v2/src/SuperPoolFactory.sol#L47C5-L80C6

Tool used

Manual Review

Recommendation

Only transfer the MIN_BURNED_SHARES to the burn address, and return the remaining shares to the initial depositor. To ensure sufficient deployment incentive, ensure that the initial depositor may receive non-zero shares:

-  if (shares < MIN_BURNED_SHARES) revert SuperPoolFactory_TooFewInitialShares(shares);
+  if (shares <= MIN_BURNED_SHARES) revert SuperPoolFactory_TooFewInitialShares(shares);
-  IERC20(superPool).transfer(DEAD_ADDRESS, shares);
+  IERC20(superPool).transfer(DEAD_ADDRESS, MIN_BURNED_SHARES);
+  IERC20(superPool).transfer(receiver, shares - MIN_BURNED_SHARES);

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

z3s commented:

Deployers are trusted admins and they know what are they doing

[cawfree]

Escalate

Deployers are trusted admins and they know what are they doing

It should be emphasized that deploySuperPool is permissionless.

Callers to deploySuperPool are certainly not guaranteed to be trusted admins, rather, the protocol design is specifically designed for new SuperPools to be deployed by users.

Therefore, we can anticipate any user can indeed attempt to create a new SuperPool with the reasonable expectation that the majority of their deposit will not be sacrificed to the burn address.

[sherlock-admin3]

Escalate

Deployers are trusted admins and they know what are they doing

It should be emphasized that deploySuperPool is permissionless.

Callers to deploySuperPool are certainly not guaranteed to be trusted admins, rather, the protocol design is specifically designed for new SuperPools to be deployed by users.

Therefore, we can anticipate any user can indeed attempt to create a new SuperPool with the reasonable expectation that the majority of their deposit will not be sacrificed to the burn address.

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.

[cvetanovv]

The function works properly. shares is the initial deposit that should be burned.

The only requirement is that the initial deposit is not less than MIN_BURNED_SHARES(1000) to prevent vault inflation attacks.

If a user decides to call the function with, for example, 100,000 tokens instead of 1,000, what fault does the protocol have?

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

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• cawfree: rejected