sherlock-audit / 2024-06-union-finance-update-2-judging

9 stars 4 forks source link

Nyxaris - Exchange Rate Manipulation via Supply and Redeemable Balance Distortion #42

Closed sherlock-admin4 closed 1 month ago

sherlock-admin4 commented 2 months ago

Nyxaris

High

Exchange Rate Manipulation via Supply and Redeemable Balance Distortion

Summary

An attacker can manipulate the exchange rate in the token system by exploiting the relationship between _totalRedeemable and totalSupplyin the _exchangeRateStored() function. This manipulation allows the attacker to artificially inflate the exchange rate, causing new users to receive fewer uTokens than expected when minting, and allowing the attacker to profit when redeeming their tokens.

Vulnerability Detail

The vulnerability lies in the exchange rate calculation:

function _exchangeRateStored() private view returns (uint256) {
    uint256 totalSupply_ = totalSupply();
    return totalSupply_ == 0 ? initialExchangeRateMantissa : (_totalRedeemable * WAD) / totalSupply_;
}

An attacker can exploit this by:

Minting a large amount of uTokens Borrowing a significant portion of the underlying assets Allowing interest to accrue Repaying the loan with interest, increasing _totalRedeemable Redeeming most of their uTokens, leaving a small totalSupply

This process results in a high _totalRedeemable value and a low totalSupply, artificially inflating the exchange rate.

Initial State:

initialExchangeRateMantissa = 1e18 (1:1 ratio) _totalRedeemable = 1,000,000 tokens totalSupply = 1,000,000 uTokens Exchange Rate = 1e18 (1 token = 1 uToken)

Step 1: Attacker mints 10,000,000 uTokens

_totalRedeemable = 11,000,000 tokens totalSupply = 11,000,000 uTokens Exchange Rate = 1e18 (unchanged)

Step 2: Attacker borrows 9,900,000 tokens

_totalRedeemable = 11,000,000 tokens (unchanged) totalSupply = 11,000,000 uTokens (unchanged) Exchange Rate = 1e18 (unchanged)

Step 3: Interest accrues (20% APR over a year)

Accrued interest = 1,980,000 tokens

Step 4: Attacker repays loan with interest

Repayment amount = 11,880,000 tokens _totalRedeemable = 22,880,000 tokens totalSupply = 11,000,000 uTokens (unchanged) New Exchange Rate = (22,880,000 * 1e18) / 11,000,000 ≈ 2.08e18

Step 5: Attacker redeems 10,900,000 uTokens

_totalRedeemable = 22,672,000 tokens totalSupply = 100,000 uTokens Final Exchange Rate = (22,672,000 * 1e18) / 100,000 ≈ 226.72e18

Impact on New Users

User A tries to mint with 1,000 tokens:

Expected: 1,000 uTokens Received: 1,000 * 1e18 / 226.72e18 ≈ 4.41 uTokens Loss: 995.59 uTokens worth of value

User B tries to mint with 10,000 tokens:

Expected: 10,000 uTokens Received: 10,000 * 1e18 / 226.72e18 ≈ 44.11 uTokens Loss: 9,955.89 uTokens worth of value

Attacker's Profit

Attacker mints again with 1,000,000 tokens:

Received: 1,000,000 * 1e18 / 226.72e18 ≈ 4,411 uTokens

Exchange rate returns to normal (assume 1:1 for simplicity):

Attacker redeems 4,411 uTokens Received: 4,411 tokens

Profit: 4,411 - 1,000,000 = 3,411 tokens

Impact

New users minting uTokens receive fewer tokens than they should, effectively losing value. The attacker can later mint uTokens at the inflated rate, gaining more underlying tokens when they redeem.

Code Snippet

