hats-finance / Possum-Labs--Portals--0xed8965d49b8aeca763447d56e6da7f4e0506b2d3

GNU General Public License v2.0
0 stars 2 forks source link

Inappropriate exchange ratio when principal is a high notional value token #29

Open hats-bug-reporter[bot] opened 1 year ago

hats-bug-reporter[bot] commented 1 year ago

Github username: @rokinot Twitter username: rokinot Submission hash (on-chain): 0x005679170d765a93977b6b5ff70dda1fe8c3e1cca069d8cbca8722bf5d2368a7 Severity: high

Description: Description\ Tokens like YFI or ETH, though having 18 decimals, have a very high notional value per unit. This is causing the exchange ratio to weigh more on portal energy than PSM. However, the protocol currently doesn't support this scenario and will incur a loss of functionality or possibly a huge monetary loss for depositors.

Attack Scenario\ Picture the following:

Case 1: Exchange ratio is 0\ Consequently, the constantProduct variable will also be zero, reserve1 will be zero, and buyPortalEnergy() will always add zero portal energy to the buyer.

        /// @dev Calculate the amount of portalEnergy received based on the amount of PSM tokens sold
        uint256 amountReceived = (_amountInput * reserve1) / (_amountInput + reserve0);

sellPortalEnergy() now returns reserve0 regardless of the input value, since (_amountInput * reserve0) / (_amountInput + reserve1) will result in(_amountInput * reserve0) / (_amountInput) = reserve0

Case 2: Exchange ratio is 1\ constantProduct is calculated as follows:

        uint256 requiredPortalEnergyLiquidity = fundingBalance * FUNDING_EXCHANGE_RATIO;
        constantProduct = fundingBalance * requiredPortalEnergyLiquidity;

In our case, fundingBalance is 1e6 * 1e18 PSM. The result will now be (1e5 * 1e18) * (1e5 * 1e18) = 1e+46.

Now, when buying or selling tokens, we have both calculations of reserve1 and reserve0. reserve0 will be 1e5 * 1e18 = 1e23. We assume no calls of convert() have happened as they're not relevant to this issue and in order to ease the calculations.

reserve1 = constantProduct / reserve0 = 1e+46 / 1e23 = 1e23. Finally, when getting the amount of PSM received when you buy portal energy, assuming the input amount is 10 PSM worth 100 dollars, we'll earn (1e19 * 1e23) / (1e19 + 1e23) = 9.99e18 portal energy.

If the user chooses to sell all their portal energy, the amount of PSM gained will be (2.4638783e+15 * 1e23) / (2.4638783e+15 + 1e23) = 2.4638783e+15. The USD market value of these many tokens are roughly 2.46 cents. In conclusion, depositors who convert their portal energy into PSM will incur a huge loss.

Possible mitigation suggestion\ Scale FUNDING_EXCHANGE_RATIO to 18 decimals, so for example, if the portal energy to PSM ratio is 1:1, FUNDING_EXCHANGE_RATIO will be 1e18. This way, if portal energy surpasses PSM in value, the aforementioned vulnerability won't happen anymore.

Note that it will also be required to scale down the transfer amounts by 1e18 in order to normalize the values.

To demonstrate, reusing the example shown above, the FUNDING_EXCHANGE_RATIO would be 2.46e14 in order to accomodate the proper energy:PSM ratio, so constantProduct will be 2.46e+60. reserve0 remains the same, reserve1 = constantProduct / reserve0 = 2.46e+37.

When calculating the received amount in selling, add a multiplier of 1e18 in order to compensate for the one in the funding ratio as well as dividing it by 1e18 in the buying function as shown below.

        uint256 amountReceived = (_amountInput * reserve0 * 1e18) / (_amountInput + reserve1); //sell

        uint256 amountReceived = (_amountInput * reserve0) / (1e18*(_amountInput + reserve1)); //buy

Now when a depositor sells their portal energy, the amount of PSM received will be (2.4638783e+15 * 1e23 * 1e18 / (2.4638783e+15 + 2.46e+37)) = 1e19, which is 10 PSM tokens.

PossumLabsCrypto commented 1 year ago

Thank you for the detailed report! Unfortunately, I see a fundamental misunderstanding about the purpose of FUNDING_EXCHANGE_RATIO which makes this finding invalid.

The mentioned immutable is set at deployment of the Portal. It is not calculated based on individual stakes but rather defines the starting price of PE vs. PSM in the internal LP.

Because there is a validity check in the constructor, the value cannot be set to 0 which mitigates issue 1 where constantProduct is zero. There is an alternative case that causes constantProduct to become 0, when the Portal is activated with 0 funding. However, both functions buyPortalEnergy() and sellPortalEnergy() will always fail in this case because reserve0 will be 0 which causes a mathError in the calculation of reserve1. This is intentional because it prevents people from using a portal with faulty configuration. Withdrawals still work normally.

If the exchangeRatio is 1 or any other number, the effect is just that the starting upfront yield APR adjusts. A low value means low APR and higher value means higher APR. The value of FUNDING_EXCHANGE_RATIO must be set at deployment to make economical sense. From there onwards, the free market decides the right exchange rate between PSM and PE via supply and demand.

rokinot commented 1 year ago

Hi, @PossumLabsCrypto, thanks for the reply. I'd like to propose a different point of view for this vulnerability.

Ideally, the market price of PE:PSM will be somewhat above the FUNDING_EXCHANGE_RATIO, depending on how the market perceives the yield. If it's higher, there's an arb, but users will have to sell their PE, which translates into more time they have to wait in order to withdraw. If it's lower, there's also an arb, but a free one, as I explain below.

