Price Calculation Discrepancy in Asset Conversion

[hats-bug-reporter[bot]]

Github username: -- Twitter username: ACai_sec Submission hash (on-chain): 0x33d536198fecda7c86619265c0e63f9b8400261c847e74836e4591b11fa232c7 Severity: medium

Description: Description\ A vulnerability exists in the price calculation mechanism between deposits and withdrawals in the InvestToken contract. The issue arises due to inconsistent price references when converting between assets (USDE) and shares, potentially causing users to suffer losses during emergency withdrawals.

Attack Scenario\

// In InvestToken.sol
function deposit(uint256 assets, address receiver) public returns (uint256 shares) {
    shares = convertToShares(assets);  // Uses currentPrice
    usde.burn(msg.sender, assets);
    _mint(receiver, shares);
}

function withdraw(uint256 assets, address receiver, address owner) public returns (uint256 shares) {
    shares = convertToShares(assets);  // Uses currentPrice for calculation
    _burn(owner, shares);
    usde.mint(receiver, assets);
}

// In YieldOracle.sol
function assetsToShares(uint256 assets) external view returns (uint256) {
    return Math.mulDiv(assets, 10 ** 18, currentPrice);
}

function sharesToAssets(uint256 shares) external view returns (uint256) {
    return Math.mulDiv(shares, previousPrice, 10 ** 18);  // Uses previousPrice
}

Consider the following scenario:

Initial state:

currentPrice = 100 USDE/share
previousPrice = 90 USDE/share
User deposits 1000 USDE:

shares = 1000 * (10**18) / 100 = 10 shares  // Using currentPrice
Oracle malfunctions or updates are delayed
User attempts emergency withdrawal:

assets = 10 * 90 / (10**18) = 900 USDE  // Using previousPrice
Result: User loses 100 USDE (10% loss) due to price calculation discrepancy

Attachments

1. Proof of Concept (PoC) File

   contract PriceDiscrepancyTest {
   function testPriceDiscrepancy() public {
       // Setup
       address user = address(this);
       uint256 depositAmount = 1000e18;
   
       // Step 1: Deposit
       usde.approve(address(investToken), depositAmount);
       uint256 shares = investToken.deposit(depositAmount, user);
   
       // Step 2: Simulate oracle malfunction
       // (price update delay or oracle address misconfiguration)
   
       // Step 3: Emergency withdrawal
       uint256 withdrawnAmount = investToken.redeem(shares, user, user);
   
       // Assert: withdrawnAmount < depositAmount
       assert(withdrawnAmount < depositAmount);
   }
   }

2. Revised Code File (Optional)

[AndreiMVP]

Yeah, seems like a valid issue

[PlamenTSV]

Judging by previous comments, isn't the usage of the previous price a protective mechanism against arbitrage? In case the Oracle malfunctions, the owner can manually update the price to keep the protocol running and solvent. I see no issue currently, users should be aware that holding the deposit for 1 price update is mandatory to avoid losses.

[AndreiMVP]

Alright, I read this more carefully and @PlamenTSV is right, this submissions puts emphasis on previous and current price, which is intended design. But there is the issue I understood when first looking at this, and the submitter also seems to hint at this - that convertToShares which uses currentPrice is being used in both deposit() and withdraw() (while the idea is when investing currentPrice is used and when selling previousPrice is used).

This was the first submission that first hinted at this, but the submitter might have misunderstood the intended behavior.

Since the scope of currentPrice and previousPrice are explained in the README, and there were some submissions which explained the issue more precisely relative to the intended behavior, it might be fair to reward one of those. We'll come up with a decision in the coming days.