Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using an average of 8 periods, each period covering 30 minutes may cause the price to be extremely delayed. Hackers can profit from these delays. And may cause bad debt. #128

Open
code423n4 opened this issue Sep 8, 2022 · 2 comments
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-09-canto/blob/65fbb8b9de22cf8f8f3d742b38b4be41ee35c468/src/Swap/BaseV1-periphery.sol#L524-L593
https://github.com/code-423n4/2022-09-canto/blob/65fbb8b9de22cf8f8f3d742b38b4be41ee35c468/src/Swap/BaseV1-core.sol#L45

Vulnerability details

Impact

Using an average of 8 periods, each period covering 30 minutes may cause the price to be extremely delayed. Hackers can profit from these delays.

In crypto, the price can +100% in just 30 minutes. Imagine volatility of 8 * 30 = 240 minutes = 4 hours. Hackers can profit a lot from these delays. Moreover, if the market crash just like UST, the liquidator can't liquidate assets in time due to extremely delayed price, causing bad debt.

Proof of Concept

uint public periodSize = 1800;

Every 30 minutes one observation will be stored.

...
uint price = IBaseV1Pair(pair).quote(address(token), decimals, 8);
...
        for(uint i; i < 8; ++i) {
            uint token0TVL = assetReserves[i] * (prices[i] / decimals);
            uint token1TVL = unitReserves[i]; // price of the unit asset is always 1
            LpPricesCumulative += (token0TVL + token1TVL) * 1e18 / supply[i];
        }
        uint LpPrice = LpPricesCumulative / 8; // take the average of the cumulative prices 
...

Many functions in BaseV1-periphery use an average of 8 periods = 240 minutes = 4 hours which is ridiculously long.

Recommended Mitigation Steps

Reduce periodSize to 5 minutes

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Sep 8, 2022
code423n4 added a commit that referenced this issue Sep 8, 2022
@nivasan1 nivasan1 added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Sep 9, 2022
@nivasan1
Copy link
Collaborator

nivasan1 commented Sep 9, 2022

The Pricing in the lending market is meant to be as resistant to large swings in volatility as possible to protect suppliers in each cToken market, changing to a shorter period size leaves the possibility of attackers manipulating prices through flash-loans over time. Also the period size can be changed through governance in the case that the community decides to decrease the TWAP.

@0xean
Copy link
Collaborator

0xean commented Sep 12, 2022

This is a design tradeoff on the "correct" time for the TWAP, hard to see it as a critical vulnerability. Downgrading to QA. Warden has no QA report, so this will stand as theirs.

@0xean 0xean added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

3 participants