Price Manipulation Vulnerability in Withdraw Function

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x9c0eb42c4cbd261b6c918bb510fe8772f9439a1db67823d1b31423f7cf664f7b Severity: high

Description: Description\ A discrepancy exists in the InvestToken contract’s withdraw and redeem functionalities that allows users to receive a disproportionate amount of assets (USDE) while retaining a portion of their shares (InvestToken). When using withdraw with maxWithdraw calculated, the user's assets are calculated based on userTotalShare * previousPrice / (10 18), but the amount of shares removed is calculated with userTotalShare * previousPrice / currentPrice - the remain share is userTotalShare - userTotalShare * previousPrice / currentPrice = userTotalShare*(1-previousPrice/currentPrice). Conversely, if the user utilizes the redeem function and redeems the maximum redeemable amount, they receive the same calculated asset amount (userTotalShare * previousPrice / (10 18)) but fully redeem all shares.

Attack Scenario\ If the user proceeds with withdrawal again with the remaining shares and repeats this operation until the end, the maximum amount of assets that can be received is as follows. userTotalShare * previousPrice / (10 ** 18) + userTotalShare*(1-previousPrice/currentPrice) * previousPrice / (10 ** 18) + [(userTotalShare*(1-previousPrice/currentPrice)) ** 2] * previousPrice / (10 ** 18) + [(userTotalShare*(1-previousPrice/currentPrice)) ** 3] * previousPrice / (10 ** 18) + ... = userTotalShare * previousPrice / (10 ** 18) / [1 - (1-previousPrice/currentPrice)] = userTotalShare * currentPrice/ (10 ** 18)

In this way, if a user deposits with currentPrice and then executes commitPrice to change currentPrice with nextPrice(nextPrice>currentPrice) and then withdraws, he will obtain more assets. This causes a loss of funds in the protocol.

Attachments\ Code Analysis: Demonstrating the calculation discrepancies in withdraw versus redeem. Mathematical Justification: Series expansion example of the iterative extraction process using the discrepancy in share deduction, showcasing how an attacker can effectively extract userTotalShare * currentPrice / (10 ** 18) instead of a proportionate asset amount.

[AndreiMVP]

Related to https://github.com/hats-finance/Euro-Dollar-0xa4ccd3b6daa763f729ad59eae75f9cbff7baf2cd/issues/34

[newspacexyz]

