blutorque - Theft of dollar token

[sherlock-admin]

blutorque

high

Theft of dollar token

Summary

Attacker can exploit the wrong accounting in getDollarInCollateral fn to pay less collateral than is needed.

Vulnerability Detail

The issues lies in getDollarInCollateral fn, the way we use it with mintDollar fn:

    function getDollarInCollateral(
        uint256 collateralIndex,
        uint256 dollarAmount
    ) internal view returns (uint256) {
        UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();
        return
            dollarAmount  
                .mul(UBIQUITY_POOL_PRICE_PRECISION)
                .div(10 ** poolStorage.missingDecimals[collateralIndex])
                .div(poolStorage.collateralPrices[collateralIndex]); 
    }

To mintDollar, a user can specified collateralIndex, dollarAmount, etc. The fn internally uses getDollarInCollateral to determine how much collateral is needed to mint user-specified dollarAmount. Before this call, it ensures the dollar USD price is up to date(via calling LibTWAPOracle.update()). However, it fails to convert the dollarAmount to the USD value of dollarAmount before passing it to the getDollarInCollateral fn.

function mintDollar(
    uint256 collateralIndex,
    uint256 dollarAmount,
    uint256 dollarOutMin,
    uint256 maxCollateralIn
)
    internal
    collateralEnabled(collateralIndex)
    returns (uint256 totalDollarMint, uint256 collateralNeeded)
{
    ...SNIP...
    // update Dollar price from Curve's Dollar Metapool
    LibTWAPOracle.update();

    require(
        getDollarPriceUsd() >= poolStorage.mintPriceThreshold, 
        "Dollar price too low"
    );

    // update collateral price
    updateChainLinkCollateralPrice(collateralIndex);

    // get amount of collateral for minting Dollars
    collateralNeeded = getDollarInCollateral(collateralIndex, dollarAmount);

    ...SNIP...
}

function getDollarInCollateral(
    uint256 collateralIndex,
    uint256 dollarAmount
) internal view returns (uint256) {
    UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();
    return
        dollarAmount  // @audit: not using dollar usd price for dollarAmount
            .mul(UBIQUITY_POOL_PRICE_PRECISION)
            .div(10 ** poolStorage.missingDecimals[collateralIndex])
            .div(poolStorage.collateralPrices[collateralIndex]); // @audit: returns the usd price of collateral
}

That means, if currently dollar price is at 1.1 usd/uad, an attacker can pay collateral worth 1mil USD to get 1mil uad(= 1mil x 1.1 USD), considering zero fees. Consequently, the attacker can pocket an extra $100k worth of dollar tokens.

Impact

Fund loss.

Code Snippet

Add test to /packages/contracts/test/diamond/facets/UbiquityPoolFacet.t.sol and run forge test --mt test_exploit -vv

 function test_Attack() public { 
    address bob = makeAddr("bob");
    collateralToken.mint(bob, 1_000_000e18);

    // set no fee and poolCeiling high enough for ease
    vm.startPrank(admin);
    ubiquityPoolFacet.setFees(0,0,0);
    ubiquityPoolFacet.setPriceThresholds(1000000,1000000);
    ubiquityPoolFacet.setPoolCeiling(0, 10_000_000e18);
    vm.stopPrank();

    vm.startPrank(bob);
    collateralToken.approve(address(ubiquityPoolFacet), type(uint256).max); 
    (uint256 totalDollarMintBob, ) = ubiquityPoolFacet.mintDollar(
        0,
        1_000_000e18, 
        0, 
        1_000_000e18
    ); 
    assertEq(totalDollarMintBob, 1_000_000e18);

    uint256 dollarPrice = 1.1e6; // can't find a way to set price via twap 

    uint256 attacker_usd_balance = (totalDollarMintBob *  dollarPrice) / 1e6; 
    uint256 collateral_transferred_usd = collateralToken.balanceOf(address(ubiquityPoolFacet));  // consider 1:1 for stablecoins
    assertGt(attacker_usd_balance, collateral_transferred_usd);

    emit log_named_uint("collateral_transferred_usd", collateral_transferred_usd);
    emit log_named_uint("attacker_usd_balance ", attacker_usd_balance);
}

Tool used

Manual Review

Recommendation

Fix it by converting dollarAmount to USD value of dollarAmount using getDollarPriceUsd into the getDollarInCollateral fn, https://github.com/sherlock-audit/2023-12-ubiquity/blob/d9c39e8dfd5601e7e8db2e4b3390e7d8dff42a8e/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L284-L294

[blutorque]

Correction, price deviation should be 0.01$ instead of 0.1$ which I assumed above, as mintPriceThreshold allows deviation upto 0.01$. So practically loss can be around 10k.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

auditsea commented:

The issue describes that required collateral amount is not determined by Ubiquity Dollar price but by $1, I think it's protocol decision and it seems fine to stick Ubiquity Dollar price to $1

[molecula451]

@pavlovcik @rndquu @gitcoindev

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

auditsea commented:

The issue describes that required collateral amount is not determined by Ubiquity Dollar price but by $1, I think it's protocol decision and it seems fine to stick Ubiquity Dollar price to $1

[0x4007]

I'm not sure if I'm fully understanding the vulnerability. However consider this intended mechanism design:

The system is designed to provide financial incentives for arbitrageurs to stabilize the token price.

• Below peg: the arbitrageur should buy cheap dollars (e.g. $0.99/Dollar) and redeem for $1.00 of collateral, making a profit (1% in this scenario.)
• Above peg: (e.g. $1.01) the arbitrageur should mint new dollars, and sell them for more than $1 (e.g. $1.01) making a profit (1% in this scenario.)

Does that address this issue?

[rndquu]

@blutorque

I don't understand the issue.

As far as I understand you provide the following example:

1. Dollar token price is $1.1
2. User provides 1 mln of collateral and gets 1 mln of Dollar tokens
3. User sells 1 mln of Dollar tokens and gets 1.1 mln in USD value

This is expected behaviour since on "step 2" 1 mln of Dollar tokens are minted hence the Dollar token price goes from $1.1 to $1.00.

The test (code snippet) is also not clear. Pls provide a test with malicious behaviour and expected one.

[molecula451]

This only looks like a normal expected arbitrage and does not take in count other scenarios, also the user didn't provide an accurate proof on concept with out current mechanism because:

the user stated a comment in a variable:

uint256 dollarPrice = 1.1e6; // can't find a way to set price via twap

It will be impossible for us to fully determine an issue without providing a correct concept proof within our mechanism in this case fully setting a Price with the TWAP

lastly the profit looks normal due to market change and doesn't show the "EXTRA" tokens aside the profit

say there is no a liquidity disaster

[rndquu]

I've double checked all related issues and I still think the issue is invalid. If we apply the proposed solutions (one, two) then users won't be able to arbitrage on the Dollar token price hence maintaining it's peg.

So basically this issue describes an arbitrage which is an expected behavior.