sherlock-audit / 2023-12-dodo-gsp-judging

6 stars 5 forks source link

shealtielanz - All contracts using `DecimalMath.sol` for calculations will be broken for stableTokens like `USDC` that have only `6` Decimals. #129

Closed sherlock-admin2 closed 10 months ago

sherlock-admin2 commented 10 months ago

shealtielanz

high

All contracts using DecimalMath.sol for calculations will be broken for stableTokens like USDC that have only 6 Decimals.

Summary

In the Read.me on the contest page for this contest it is stated that:

Which ERC20 tokens do you expect will interact with the smart contracts?

stablecoin, such as USDC and USDT etc.

However, the DecimalMath Library assumes that all tokens(Base and Quote Tokens) will be 18 decimals thereby causing massive rounding UP/DOWN issues in the amounts, reserves, and shares for Tokens like USDC(which have 6 Token Decimals) when buying and selling.

Vulnerability Detail

https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/main/dodo-gassaving-pool/contracts/lib/DecimalMath.sol#L7C1-L12C4

/**
 * @title DecimalMath
 * @author DODO Breeder
 *
 * @notice Functions for fixed point number with 18 decimals
 */

The snippet above shows it works mainly for fixed point with 18 decimals. Using the GSPFunding::buyShares() as an instance, during the calculation of the shares, it calls DecimalMath.divFloor(), & DecimalMath.mulFloor(), for calculations.

https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/main/dodo-gassaving-pool/contracts/GasSavingPool/impl/GSPFunding.sol#L56C7-L76C10

  function buyShares(address to)
        if (totalSupply == 0) {
            // case 1. initial supply
..SNIP..
            shares = quoteBalance < DecimalMath.mulFloor(baseBalance, _I_)
                ? DecimalMath.divFloor(quoteBalance, _I_)
                : baseBalance;
..SNIP..
            _QUOTE_TARGET_ = uint112(DecimalMath.mulFloor(shares, _I_));
        } else if (baseReserve > 0 && quoteReserve > 0) {
            // case 2. normal case
            uint256 baseInputRatio = DecimalMath.divFloor(baseInput, baseReserve);
            uint256 quoteInputRatio = DecimalMath.divFloor(quoteInput, quoteReserve);
..SNIP..
            _BASE_TARGET_ = uint112(uint256(_BASE_TARGET_) + (DecimalMath.mulFloor(uint256(_BASE_TARGET_), mintRatio)));
            _QUOTE_TARGET_ = uint112(uint256(_QUOTE_TARGET_) + (DecimalMath.mulFloor(uint256(_QUOTE_TARGET_), mintRatio)));
        }

When calling DecimalMath.mulFloor() with tokens with 6 Decimals.

--> 200e6 * 10e6  / 1e18
--> 2000e6 / 1e18
--> 0

Here the value will be under inflated(rounded down far lower than the expected amount). When calling DecimalMath.divFloor() with tokens with 6 Decimals.

--> 200e6 * 10e18  / 1e6
--> 2000e24 / 1e6
--> 2000e18

Here the value is Over Inflated(the value is far bigger than the expected amount).

Impact

This will lead to Incorrect shares minted to users and accounting issues in the contracts for the base and quote tokens..

Code Snippet

Manual Review

Recommendation

use the Tokens decimals for the calculation rather than 1e18 fixed in Decimalmath.sol library.

nevillehuang commented 10 months ago

Invalid, I is the price set by trusted admin used to convert base token balance to amount of equivalent quote balance and vice versa. It is presumed that the appropriate I variable will be set to scale quote balance and base balance appropriately. You can see from this test here where DAI with 18 decimals is the base token and USDC with 6 decimals is the quote token

Additionally, since the only tokens expected to be used is stable coins, the maximum difference in decimals is 18 - 2 = 10. Since stable coins are priced almost identical to each other, I think I being max 36e18 is more than suffiicient to prevent rounding errors and scale decimals appropriately