`WiseOracleHub.getTokensPriceFromUSD()`skips TWAP computation which permits the use of price even if the difference >`ALLOWED_DIFFERENCE`

hats-bug-reporter[bot] commented 9 months ago

Github username: @@Tri-pathi Twitter username: @0xTripathi Submission hash (on-chain): 0xeebe2b67d0b63b921e9b6c32ecb4d41eb3c6a2e12dbfc2d4b7e613094383eb66 Severity: medium

Description: Description\

WiseOracleHub.getTokensPriceFromUSD()skips TWAP computation which permits the use of price even if the difference >ALLOWED_DIFFERENCE

Attack Scenario\

Attachments

Proof of Concept (PoC) File WiseOracleHub.getTokensPriceFromUSD() Converts USD value of a token into token amount with a current price.
```
function getTokensPriceFromUSD(
    address _tokenAddress,
    uint256 _usdValue
)
    external
    view
    returns (uint256)
{
    return getTokensFromETH(
        _tokenAddress,
        _usdValue
            * 10 ** _decimalsUSD
            / getETHPriceInUSD()
    );
}
```
getTokensFromETH() calls latestResolver() where TWAP logic implemented to check the difference between twap price and chainlink price is with in the ALLOWED_DIFFERENCE

If _tokenAddress=WETH_ADDRESS, latestResolver() skips these checks and return amount directly


    function getTokensFromETH(
        address _tokenAddress,
        uint256 _ethAmount
    ){
        public
        view
        returns (uint256)
    {
        if (_tokenAddress == WETH_ADDRESS) {
            return _ethAmount;
        }

     //////////////////           
    }

AVoiding this check fails the purpose of having 2 oracles

Add the TWAP check for WETH_ADDRESS corresponding to uniswap pool

vm06007 commented 9 months ago

@Tri-pathi based on your suggestion, could you point to ETH/WETH pool on Uniswap?

Tri-pathi commented 9 months ago

why ETH/WETH Pool? Protocol getting ETH/USD(getETHPriceInUSD) price from chainlink feed. As the purpose of 2 Oracles is to check if price difference < ALLOWED_DIFFERENCE . A good fix could be fetch ETH price from any good TVL WETH/stable USD coin pool and revert if difference from chainlink getETHPriceInUSD and TWAP is greater than ALLOWED_DIFFERENCE

This is just a suggestion. There can be better ways to mitigate this issue

Quoting from the Hats docs Medium Severity: These issues represent a greater threat and may include: Vulnerabilities that can cause temporary disruption but do not lead to direct loss of funds or long-term damage. Flaws that require specific conditions or privileges to exploit. Vulnerabilities that impact the user experience but do not compromise the overall security of the protocol

As it directly breaks the 2 Oracle protection and I hope it's not design or intended to use price even if it's out of defined ALLOWED_DIFFERENCE makes it Valid Medium

vm06007 commented 9 months ago

getTokensPriceFromUSD

@Tri-pathi I think you're missing the point, all our feeds and pools/tokens are priced in ETH not USD, hence we use getTokensFromETH function everywhere and in that function we price 1 ETH = 1 WETH, hence we return the value instead of needing to convert to ETH equivalent in that function.

You've asked about Add the TWAP check for WETH_ADDRESS corresponding to uniswap pool so I've replied that there is no such pool and based on WETH contract implementation 1 ETH is always 1 WETH, unless you can prove that WETH contract is corrupt and has a flaw. Thats why there's also no ETH/WETH pool and even Uniswap just routes to deposit/withdraw function on WETH contract when you try to swap these assets.

We don't price our pools in USD nor use that for calculating collateral or borrowed amount when it comes to users, we price everything in ETH. Only incentives are priced in USD, but that is for internal team use to pay or not to pay incentives. TWAP is used for pools only, Since if that de-pegs, it only affects how much USD can be gathered by incentives owner not in their favor. @vonMangoldt / @Foon256 can confirm that.

Tri-pathi commented 9 months ago

Let me be more clear 1) I'm not saying that you are pricing everything in USD and you have problem
2) There is function getTokensPriceInUSD() which returns the token value corresponding to some input amount of usd. So here You are using a chainlink feed to get the price of USD -> ETH and then finding the ETH corresponding to initial token to return the value in token Amount which perfectly fine. (USD -> ETH and token-> ETH). 3) For Token -> ETH we have all the checks that we need in latestResolver() but not for USD->ETH since direct chainlink price is accounted, which is an issue since it allow protocol to use price even if it's out of ALLOWED_DIFFERENCE

4) This function is used in incentive computation which will be affected. Only incentives are priced in USD, but that is for internal team use to pay or not to pay incentives so To invalidate a issue you are saying that we can use use to pay or not to pay incentives. What if next comment is we can use decide to deploy or may be not . Sorry for previous comment but it's so frustrating to see Sponsor wanna invalidate issue with comments that we may or may not use

