search

sherlock-audit / 2023-01-uxd-judging

3 stars 1 forks source link

berndartmueller - Redeeming all UXD tokens is not possible if some have been minted via Perp quote minting #338

Open github-actions[bot] opened 1 year ago

github-actions[bot] commented 1 year ago

berndartmueller

high

Redeeming all UXD tokens is not possible if some have been minted via Perp quote minting

Summary

If some UXD tokens have been minted via Perp quote minting (i.e. USDC - due to negative PnL), rather than asset minting (i.e. WETH), redeeming all UXD tokens is not possible anymore due to not accounting for quote minting in the netAssetDeposits variable.

Vulnerability Detail

The Perp integration allows users to deposit assets and return the redeemable (UXD) amount that can be minted. Assets can be deposited in two ways: either assetToken or quoteToken. quoteToken (i.e., USDC) can only be deposited if there's a negative PnL > amount to pay off the negative PnL.

Depositing assetToken (i.e., WETH) increases the internal accounting variable netAssetDeposits, which is the amount of assetToken deposited minus the amount of assetToken withdrawn. Withdrawing assetToken decreases netAssetDeposits by the amount withdrawn.

However, depositing quoteToken (if available) does not increase the netAssetDeposits variable. This leads to more UXD tokens being minted than assetToken deposited.

Thus, redeeming all UXD tokens is not possible anymore if some UXD tokens have been minted via quote minting. Users who are left last with redeeming their UXD tokens cannot do so.

Consider the following example:

Time Action netAssetDeposits UXD supply
T0 Alice deposits 10 ether of assetToken 10 ether 10_000e18
T1 Bob deposits 10 ether of assetToken 20 ether 20_000e18
T2 Caroline deposits 1_000e6 of quoteToken (due to negative PnL) 20 ether 21_000e18
T10 Caroline redeems 1_000e18 UXD via 1 ether assetToken 19 ether 20_000e18
T11 Bob redeems 10_000e18 UXD via 10 ether assetToken 9 ether 10_000e18
T12 Alice redeems 10_000e18 UXD via 10 ether assetToken -> fails 9 ether 10_000e18

Alice is not able to redeem her balance of 10_000e18 UXD because netAssetDeposits = 9 ether is insufficient to redeem 10_000e18 UXD = 10 ether.

Impact

UXD tokens can not be fully redeemed via the Perp depository if some have been minted via quote minting.

Code Snippet

integrations/perp/PerpDepository.sol#L284

Asset token deposits increase the internal accounting variable netAssetDeposits by the amount deposited.

/// @notice Deposits collateral to back the delta-neutral position
/// @dev Only called by the controller
/// @param amount The amount to deposit
function _depositAsset(uint256 amount) private {
    netAssetDeposits += amount;

    IERC20(assetToken).approve(address(vault), amount);
    vault.deposit(assetToken, amount);
}

integrations/perp/PerpDepository.sol#L298

Withdrawing asset tokens (i.e. WETH) decreases the internal accounting variable netAssetDeposits by the amount withdrawn and thus limits the amount of redeemable UXD tokens to netAssetDeposits.

/// @notice Withdraws collateral to used in the delta-neutral position.
/// @dev This should only happen when redeeming UXD for collateral.
/// Only called by the controller.
/// @param amount The amount to deposit
function _withdrawAsset(uint256 amount, address to) private {
    if (amount > netAssetDeposits) {
        revert InsufficientAssetDeposits(netAssetDeposits, amount);
    }
    netAssetDeposits -= amount;

    vault.withdraw(address(assetToken), amount);
    IERC20(assetToken).transfer(to, amount);
}

integrations/perp/PerpDepository.redeem

Redeeming via quote tokens is disabled. Only redeeming via assetToken is possible. Thus, leaving no other option than to redeem via assetToken.

function redeem(
        address asset,
    uint256 amount
) external onlyController returns (uint256) {
    if (asset == assetToken) {
        (uint256 base, ) = _openLong(amount);
        _withdrawAsset(base, address(controller));
        return base;
    } else if (asset == quoteToken) {
        revert QuoteRedeemDisabled(msg.sender);
        // return _processQuoteRedeem(amount);
    } else {
        revert UnsupportedAsset(asset);
    }
}

Tool used

Manual Review

Recommendation

Consider changing the internal accounting of netAssetDeposits to include quoteToken deposits.

berndartmueller commented 1 year ago

Escalate for 25 USDC

I think this issue is wrongly duped. The duped issues mention a similar issue in the context of the rebalancing functions. This issue demonstrated here is about quote minting in the case of a negative PnL.

I think it's justified to leave this issue on its own without the duplicate issues. The duped issues should be left as a separate issue (Please note that #434 does not mention the real impact of not being able to redeem fully).

(https://github.com/sherlock-audit/2023-01-uxd-judging/issues/97 has also been considered a separate issue)

sherlock-admin commented 1 year ago

Escalate for 25 USDC

I think this issue is wrongly duped. The duped issues mention a similar issue in the context of the rebalancing functions. This issue demonstrated here is about quote minting in the case of a negative PnL.

I think it's justified to leave this issue on its own without the duplicate issues. The duped issues should be left as a separate issue (Please note that #434 does not mention the real impact of not being able to redeem fully).

(https://github.com/sherlock-audit/2023-01-uxd-judging/issues/97 has also been considered a separate issue)

You've created a valid escalation for 25 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

hrishibhat commented 1 year ago

Escalation accepted

Agreed that the issues is different from its dupes. Separating from the rest of the issues #178, #262, #141 as they are considered as low together, since not updating netAssetDeposits on rebalance does not cause any real issues. Also, issue #434 touches upon the issue partially, but does not fully describe the complete issue & its impact. Hence considering it a low.

sherlock-admin commented 1 year ago

Escalation accepted

Agreed that the issues is different from its dupes. Separating from the rest of the issues #178, #262, #141 as they are considered as low together, since not updating netAssetDeposits on rebalance does not cause any real issues. Also, issue #434 touches upon the issue partially, but does not fully describe the complete issue & its impact. Hence considering it a low.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

hrishibhat commented 1 year ago

Fix: https://github.com/UXDProtocol/uxd-evm/pull/32

IAm0x52 commented 1 year ago

Fixed in same PR as #250