https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/main/union-v2-contracts/contracts/market/UToken.sol#L810-#L835

    /**
     * @dev Mint uTokens by depositing tokens
     * @param amountIn The amount of the underlying asset to supply
     */
    function mint(uint256 amountIn) external override whenNotPaused nonReentrant {
        if (amountIn < minMintAmount) revert AmountError();
        if (!accrueInterest()) revert AccrueInterestFailed();
        uint256 exchangeRate = _exchangeRateStored();
        IERC20Upgradeable assetToken = IERC20Upgradeable(underlying);
        uint256 balanceBefore = assetToken.balanceOf(address(this));
        assetToken.safeTransferFrom(msg.sender, address(this), amountIn);
        uint256 balanceAfter = assetToken.balanceOf(address(this));
        uint256 actualObtained = balanceAfter - balanceBefore;
        uint256 mintTokens = 0;
        uint256 totalAmount = decimalScaling(actualObtained, underlyingDecimal);
        uint256 mintFee = decimalScaling((actualObtained * mintFeeRate) / WAD, underlyingDecimal);
        if (mintFee > 0) {
            // Minter fee goes to the reserve
            _totalReserves += mintFee;
        }
        // Rest goes to minting UToken
        uint256 mintAmount = totalAmount - mintFee;
        _totalRedeemable += mintAmount;
        mintTokens = (mintAmount * WAD) / exchangeRate;
        _mint(msg.sender, mintTokens);
        // send all to asset manager
        _depositToAssetManager(balanceAfter - balanceBefore);

        emit LogMint(msg.sender, mintAmount, mintTokens);
    }

Tool used

Manual Review

Recommendation

1.) Implement slippage protection for minting and redemption operations. 2.) Imlement a maximum exchange


