Sulpiride - Precision loss in getUserQuota

[sherlock-admin]

Sulpiride

high

Precision loss in getUserQuota

Summary

In D3UserQuota contract, there's a precision loss in getUserQuota function that results in incorrect user quota

Vulnerability Detail

In D3UserQuota contract, quotaTokenAmount holds the amount of tokens a user can deposit, depending on the amount of DODO tokens that user has, in terms of USD. But quotaTokenAmount is denominated in integers without fractionals parts and it results in a precision loss in certain scenarios and in incorrect calculation of a user's used quota.

Let's take a real example from the deployed DODOv3 contracts on Goerli:
D3Oracle: https://goerli.etherscan.io/address/0xE4b90C582B9597A4EFF505fa11B8254495b54F9d
DAI token: 0x5e2C68Fd294a28b054565b8D3a764E5cbF8c58D6 (decimals = 18)

D3UserQuota.quotaTokenAmount fetches the price of an asset by calling D3Oracle.getOriginalPrice. Calling this on DAI returns this:
tokenPrice = 99980000 ($0.9998, slighthly less than 1 USD)
priceDecimal = 8

Quota used for a given asset is calculated with this formula: tokenBalance * tokenPrice / 10 ** (priceDecimal+tokenDecimals)

But if the tokenBalance is less than 10 ** tokenDecimals and tokenPrice is less than 10 ** priceDecimal (less than 1 USD), the result of this formula will be 0:
tokenBalance * tokenPrice / 10 ** (priceDecimal+tokenDecimals) = 9e17 * 99980000 / 10 ** (8 + 18) = 0

This may be expected behaviour since a depositor really tried to deposit less than 1 USD worth of asset and trying to repeat such transaction will make the tokenBalance greater than 10 ** tokenDecimals.
However a bug arrises when there are 2 such assets.

POC:

Prepare the environment:

In test\TestContext.t.sol change token decimals to simulate behaviour of DAI in Goerli:

function createTokens() public {
  token1 = new MockERC20("Wrapped BTC", "WBTC", 18);
  token2 = new MockERC20("Token2", "TK2", 18);
  // ...

function createD3Oracle() public {
  oracle = new D3Oracle();
  token1ChainLinkOracle = new MockChainlinkPriceFeed("Token1/USD", 8);
  token2ChainLinkOracle = new MockChainlinkPriceFeed("Token2/USD", 8);
  // ...
  token1ChainLinkOracle.feedData(99980000);
  token2ChainLinkOracle.feedData(99980000);
  // ...
  oracle.setPriceSource(
      address(token1), PriceSource(address(token1ChainLinkOracle), true, 5 * (10 ** 17), 8, 18, 3600)
  );
  oracle.setPriceSource(
      address(token2), PriceSource(address(token2ChainLinkOracle), true, 5 * (10 ** 17), 8, 18, 3600)
  );
  //...
}

function createD3VaultTwo() public {
  // ...
  d3Vault.addNewToken(
    address(token1), // token
    1000 * 1e18, // max deposit
    100 * 1e8, // max collateral
    80 * 1e16, // collateral weight: 80%
    120 * 1e16, // debtWeight: 120%
    20 * 1e16 // reserve factor: 20%
  );

  d3Vault.addNewToken(
    address(token2), // token
    1000 * 1e18, // max deposit
    500 * 1e18, // max collateral
    90 * 1e16, // collateral weight: 90%
    110 * 1e16, // debtWeight: 110%
    10 * 1e16 // reserve factor: 10%
  );
 // ...

POC:

Add this in test\DODOV3MM\D3Vault\periphery\D3UserQuota.t.sol:

function testQuotaTokenUnderflow() public {
  vm.prank(user1);
  token1.approve(address(dodoApprove), type(uint256).max);
  vm.prank(user1);
  token2.approve(address(dodoApprove), type(uint256).max);

  d3UserQuota.enableQuota(address(token1), true);
  d3UserQuota.enableGlobalQuota(address(token1), false);
  d3UserQuota.enableQuota(address(token2), true);
  d3UserQuota.enableGlobalQuota(address(token2), false);

  d3UserQuota.setQuotaTokenHold(address(dodo));

  uint256[] memory _quotaTokenHoldAmount = new uint256[](3);
  _quotaTokenHoldAmount[0] = 100 * 1e18;
  _quotaTokenHoldAmount[1] = 1000 * 1e18;
  _quotaTokenHoldAmount[2] = 10000 * 1e18;
  uint256[] memory _quotaTokenAmount = new uint256[](3);
  _quotaTokenAmount[0] = 100;
  _quotaTokenAmount[1] = 1000;
  _quotaTokenAmount[2] = 10000;
  d3UserQuota.setQuotaTokennAmount(_quotaTokenHoldAmount, _quotaTokenAmount);

  uint256 userQuota = d3UserQuota.getUserQuota(user1, address(token1));
  emit log_named_uint("userQuota", userQuota); // user quota before deposit

  faucetToken(address(token1), user1, 1000 * 1e18);
  userDeposit(user1,address(token1), 9 * 1e17); // Deposit first token
  faucetToken(address(token2), user1, 1000 * 1e18);
  userDeposit(user1, address(token2), 9 * 1e17); // Deposit second token
  uint256 userQuota1 = d3UserQuota.getUserQuota(user1, address(token1));
  uint256 userQuota2 = d3UserQuota.getUserQuota(user1, address(token2));
  emit log_named_uint("userQuota1", userQuota1); // quota of token1 after two deposits
  emit log_named_uint("userQuota2", userQuota2); // quota of token2 after two deposits
}

Output of this function:

userQuota: 100020004000800160032
userQuota1: 100020004000800160032
userQuota2: 100020004000800160032

User quota stayed the same despite that we deposited ~$1.6 worth of assets.

Impact

Users will be able to deposit more than the protocol allows them.

Code Snippet

https://github.com/sherlock-audit/2023-06-dodo/blob/main/new-dodo-v3/contracts/DODOV3MM/D3Vault/periphery/D3UserQuota.sol#L82

https://github.com/sherlock-audit/2023-06-dodo/blob/main/new-dodo-v3/contracts/DODOV3MM/D3Vault/periphery/D3UserQuota.sol#L19-L23

Tool used

Manual Review

Recommendation

Use decimals with fractional parts in quotaTokenAmount and DecimalMath library to calculate used quota of a user

[0xSulpiride]

Escalate for 10 USDC

I believe this issue is not a duplicate of #45 or of any other duplicate of that issue.

#45 refers to the loss of precision in D3VaultLiquidation.finishLiquiditation that can be fixed just by doing multiplication before divison.

However the issue that I described is regarding the design of the quota management in D3UserQuota contract which at best leads to a incorrect quota calculation and at worst allows users to deposit more than they should.

I believe this issue should a unique high

[sherlock-admin2]

Escalate for 10 USDC

I believe this issue is not a duplicate of #45 or of any other duplicate of that issue.

#45 refers to the loss of precision in D3VaultLiquidation.finishLiquiditation that can be fixed just by doing multiplication before divison.

However the issue that I described is regarding the design of the quota management in D3UserQuota contract which at best leads to a incorrect quota calculation and at worst allows users to deposit more than they should.

I believe this issue should a unique high

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

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

[hrishibhat]

@marie-fourier
Considering the impact of this issue low based on additional Sponsor comment:

We don't have such high precision requirements for userquota. Moreover, there were errors in the previous design of userQuota formula, and we have already upgraded it.

[maarcweiss]

It seems like the precision loss is minimal too and the requirements for it are also quite strict But if the tokenBalance is less than 10 ** tokenDecimals and tokenPrice is less than 10 ** priceDecimal (less than 1 USD), the result of this formula will be 0 I agree with low

[hrishibhat]

Result:
Low
Unique
Considering the impact of the issue as low based on the above comments.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• marie-fourier: accepted