Given that same 0.000246 starting price ratio in the market but a 1:1 conversion in the contract, an existing user can buy PSM in order to acquire substantially more portal energy, convert it into ERC-20 and sell in the market for a profit. As long as the market price is below the smart contract ratio, there's the arb of buying PSM -> selling for PE in the contract -> selling these on the open market, which decreases the portal energy price. When it reaches the contract's ratio, arbitration is finished.

Now, this ratio is working against depositors because the limited exchange ratio is holding back the yield.

To calculate the APR. At the aforementioned prices and a terminal lock duration of 1 year, the user with 0.01 ETH in stake will earn (1e16 * 365 * 86400 + 1e16 * (365 - 90) * 86400) / 31536000 = 1.75e16 PE in a year, which is 1.75e16 PSM or 17 cents. The PE sale return in USD is maximum here, since if the ratio increases so that the PSM is more valuable than PE and the user really needs to sell his PE without worrying about extending his time lock, it's more profitable to do so through the smart contract, because the open market rates will give him fewer tokens.

At 17 cents a year, this user is earning 0.17% APR on his staked balance. This applies to any other user as well. So now the issue is that the yield on the principal isn't captured by the users if its earning a higher APR. Notice that for assets with lower notional value - consequently ratios that already fit in the current code - the upfront amount they earn by selling PE is significantly higher.

If the proposed fix is implemented, the exchange ratio could be set to values where the PE stays more valuable than the PSM in the smart contract. So for example, if it's set to 0.01:1, his yield is 1.75e16 portal energy, which can be sold for 1.75e18 PSM = 17 USD, so the yield caps at 17% APR instead. The arbitration of buying PSM -> swap for PE in the contract -> sell it in the market doesn't happen here because the LP market price will adjust to somewhere closer to the smart contract ratio.

PossumLabsCrypto commented 1 year ago

I think there are some disconnects, so I´ll address your points individually to help bridge the gap and to inform anyone reading this.

Ideally, the market price of PE:PSM will be somewhat above the FUNDING_EXCHANGE_RATIO, depending on how the market perceives the yield. If it's higher, there's an arb, but users will have to sell their PE, which translates into more time they have to wait in order to withdraw. If it's lower, there's also an arb, but a free one, as I explain below.

Arbitrage in the true sense means risk free profit. This is not the case here unless there is a liquid, fixed-rate lending market of the principal (HLP) which is not the case atm. If that was true, even better, because it would increase the financial efficiency of the Portal.

That being said, if the exchange rate of PE:PSM is high, i.e. upfront yield APR is higher than the expected yield, there is an incentive for investors to lock up their principal to get the higher yield rate upfront directly from the Portal. While this is certainly a dynamic that can be used to attract early capital, it is not the ideal state of the system because it is unsustainable. The Portal can´t consistently pay out more yield than is earned on the backend. Ideally, upfront APR is lower than the expected yield over the lock duration which results in a profit for the Portal. Because of the opportunity value of receiving money upfront, this can be expected as the standard setting in efficient markets. Following financial theory, the present value of expected yield is determined by discounting expected cashflows by the time value of money, i.e. by the rate of expected yield of that staking opportunity or at least by the risk-free rate.

Meanwhile, if the upfront yield APR is far too low and there are staked assets in the Portal, it may be opportune to buy PE because the staked assets generate yield at a higher APR which will inevitably push up the internal LP price via the convert() process. Therefore, it can be expected that the upfront APR will fluctuate within realistic ranges around the expected forward yield of the underlying yield source. Unless something is broken in the underlying yield source, the upfront APR should never reach 0.17%.

Given that same 0.000246 starting price ratio in the market but a 1:1 conversion in the contract, an existing user can buy PSM in order to acquire substantially more portal energy, convert it into ERC-20 and sell in the market for a profit. As long as the market price is below the smart contract ratio, there's the arb of buying PSM -> selling for PE in the contract -> selling these on the open market, which decreases the portal energy price. When it reaches the contract's ratio, arbitration is finished.

There seems to be a disconnect. The Portal is the market as it contains an internal LP. You are right that there could be arbitrage opportunities if PE is minted as ERC20 and traded externally but that is not the default situation at deployment. Because of that arbitrage, prices of the internal LP and external markets will stay closely in sync which is overall positive.

So for example, if it's set to 0.01:1, his yield is 1.75e16 portal energy, which can be sold for 1.75e18 PSM = 17 USD, so the yield caps at 17% APR instead. The arbitration of buying PSM -> swap for PE in the contract -> sell it in the market doesn't happen here because the LP market price will adjust to somewhere closer to the smart contract ratio.

It´s important to note that the ongoing yield rate is not capped by FUNDING_EXCHANGE_RATIO as this is merely the starting price. The real price of the full-range internal LP is calculated ongoingly via the constant product formula using reserves.

It is acknowledged that setting the starting price, i.e. FUNDING_EXCHANGE_RATIO too high or too low can result in a misconfiguration of the Portal that either leads to losses to the Portal (too high APR) or no users at all (too low APR). However, given the wide range of token values as you mentioned and the ever changing price of PSM, there is no way of technically limiting these potential fuck-ups aside from disallowing 0 as starting price.

The current Portal enables the vast majority of starting scenarios and only needs additional adjustment if the price of the principal token is equal or lower than the price of PSM. This is not the case with any current or planned principal tokens even if PSM price rises significantly.