SecEngBlockchain/warp.md Secret

## warp.md

      
    Raw
  

              warp.md
            
          
    WARP Review

The following report is a result of a security review from n-Var, requested by Warp to audit the changes in the system after the attack. 36 hours were spent in total during that engagement. This is not a full security audit of the system. This review's main focus was targeted on fixes of the hack and changes in price oracles. The review was done in multiple steps with continuous code changes.
Attack Fix

The recent attack was based on the wrong price evaluation of LP tokens. According to the old formula, when the price of tokens in a Uniswap token pair is not correct, LP token price increases. Thus, manipulating the price in one of the Uniswap pair led to the artificial increase of the LP token (collateral) price and allowed to lend more stable coins from the protocol than it should have. That can lead to draining the system's funds.
The attack and the math of a fix for the system are described here: https://cmichel.io/pricing-lp-tokens/. In this review, we double-checked the math in the new formula and the implementation of the fix. No issues were found in that solution.
Round 0

The initial round of the review was performed on a codebase with a following commit hash: 8a384d50be099a5b4f5b6b08a0bdce762f86f8a9. 2.5 days were spent to review the code and help with the fixes.
The following issues were found during that round:


High. The price is not updated in the consult function when called for one of the tokens.
In the consult function, the token price should be updated before it's returned. But it's only happening if the price of the token0 is requested. In the case of token1, the price remains the same.
https://github.com/warpfinance/Warp/blob/8f2fc90dae021532b496d300eaf6cbe3b8674a9e/Smart-Contracts/contracts/SwapLPOracleInstance.sol#L100-L115
Note: That issue was fixed by switching to Chainlink oracles.


High. Creation of the oracle may cause division by zero error.
When an oracle is created, the average price is updated instantly (the update function is called in the constructor). The average price is calculated between the current price and the last updated price in Uniswap (which are the same prices). So the average price is actually going to be the latest. The error occurs if a Uniswap trade happened before in the same block. In that case, the timeElapsed value is going to be zero, and we'll have a division by zero error.
https://github.com/warpfinance/Warp/blob/8f2fc90dae021532b496d300eaf6cbe3b8674a9e/Smart-Contracts/contracts/SwapLPOracleInstance.sol#L84
To fix that issue, the oracle instance should be inactive for the first hour and do the first update call after that period.
Note: That issue was fixed by switching to Chainlink oracles.


High. A lot of duplicated code is present to have view/get functions.
Many functions in the system are duplicated and are present in 2 versions: get and view version. The code is completely the same except for one part. For the get version, the prices of the tokens are updated. The view functions are showing outdated values. In some parts of the system, the outdated price is used, while the price is updated in other parts. That creates inconsistency.
A general recommendation would be to either remove the view part completely or optimize the code, so it's not duplicated. And also make the usage of view/get mechanisms consistent.
Note: The view part of the codebase was removed.


Medium. The aggregation period is not fixed if not updated frequently.
The aggregation period is assumed to be 1 hour, but in practice, it depends on how often the update function is called. So if the price hasn't been updated for a day, it will show an average price for that day.
Note: That issue was fixed by switching to Chainlink oracles.


Medium. Amount==0 is used as a special case in the getPriceOfToken function
When calculating a token price, the amount parameter is passed. When it's zero, the function returns the trice of OneToken(token). This may be misleading when the amount is zero, and the price zero is expected.
https://github.com/warpfinance/Warp/blob/8f2fc90dae021532b496d300eaf6cbe3b8674a9e/Smart-Contracts/contracts/SwapLPOracleFactory.sol#L222-L224
Note: That issue was fixed by removing that feature.


Round 1

After many fixes in the first round, the architecture of the whole oracle was reconsidered again. A decision was made to switch from Uniswap oracles to Chainlink. 8 hours were spent to review these changes and help with the fixes. One Critical issue was found during that review.


High. Decimals are wrong in LP token price calculation
When calculating the LP token price, each token's price in the pair is used (p0, p1) in the formula. Here p0 and p1 correspond to the price of OneToken(token0) and  OneToken(token1) amounts of tokens accordingly.
https://github.com/warpfinance/Warp/blob/ce83a7fbdc02f5c83c27581522aecf708bac5f2a/Smart-Contracts/contracts/SwapLPOracle.sol#L91-L96
These amounts depend on the decimals value of each token. Usually, there are 18 decimals, but for some tokens, it's different. For example, USDC has 6 decimals. On the other hand, the formula expects p0 and p1 to be prices of the same amount of tokens. For example, for the USDC-ETH pair, the price of LP tokens will be calculated wrong.
To fix that, it's recommended to pass the prices of 10^18 amount of each token. In that case, the result will also contain the price of 10^18 LP tokens.
Note: That issue was fixed as recommended.


Centralization. The owner can manipulate the price to drain the funds.
The owner can manually add/edit oracles. That allows setting any price of the collateral token and lend any amount of stable coins. It also allows to set low prices and liquidate other accounts.
Note: That issue was fixed by removing the functions that can add/edit oracles.


Additional recommendations

There are also some findings and recommendations that are not necessarily issues but should be mentioned:

The format of Chainlink return values is not very clear from the official reference. There are some assumptions regarding that format, but it should be thoroughly checked, especially when a new oracle is added.
There's no re-entrancy protection in the system while there are external calls. In the current configuration, all the external calls initially look safe from re-entrancy attacks. If new tokens are added to the system, they may be the cause of re-entrancy.
Gas limitations. During some calls, there are iterations over all stable coins and collateral tokens. For each of them, external calls are happening. If the number of allowed tokens increases, some function calls may run out of gas. Since the liquidate function looks the most expensive, liquidation may become impossible.