Skip to content
This repository has been archived by the owner on May 26, 2024. It is now read-only.

xiaoming90 - Incorrect scaling of the spot price #79

Open
sherlock-admin2 opened this issue Nov 25, 2023 · 4 comments
Open

xiaoming90 - Incorrect scaling of the spot price #79

sherlock-admin2 opened this issue Nov 25, 2023 · 4 comments
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability High A valid High severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed

Comments

@sherlock-admin2
Copy link
Contributor

sherlock-admin2 commented Nov 25, 2023

xiaoming90

high

Incorrect scaling of the spot price

Summary

The incorrect scaling of the spot price leads to the incorrect spot price, which is later compared with the oracle price.

If the spot price is incorrect, it might potentially fail to detect the pool has been manipulated or result in unintended reverts due to false positives. In the worst-case scenario, the trade proceeds to execute against the manipulated pool, leading to a loss of assets.

Vulnerability Detail

Per the comment and source code at Lines 97 to 103, the SPOT_PRICE.getComposableSpotPrices is expected to return the spot price in native decimals.

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/BalancerComposableAuraVault.sol#L97

File: BalancerComposableAuraVault.sol
090:     function _checkPriceAndCalculateValue() internal view override returns (uint256) {
091:         (uint256[] memory balances, uint256[] memory spotPrices) = SPOT_PRICE.getComposableSpotPrices(
092:             BALANCER_POOL_ID,
093:             address(BALANCER_POOL_TOKEN),
094:             PRIMARY_INDEX()
095:         );
096: 
097:         // Spot prices are returned in native decimals, convert them all to POOL_PRECISION
098:         // as required in the _calculateLPTokenValue method.
099:         (/* */, uint8[] memory decimals) = TOKENS();
100:         for (uint256 i; i < spotPrices.length; i++) {
101:             spotPrices[i] = spotPrices[i] * POOL_PRECISION() / 10 ** decimals[i];
102:         }
103: 
104:         return _calculateLPTokenValue(balances, spotPrices);
105:     }

Within the getComposableSpotPrices function, it will trigger the _calculateStableMathSpotPrice function. When the primary and secondary balances are passed into the StableMath._calculateInvariant and StableMath._calcSpotPrice functions, they are scaled up to 18 decimals precision as StableMath functions only work with balances that have been normalized to 18 decimals.

Assuming that the following states:

  • Primary Token = USDC (6 decimals)
  • Secondary Token = DAI (18 decimals)
  • Primary Balance = 100 USDC (=100 * 1e6)
  • Secondary Balance = 100 DAI (=100 * 1e18)
  • scalingFactors[USDC] = 1e12 * Fixed.ONE (1e18) = 1e30
  • scalingFactors[DAI] = 1e0 * Fixed.ONE (1e18) = 1e18
  • The price between USDC and DAI is 1:1

After scaling the primary and secondary balances, the scaled balances will be as follows:

scaledPrimary = balances[USDC] * scalingFactors[USDC] / BALANCER_PRECISION
scaledPrimary = 100 * 1e6 * 1e30 / 1e18
scaledPrimary = 100 * 1e18

scaledSecondary = balances[DAI] * scalingFactors[DAI] / BALANCER_PRECISION
scaledSecondary = 100 * 1e18 * 1e18 / 1e18
scaledSecondary = 100 * 1e18

The spot price returned from the StableMath._calcSpotPrice function at Line 93 will be 1e18 (1:1).

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/BalancerSpotPrice.sol#L93

File: BalancerSpotPrice.sol
78:     function _calculateStableMathSpotPrice(
79:         uint256 ampParam,
80:         uint256[] memory scalingFactors,
81:         uint256[] memory balances,
82:         uint256 scaledPrimary,
83:         uint256 primaryIndex,
84:         uint256 index2
85:     ) internal pure returns (uint256 spotPrice) {
86:         // Apply scale factors
87:         uint256 secondary = balances[index2] * scalingFactors[index2] / BALANCER_PRECISION;
88: 
89:         uint256 invariant = StableMath._calculateInvariant(
90:             ampParam, StableMath._balances(scaledPrimary, secondary), true // round up
91:         );
92: 
93:         spotPrice = StableMath._calcSpotPrice(ampParam, invariant, scaledPrimary, secondary);
94: 
95:         // Remove scaling factors from spot price
96:         spotPrice = spotPrice * scalingFactors[primaryIndex] / scalingFactors[index2];
97:     }

Subsequently, in Line 96 above, the code attempts to remove the scaling factor from the spot price (1e18).

spotPrice = spotPrice * scalingFactors[USDC] / scalingFactors[DAI];
spotPrice = 1e18 * 1e30 / 1e18
spotPrice = 1e30
spotPrice = 1e12 * 1e18

The spotPrice[DAI-Secondary] is not denominated in native precision after the scaling. The SPOT_PRICE.getComposableSpotPrices will return the following spot prices:

spotPrice[USDC-Primary] = 0
spotPrice[DAI-Secondary] = 1e12 * 1e18