My report(#48) is different with #34.

34 said only asset amount of redeem is less than amount of deposit asset.

In other words, #34 is about the loss of the user's funds. And my report is about the loss of protocol's funds.

amount of deposit asset(depositAssetAmount)

share of user received after deposit(userTotalShare) = depositAssetAmount * (10 ** 18) / currentPrice User can maxWithdraw: amount of maxWithdraw asset(withdrawAssetAmount1) = userTotalShare * previousPrice / (10 ** 18) removed share = userTotalShare * previousPrice / currentPrice remain share(remainShare1) = userTotalShare - userTotalShare * previousPrice / currentPrice = userTotalShare*(1-previousPrice/currentPrice)

User can maxWithdraw with the remain share: amount of maxWithdraw asset(withdrawAssetAmount2) = remainShare1 * previousPrice / (10 ** 18) = withdrawAssetAmount1 (1-previousPrice/currentPrice) removed share = `remainShare1 previousPrice / currentPrice remain share =remainShare1(1-previousPrice/currentPrice) And go on like this: withdrawAssetAmount3=withdrawAssetAmount2` (1-previousPrice/currentPrice) = withdrawAssetAmount1 * [(1-previousPrice/currentPrice) ** 2] withdrawAssetAmount4 = withdrawAssetAmount3 (1-previousPrice/currentPrice) = withdrawAssetAmount1 [(1-previousPrice/currentPrice) ** 3]

User can withdraw until the remain share is 0 and the total withdraw asset amount is : withdrawAssetTotal = withdrawAssetAmount1 + withdrawAssetAmount2 + withdrawAssetAmount3 + ... withdrawAssetAmout(i) is geometric sequence with a common ratio of q=(1-previousPrice/currentPrice) < 1, so withdrawAssetTotal = withdrawAssetAmount1 / (1 - q) = userTotalShare * currentPrice / (10 ** 18)

If user deposits and withdraw with the same current price, depositAssetAmount is equal to withdrawAssetTotal. But if user deposits assets(A) with lower current price(p0) just before commitPrice, and then does commitPrice for higher current price(pending next price: p1 > p0) and withdraw, he can get much more assets(A * p1 / p0). Of course, it may differ from the formula due to precision loss, but any user can benefit from the price difference just before and after commitPrice.

[newspacexyz]

Hi, @AndreiMVP Any update with this issue review?

[AndreiMVP]

So while #34 might not be the highlighting the core issue, your issue still falls behind #41 #45 so we'll have to consider this as duplicate

[newspacexyz]

@AndreiMVP #45's attack Scenario never contains commitPrice, but my scenario uses commitPrice.

45 and #48 are different. Please read carefully my report once again.

[newspacexyz]

45 used mint and withdraw.

48 used mint and commitPrice and maxWithdraw several times.

[AndreiMVP]

Unless you highlight a core issue that wasn't touched by previous reports, just providing different attack permutations doesn't justify this being a valid submission.

[newspacexyz]

I'm sorry. #48 used deposit(not mint) and commitPrice and maxWithdraw .

45 used mint and withdraw so the #45' core issue is the difference between converToAssets(for mint) and converToShare(for withdraw).

48's core issue is the change of currentPrice. In other word, my attack scenario always uses converToShare.

[newspacexyz]

@AndreiMVP Please review my comment again.

[AndreiMVP]

I see the same core issue - mint and withdraw using the inverse conversion price they should. Investing should be done at currentPrice and selling at previousPrice. withdraw should have used previousPrice.

In this way, if a user deposits with currentPrice and then executes commitPrice to change currentPrice with nextPrice(nextPrice>currentPrice) and then withdraws, he will obtain more assets. This causes a loss of funds in the protocol.

In correct behavior when committing the price, attacker wouldn't profit because selling would be done at the same price that he previously bought at.

Your writing is a bit confusing without a test file and if you just name functions you used compared to others it doesn't help me (I don't see what maxWithdraw adds to the core issue here).

[newspacexyz]

maxWithdraw is literally the maximum amount of assets tha the owner can withdraw. But if user withdraws with the maxWithdraw amount, some share remains and user can maxwithdraw with the reamain amount. In a word, maxWithdraw is incorrect and attacker can use this Vulnerability.

[newspacexyz]

@AndreiMVP If the issues with different attack scenario has the same mitigation, they are dup? If this is what you mean, no more opinion.

[newspacexyz]

@AndreiMVP , If #34 and #41 is low, my issue could be low. What do you think about it?

[AndreiMVP]

If the issues with different attack scenario has the same mitigation, they are dup? If this is what you mean, no more opinion.

Fair to both parties (auditors and owners) is to reward only the first submitter that explained the core issue. Otherwise once an issue is found and others submit different attack vectors corresponding to it, the first submitter would share his pie unfairly.

If https://github.com/hats-finance/Euro-Dollar-0xa4ccd3b6daa763f729ad59eae75f9cbff7baf2cd/issues/34 and https://github.com/hats-finance/Euro-Dollar-0xa4ccd3b6daa763f729ad59eae75f9cbff7baf2cd/issues/41 is low, my issue could be low. What do you think about it?

Well those were earlier submitted than #45 which we marked as high because it was the first to provide a clear overview of the core issue. If #45 was first submission out of all, it would have been the only one rewarded.

[newspacexyz]

What's your thought about https://github.com/hats-finance/Euro-Dollar-0xa4ccd3b6daa763f729ad59eae75f9cbff7baf2cd/issues/48#issuecomment-2486157558 ? @AndreiMVP

[AndreiMVP]

Well, I think it corresponds to the unintended behavior of withdraw.