it only affects how much USD can be gathered by incentives owner not in their favor isn't that a issue? incentives owner are getting less/more than they are expected. If i remember this is part of scope

Adding just A TWAP for USD-ETH token pair can solve all these issue.

Thankyou

Tri-pathi commented 9 months ago

@vm06007 getTokensPriceFromUSD() is used to distribute accumulated fees to incentive, beneficial's and decrease bad debt via claimWiseFees .

    function claimWiseFees(
        address _poolToken
    )
        public
    {
      ///////////
/////////////////////
        uint256 tokenAmount = WISE_LENDING.withdrawExactShares(
            FEE_MANAGER_NFT,
            _poolToken,
            shares
        );

        if (isAaveToken[_poolToken] == true) {

            underlyingTokenAddress = underlyingToken[
                _poolToken
            ];

            tokenAmount = AAVE.withdraw(
                underlyingTokenAddress,
                tokenAmount,
                address(this)
            );
        }

        if (totalBadDebtETH == 0) {

            tokenAmount = _distributeIncentives(
                tokenAmount,
                _poolToken,
                underlyingTokenAddress
            );
        }

        _increaseFeeTokens(
            underlyingTokenAddress,
            tokenAmount
        );

        emit ClaimedFeesWise(
            underlyingTokenAddress,
            tokenAmount,
            block.timestamp
        );
    }

_distributeIncentives() is place where some fees is given as incentive to incentiveOwnerA and incentiveOwnerB

    function _distributeIncentives(
        uint256 _amount,
        address _poolToken,
        address _underlyingToken
    )
        internal
        returns (uint256)
    {

        if (incentiveUSD[incentiveOwnerA] > 0) {

            reduceAmount += _gatherIncentives(
                _poolToken,
                _underlyingToken,
                incentiveOwnerA,
                _amount
            );
        }

        if (incentiveUSD[incentiveOwnerB] > 0) {

            reduceAmount += _gatherIncentives(
                _poolToken,
                _underlyingToken,
                incentiveOwnerB,
                _amount
            );
        }

        return _amount - reduceAmount;
    }

Here reduceAmount is inversely proportional to fees going for beneficial and bad debt things. While reduceAmount could be inflated/deflated since it's coming from USD<->ETH pricefeed which can me much more deviated than ALLOWED_DIFFERENCE [see history of such instances]

In Case of inflated reduceAmount, 1) Even possibilities are very low (since INCENTIVE_PORTION is low) but including pool fee rate such that _amount - reduceAmount reverts due to underflow, reverting overall claimWiseFees() 2) ofc incentive will be inflated for owners .which will increase bad debt due to less fee token(_receivingToken) available if caller wanted to pay for bad debt. which is again bad for owners since in next rounds they won't get any incentive untill totalBadDebtETH == 0

For Deflated reduceAmount

1) Incentive will be less for owners. As most of the time totalBadDebtETH will be no zero, even malicious actors can make totalBadDebtETH to dust and owners won't get incentive so it's crucial to get correct incentive whenever there is opportunity

Having 2 oracles had a purpose to revert if there is such inflated or deflated price which doesn't work in this case. Fix is simple, add a TWAP from any good TVL pool and revert if the difference between ETH price in USD from chainlink and TWAP differs much in the same way it's done for all other feeds

From the docs of HATs it's valid Medium, leaving to judges Now

ThankYou

PS: I understand all the fees accumulated belongs to protocol owners it's just their big heart to give back to people who clear bad debt and to beneficial for bootstrapping . It doesn't mean their should be incorrect accounting. If that's the case why smart contract logics for such distribution? just get all the fees and transfer the whatever share you want to beneficial or in a treasury for bad debt payment.

vm06007 commented 9 months ago

Hello, please refer to OOS, with that said its a centralization topic where FeeManager contract (master) decides to give any incentives or not by default no incentives.

Incentives are not correlated with totalBadDebtETH because it is nominated in ETH.
totalBadDebtETH takes priority and no incentives are paid ever if there's some bad debt

most of the time incentives will always be correct or not assigned, it is unlikely they ever be not correct (very low probability) and is a subject of admin deciding to add them or not hence we've put that out of scope anything related to admin adding malicious tokens or feeds, assigning values etc. We do not plan to add TWAP for that. If it is part of the check then it would be done inside ETH_PRICE_FEED contract directly, which is not in the scope

wrong assumption that ETH_PRICE_FEED is a direct chainLInk feed as admin can pass this as argument when deploying WiseLending, hence it can be a wrapper around chainlink feed with more strict checks including TWAP.

vm06007 commented 9 months ago

@vonMangoldt and @Foon256 can add more intel if needed.

vm06007 commented 9 months ago

Decision was made split reward with #29

hats-finance / Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573

`WiseOracleHub.getTokensPriceFromUSD()`skips TWAP computation which permits the use of price even if the difference >`ALLOWED_DIFFERENCE` #47