function _exchangeRateStored() private view returns (uint256) {
    uint256 totalSupply_ = totalSupply();
    if (totalSupply_ == 0) return initialExchangeRateMantissa;

    uint256 calculatedRate = (_totalRedeemable * WAD) / totalSupply_;
    uint256 maxAllowedRate = initialExchangeRateMantissa * 2;  // Max 100% increase

    return Math.min(calculatedRate, maxAllowedRate);
}
3.)
Use a time-weighted average price (TWAP) for the exchange rate calculation:
```solidity
struct ExchangeRateObservation {
    uint256 timestamp;
    uint256 rate;
}

ExchangeRateObservation[] private rateHistory;

function updateExchangeRate() public {
    uint256 currentRate = _calculateCurrentRate();
    rateHistory.push(ExchangeRateObservation(block.timestamp, currentRate));
    if (rateHistory.length > 10) {
        // Keep only last 10 observations
        for (uint i = 0; i < 9; i++) {
            rateHistory[i] = rateHistory[i+1];
        }
        rateHistory.pop();
    }
}

function getTWAP() public view returns (uint256) {
    require(rateHistory.length > 0, "No rate history");
    uint256 sum = 0;
    for (uint i = 0; i < rateHistory.length; i++) {
        sum += rateHistory[i].rate;
    }
    return sum / rateHistory.length;
}
dmitriia commented 2 months ago

@mystery0x @WangSecurity _totalRedeemable calculation in Step 4 is incorrect as only interest is added to this accumulator, not the whole amount repaid:

UToken.sol#L710

            toRedeemableAmount = interest - toReserveAmount;

UToken.sol#L753

        _totalRedeemable += toRedeemableAmount;

This way it should be (setting protocol fees to zero for the sake of example):

Step 4: Attacker repays loan with interest

Repayment amount = 11,880,000 tokens
_totalRedeemable = 11,000,000 + 1,980,000 = 12,980,000 tokens
totalSupply = 11,000,000 uTokens (unchanged)
New Exchange Rate = (12,980,000 * 1e18) / 11,000,000 ≈ 1.18e18

Step 5: Attacker redeems 10,000,000 uTokens (they can’t redeem 10,900,000 as they minted only 10,000,000 in Step 1)

_totalRedeemable = 12,980,000 - 10,000,000 * 1.18 = 1,180,000 tokens
totalSupply = 1,000,000 uTokens
Final Exchange Rate = (1,180,000 * 1e18) / 1,000,000 ≈ 1.18e18

So the rate is increased by the interest attacker paid only. Impact on the users looks to be usual Vault mechanics. Also, rate cannot be reset back to 1.0, it’s accumulated with interest payouts in a one way fashion (i.e. _totalRedeemable is not modified on write offs).

olaoyesalem commented 1 month ago

@dmitriia , please review the steps carefully.In Step 4, both the interest and the repayAmount were added to the totalRedeemable, resulting in 12,980,000 tokens.Please recheck this.Additionally, this issue was not escalated and we've already passed the escalation window.

dmitriia commented 1 month ago

Hey @olaoyesalem, only interest (1,980,000) is added to _totalRedeemable, so the principal is not double counted:

UToken.sol#L704-L753

        if (repayAmount >= interest) {
            // Interest is split between the reserves and the uToken minters based on
            // the reserveFactorMantissa When set to WAD all the interest is paid to teh reserves.
            // any interest that isn't sent to the reserves is added to the redeemable amount
            // and can be redeemed by uToken minters.
            toReserveAmount = (interest * reserveFactorMantissa) / WAD;
>>          toRedeemableAmount = interest - toReserveAmount;  // @audit only interest

            // Update the total borrows to reduce by the amount of principal that has
            // been paid off
            _totalBorrows -= (repayAmount - interest);

            // Update the account borrows to reflect the repayment
            accountBorrows[borrower].principal = borrowedAmount - repayAmount;
            accountBorrows[borrower].interest = 0;

            uint256 pastTime = currTime - getLastRepay(borrower);
            if (pastTime > overdueTime) {
                // For borrowers that are paying back overdue balances we need to update their
                // frozen balance and the global total frozen balance on the UserManager
                IUserManager(userManager).onRepayBorrow(borrower, getLastRepay(borrower) + overdueTime);
            }

            // Call update locked on the userManager to lock this borrowers stakers. This function
            // will revert if the account does not have enough vouchers to cover the repay amount. ie
            // the borrower is trying to repay more than is locked (owed)
            IUserManager(userManager).updateLocked(
                borrower,
                decimalReducing(repayAmount - interest, underlyingDecimal),
                false
            );

            if (_getBorrowed(borrower) == 0) {
                // If the principal is now 0 we can reset the last repaid time to 0.
                // which indicates that the borrower has no outstanding loans.
                accountBorrows[borrower].lastRepay = 0;
            } else {
                // Save the current block timestamp as last repaid
                accountBorrows[borrower].lastRepay = currTime;
            }
        } else {
            // For repayments that don't pay off the minimum we just need to adjust the
            // global balances and reduce the amount of interest accrued for the borrower
            toReserveAmount = (repayAmount * reserveFactorMantissa) / WAD;
>>          toRedeemableAmount = repayAmount - toReserveAmount;  // @audit the whole payment is interest
            accountBorrows[borrower].interest = interest - repayAmount;
        }

        _totalReserves += toReserveAmount;
>>      _totalRedeemable += toRedeemableAmount;
olaoyesalem commented 1 month ago

@dmitriia but this doesn't still stop the attack from going through.

Initial State: initialExchangeRateMantissa = 1e18 (1:1 ratio) _totalRedeemable = 1,000,000 tokens totalSupply = 1,000,000 uTokens Exchange Rate = 1e18 (1 token = 1 uToken) Step 1: Attacker mints 10,000,000 uTokens _totalRedeemable = 1,000,000 tokens (unchanged) totalSupply = 11,000,000 uTokens Exchange Rate = (1,000,000 1e18) / 11,000,000 ≈ 0.09e18 Step 2: Attacker borrows 9,900,000 tokens _totalRedeemable = 1,000,000 tokens (unchanged) totalSupply = 11,000,000 uTokens (unchanged) Exchange Rate = (1,000,000 1e18) / 11,000,000 ≈ 0.09e18 (unchanged) Step 3: Interest accrues (20% APR over a year) Accrued interest = 1,980,000 tokens _totalRedeemable = 1,980,000 tokens totalSupply = 11,000,000 uTokens (unchanged) New Exchange Rate = (1,980,000 1e18) / 11,000,000 ≈ 0.18e18 Step 4: Attacker repays loan with interest Repayment amount = 11,880,000 tokens _totalRedeemable = 1,980,000 tokens (unchanged) totalSupply = 11,000,000 uTokens (unchanged) Exchange Rate = (1,980,000 1e18) / 11,000,000 ≈ 0.18e18 (unchanged) Step 5: Attacker redeems 10,900,000 uTokens _totalRedeemable = 1,980,000 tokens (unchanged) totalSupply = 100,000 uTokens Final Exchange Rate = (1,980,000 1e18) / 100,000 ≈ 19.8e18 Impact on New Users User A tries to mint with 1,000 tokens: Expected: 1,000 uTokens Received: 1,000 1e18 / 19.8e18 ≈ 50.51 uTokens Loss: 949.49 uTokens worth of value User B tries to mint with 10,000 tokens: Expected: 10,000 uTokens Received: 10,000 * 1e18 / 19.8e18 ≈ 505.05 uTokens Loss: 9,494.95 uTokens worth of value

olaoyesalem commented 1 month ago

Now look at this scenario. I wrote a PoC for it This PoC demonstrates a potential vulnerability in the contract where users could manipulate the exchange rate over time, potentially leading to unfair advantages for early participants or those who can time their interactions with the contract strategically.


function testExchangeRates() public {
    uint256 UNIT = 1e18;
     uint256 exchangeRateStoredbeforeAttack = uToken.exchangeRateStored();

        emit log_uint(exchangeRateStoredbeforeAttack);
    // Step 1: Mint
    uint256 mintAmount = 10 * UNIT;
    vm.startPrank(ALICE);
    erc20Mock.approve(address(uToken), mintAmount);
    uToken.mint(mintAmount);

    // Step 2: Borrow
    uint256 borrowAmount = 9 * UNIT;
    uToken.borrow(ALICE, borrowAmount);

    // Skip some time to accrue interest
    skip(block.timestamp + 30 days);

    // Step 3: RepayBorrow
    uint256 borrowed = uToken.borrowBalanceView(ALICE);
    uint256 interest = uToken.calculatingInterest(ALICE);
    uint256 repayAmount = borrowed + interest;
    erc20Mock.approve(address(uToken), repayAmount);
    uToken.repayBorrow(ALICE, repayAmount);

    // Step 4: Redeem
    uint256 uBalance = uToken.balanceOf(ALICE);
    uint256 redeemAmount = uBalance * 8 / 10;  // Redeem 80% of the balance
    uToken.redeem(redeemAmount, 0);

    // Log final state
    emit log_uint(uToken.totalRedeemable());
    emit log_uint(uToken.totalSupply());
    uint256 exchangeRateStoredAfterAttack = uToken.exchangeRateStored();
        //exchangeRateStoredbeforeAttack
        emit log_uint(exchangeRateStoredAfterAttack);
        assert(exchangeRateStoredAfterAttack>exchangeRateStoredbeforeAttack);
}

}

after running

forge test --match-test testExchangeRates  -vvv

we get

[⠒] Compiling...
[⠆] Compiling 130 files with Solc 0.8.16
[⠒] Solc 0.8.16 finished in 18.25s

Logs:
  1500000000000000000
  4354128909000000002
  1332000000000000001
  3268865547297297296

Which means the exchangeRate before the attack is 1500000000000000000 and the exchangeRate after the attack is 3268865547297297296 which means later users receive lesser tokens than first time users who can manipulate the contract. With this point. this is a valid issue

dmitriia commented 1 month ago

Your numbers in steps 4 and 5 are incorrect, redeem will return underlying tokens pro rata.

olaoyesalem commented 1 month ago

Check the PoC. and please understand the code! Also check the test file in the union code v2 to understand better. Thanks!

WangSecurity commented 1 month ago

I'm not sure I understand what's the problem here and it looks to be functioning as expected. The exchange rate should increase and it is expected and as pointed out by @dmitriia the exchange rate won't suddenly return to the initial state, because totalRedeemable doesn't decrease at any point. Therefore, the attacker and other users receive what they should.

Impact on New Users User A tries to mint with 1,000 tokens: Expected: 1,000 uTokens Received: 1,000 1e18 / 226.72e18 ≈ 4.41 uTokens Loss: 995.59 uTokens worth of value User B tries to mint with 10,000 tokens: Expected: 10,000 uTokens Received: 10,000 1e18 / 226.72e18 ≈ 44.11 uTokens Loss: 9,955.89 uTokens worth of value

I believe it's incorrect, they don't lose anything, they get what they should based on the current exchange rate. And if they withdraw, they will receive the same 1,000 and 10,000 of tokens they deposited, respectively.

Attacker's Profit Attacker mints again with 1,000,000 tokens: Received: 1,000,000 * 1e18 / 226.72e18 ≈ 4,411 uTokens Exchange rate returns to normal (assume 1:1 for simplicity): Attacker redeems 4,411 uTokens Received: 4,411 tokens Profit: 4,411 - 1,000,000 = 3,411 tokens

This is also incorrect, the exchange rate cannot return, as I understand. Hence, when redeeming their 4,411 uTokens, they would still receive 1,000,000 tokens back. Moreover, I don't understand how the calculations are done here. If the exchange rate returns to 1:1 (which won't happen), the attacker would receive 4,411 out of 1,000,000 they deposited, so the attacker loses and not profits.

Hence, I believe the report is incorrect and just describes how the protocol functions, i.e. exchange rate changes when the interest is paid. Thus, I plan to invalidate it.

olaoyesalem commented 1 month ago

@WangSecurity Please check the PoC . I wrote. Also I showed how an attacker can inflate the the exchangeRate greatly by more than 200%.

From the PoC the exchangeRate before the attack is 1500000000000000000 but after the attack the exchange rate rose to, 3268865547297297296, more than 2 times of the initial exchangeRate. The exchangeRate was maybe designed to increase, but not to more than 2 times of the intial exchangeRate after one attack as I have shown.

WangSecurity commented 1 month ago

I believe the exchange rate was updated as it should have been. Moreover, as I understand, none of the users suffers a loss due to this exchange rate. Hence, by just making the exchange rate high is not a medium or high severity.

olaoyesalem commented 1 month ago
testExchangeRates

We should note that the attacker has removed 80% of his funds and still inflated the exhcnageRate which will not favour new users . New Users will suffer it because the exchangeRate has really been inflated and they will get much more fewer tokens for the same fees the attacker paid. As highlighted in my report one way we can fix this is to

uint256 maxAllowedRate = initialExchangeRateMantissa * 2;  // Max 100% increase

This won't inflate it unnecessarily. a 200% increase after just one attack is worth looking at. as it can make users not interact with the protocol if they have to spend more thna neccessary and still get fewer token
@WangSecurity

WangSecurity commented 1 month ago

As I’ve said above, it doesn’t lead to any of the users losing funds and all of them get what they deposited, even though the new users get less tokens than they would before the attack, they don’t lose anything.

Just making the exchange rate higher is not medium or high by itself and I don’t see any med or high impact here. Hence, the decision remains the same, invalidate the finding.

MD-YashShah1923 commented 1 month ago

Let me share you my points why this should consider a valid High issue. @WangSecurity

How users lose funds here. -If an attacker is successful in making 1 share worth z assets and a user tries to mint shares using k*z assets then,

WangSecurity commented 1 month ago

@MD-YashShah1923

How users lose funds here. -If an attacker is successful in making 1 share worth z assets and a user tries to mint shares using kz assets then, If k<1, then the user gets zero share and they loose all of their tokens to the attacker If k>1, then users still get some shares but they lose (k- floor(k)) z) of assets which get proportionally divided between existing share holders (including the attacker) due to rounding errors. Users keep losing up to 33% of their assets.

Can you make a numerical example with exact numbers and links to the code where the rounding down would happen or where users would lose funds, because from previous examples I don’t see where the loss of funds happens (or adjust the previous POC)?

And to add one more point even the Watson who first commented this issue should be invalid, after discussion and POC he agreed as well that this issue should be valid.

You mean @dmitriia agreed? Unfortunately, I don’t see where they agree and what is more important I don’t see where the loss of funds happens here, but would be glad to see your example.

MD-YashShah1923 commented 1 month ago
WangSecurity commented 1 month ago

As the exchange Rate is inflated through the above given steps , now whenever the user deposits assets to mint shares he would get less shares or would loose whole amount of assests acccording to the amount of assests he provides

That's the point that it doesn't mean they lose assets. For example, exchange rate is 1 USDC == 1 UToken, then after the attack, 10 USDC == 1 UToken. After the attack the user deposits 1 USDC and gets 0.1 UToken. That doesn't mean they lost anything, since they can redeem 0.1 UToken and receive back their 1 USDC. The loss would be if they redeemed 0.1 UToken and received 0.1 USDC. But, I believe that's not the case. It seems to be what you tried to describe in the previous comment, but I need a numerical or coded proof that the user indeed loses tokens here, just inflating the exchange rate is not a loss, the user still gets UTokens equal to the value they provided. The previous POC only showed the high exchange rate, but no one lost ant funds I believe.

The watson argued above and afterwards gave no reply (many days passed away) which thus means he agreed upon the point

That doesn't mean they agree, more likely it means they got tired of arguing when the other side stopped listening.

According to me this should be valid high issue

Please prove there's a loss of funds. Just high exchange rate is not a loss of funds and not a loss of value. If you deposit 1 USDC and receive 0.1 UToken (as in example above), but then redeem that 0.1 UToken for 1 USDC, it's not a loss, there's no loss.

Would accept your decision @WangSecurity as lot of energy gets drained in arguing and it affects the other work we are doing parallely . And this goes even worse when the issue is not even escalated and watson just gave an argument to check it out and the argument was also wrong

Yep, I see this issue wasn't escalated, but it doesn't mean we have to reward the issue with almost no impact (just making the exchange rate high with no losses to anyone is at most Low).

Hence, if there's no coded or numerical proof with exact numbers, calculations and links to appropriate lines of code showing how users would lose funds, I will invalidate this report. Let me know if you need >24 hrs to make a POC @MD-YashShah1923 @olaoyesalem