sherlock-audit / 2023-12-ubiquity-judging

2 stars 2 forks source link

0xchromatin - Precision Loss/Incorrect return value #142

Closed sherlock-admin2 closed 7 months ago

sherlock-admin2 commented 8 months ago

0xchromatin

medium

Precision Loss/Incorrect return value

Summary

This report outlines a vulnerability identified in the "LibUbiquityPool::getDollarInCollateral" function within a smart contract. The issue lies in the return value calculation, which can potentially lead to incorrect financial computations.

Vulnerability Detail

The core of the issue is within the "LibUbiquityPool::getDollarInCollateral" function. It's designed to calculate and return the amount of dollar equivalent for a given amount of collateral, indexed by "LibUbiquityPool::collateralIndex". The vulnerability arises from how the return value is computed, specifically the division operations involving "poolStorage.missingDecimals[collateralIndex]" and "poolStorage.collateralPrices[collateralIndex]". These divisions could lead to imprecise or incorrect dollar value calculations.

Impact

If exploited, this vulnerability can cause significant discrepancies in financial calculations within the contract, potentially leading to incorrect minting of dollar amounts based on given collateral. This can further impact the overall financial integrity of the contract and any connected systems.

Code Snippet

LoC https://github.com/sherlock-audit/2023-12-ubiquity/blob/main/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L284C3-L284C3

PoC


function testGetDollarInCollateral_ShouldReturnCorrrectAmountOfDollarsWhichShouldBeMintedForInputCollateral()
        public
    {
        uint256 dollarAmount = 100e18;

        uint256 amount = ubiquityPoolFacet.getDollarInCollateral(0, 100e18);
        uint256 res = dollarAmount
            .mul(1e6)
            .div(10 ** 0)
            .div(1);
        console.logUint(amount);
        console.logUint(res);
        assertEq(amount, res);
    }

Assumptions:

  1. UBIQUITY_POOL_PRICE_PRECISION = 1e6
  2. Pool index = 0, as adopted from the test
  3. collateralPrice of index 0 = 1

Tool used

Manual Review

Recommendation

A thorough review and revision of the "LibUbiquityPool::getDollarInCollateral" function are recommended to ensure accurate financial computations. This might involve re-evaluating the logic used for division and considering the implications of floating-point operations in the Solidity environment. Additional unit tests and validation methods should also be implemented to ensure the accuracy of calculations, especially under varying conditions.

sherlock-admin2 commented 7 months ago

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

auditsea commented:

Collateral price is represented in 6 decimals, so guaranteed to have no issue

sherlock-admin2 commented 7 months ago

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

auditsea commented:

Collateral price is represented in 6 decimals, so guaranteed to have no issue

nevillehuang commented 7 months ago

Invalid, given price pricision is in 1e6 it is unlikely that stable coin collateral will have price of 1. Additionally, I do not see any precision loss here