The returned spot prices will be scaled to POOL_PRECISION (1e18). After the scaling, the spot price remains the same:

spotPrice[DAI-Secondary] = spotPrice[DAI-Secondary] * POOL_PRECISION / DAI_Decimal
spotPrice[DAI-Secondary] = 1e12 * 1e18 * 1e18 / 1e18
spotPrice[DAI-Secondary] = 1e12 * 1e18

The converted spot prices will be passed into the _calculateLPTokenValue function. Within the _calculateLPTokenValue function, the oracle price for DAI<>USDC will be 1e18. From here, the spotPrice[DAI-Secondary] (1e12 * 1e18) is significantly different from the oracle price (1e18), which will cause the pool manipulation check to revert.

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/BalancerComposableAuraVault.sol#L97

File: BalancerComposableAuraVault.sol
090:     function _checkPriceAndCalculateValue() internal view override returns (uint256) {
091:         (uint256[] memory balances, uint256[] memory spotPrices) = SPOT_PRICE.getComposableSpotPrices(
092:             BALANCER_POOL_ID,
093:             address(BALANCER_POOL_TOKEN),
094:             PRIMARY_INDEX()
095:         );
096: 
097:         // Spot prices are returned in native decimals, convert them all to POOL_PRECISION
098:         // as required in the _calculateLPTokenValue method.
099:         (/* */, uint8[] memory decimals) = TOKENS();
100:         for (uint256 i; i < spotPrices.length; i++) {
101:             spotPrices[i] = spotPrices[i] * POOL_PRECISION() / 10 ** decimals[i];
102:         }
103: 
104:         return _calculateLPTokenValue(balances, spotPrices);
105:     }

Impact

The spot price is used to verify if the pool has been manipulated before executing certain key vault actions (e.g. reinvest rewards).

If the spot price is incorrect, it might potentially result in the following:

  • Failure to detect the pool has been manipulated, resulting in the trade to execute against the manipulated pool, leading to a loss of assets.
  • Unintended reverts due to false positives, breaking core functionalities of the protocol that rely on the _checkPriceAndCalculateValue function.

The affected _checkPriceAndCalculateValue function was found to be used within the following functions:

  • reinvestReward - If the _checkPriceAndCalculateValue function is malfunctioning or reverts unexpectedly, the protocol will not be able to reinvest, leading to a loss of value for the vault shareholders.

  • convertStrategyToUnderlying - This function is used by Notional V3 for the purpose of computing the collateral values and the account's health factor. If the _checkPriceAndCalculateValue function reverts unexpectedly due to an incorrect invariant/spot price, many of Notional's core functions will break. In addition, the collateral values and the account's health factor might be inflated if it fails to detect a manipulated pool due to incorrect invariant/spot price, potentially allowing the malicious actors to drain the main protocol.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/BalancerComposableAuraVault.sol#L97

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/BalancerSpotPrice.sol#L93

Tool used

Manual Review

Recommendation

The spot price returned from StableMath._calcSpotPrice is denominated in 1e18 (POOL_PRECISION) since the inputted balances are normalized to 18 decimals. The scaling factors are used to normalize a balance to 18 decimals. By dividing or scaling down the spot price by the scaling factor, the native spot price will be returned.

spotPrice[DAI-Secondary] = spotPrice[DAI-Secondary] * Fixed.ONE / scalingFactors[DAI];
spotPrice = 1e18 * Fixed.ONE / (1e0 * Fixed.ONE)
spotPrice = 1e18 * 1e18 / (1e0 * 1e18)
spotPrice = 1e18
@github-actions github-actions bot added High A valid High severity issue Has Duplicates A valid issue with 1+ other issues describing the same vulnerability labels Nov 27, 2023
@jeffywu jeffywu added the Sponsor Confirmed The sponsor acknowledged this issue is valid label Nov 28, 2023
@jeffywu
Copy link

jeffywu commented Nov 28, 2023

Valid issue

jeffywu added a commit to notional-finance/leveraged-vaults that referenced this issue Nov 28, 2023
@jeffywu
Copy link

jeffywu commented Nov 28, 2023

@jeffywu jeffywu added the Will Fix The sponsor confirmed this issue will be fixed label Nov 28, 2023
@sherlock-admin sherlock-admin changed the title Shallow Peanut Elk - Incorrect scaling of the spot price xiaoming90 - Incorrect scaling of the spot price Dec 4, 2023
@sherlock-admin sherlock-admin added the Reward A payout will be made for this issue label Dec 4, 2023
jeffywu added a commit to notional-finance/leveraged-vaults that referenced this issue Dec 4, 2023
* fix: sherlock-audit/2023-10-notional-judging#79

* fix: initial refactor

* fix: more spot price adjustments

* fix: precision on test comparison

* fix: remove log statements
@MLON33
Copy link

MLON33 commented Dec 12, 2023

@xiaoming9090
Copy link
Collaborator

Fixed in PR 63

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability High A valid High severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

5 participants