Skip to content

Instantly share code, notes, and snippets.

@aviggiano

aviggiano/REVIEW.md Secret

Last active June 22, 2023 21:41
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save aviggiano/68563f33102ee2e48597d35eb023445e to your computer and use it in GitHub Desktop.
Save aviggiano/68563f33102ee2e48597d35eb023445e to your computer and use it in GitHub Desktop.
Download ZIP
Review of raft-fi/contracts v1.0..master
Raw
REVIEW.md

Report

Peer review of Raft Finance contracts performed by Antonio Viggiano on June 21, 2023 and June 22, 2023.

Scope

Review

  • v1.0..master (i.e., 25abf14da8cbb233255ea4641ea098f5f8f04fed..d5ca8febd9b7f33e1a3ad1eff1f2ae1d9dcd5b5f)
  • Files under contracts/

Ignore

Summary

  • [L-01] In PositionManager.sol, the function redeemCollateral should calculate the minimumCollateral parameter as lowTotalDebt.divUp(price), and not lowTotalDebt.divDown(price), as this is the minimum collateral required to completely redeem, so it requires rounding up
  • [L-02] In FlashLoanLiquidator.sol, the contract contains hardcoded constants that could be passed in as constructor or function arguments, such as the address for the IPoolAddressesProvider and the TARGET_CR. In particular, the hardcoded TARGET_CR means the liquidator will not optimize their returns, as the flashloaned collateral might be more than what's necessary to pay the position's debt (higher premium).
  • [L-03] In PositionManagerWETH.sol, mistakenly msg.value sent to contract will be trapped if isCollateralIncrease && collateralChange > 0 check does not pass (e.g. if collateralChange is zero but msg.value is not zero). As a recommendation, the msg.value check could be moved to the beginning of the function.
  • [L-04]
  • [L-05] In PositionManagerWrappedCollateralToken.sol, users can sweep excess wrappedCollateralToken trapped there by mistake due to redeemCollateral transfering the whole contract's balance. A possible solution would be to have IPositionManager.redeemCollateral return the actual collateral redeemed to the caller.
  • [L-06] In PositionManagerWrappedCollateralToken.sol, there are unchecked return values of _underlyingCollateralToken.transferFrom (use SafeERC20.safeTransferFrom or manually validate collateral token contracts to guarantee that they revert on failure)
  • [I-01] In PositionManagerStETH.sol and OneStepLeverageStETH.sol, the information emitted in events might not be sufficient for off-chain monitoring systems. The events StETHPositionChanged and StETHLeveragedPositionChange do not take into account the actual values changed returned by the PositionManager.
  • [I-02] In PositionManager.sol, there is a redundant comparison to trigger TotalCollateralCannotBeLowerThanMinCollateral, since x == 0 || x < POSITIVE can be simplified by x < POSITIVE
  • In SplitLiquidationCollateral.sol, [I-03] collateralToSentToLiquidator now returns totalCollateral, which effectively sets the liquidation reward as 100%. Further analysis (see here) should be performed in order to properly assess how this change will impact the protocol cryptoeconomic incentives, comparing it to other protocols such as AAVE or Compound.
  • [I-04] In FlashLoanLiquidator.sol, there is an unnecessary rToken.approve to own token address on constructor: this is only required when performing a R token flashmint, such as in FlashMintLiquidator or OneStepLeverage
  • [I-05] In WrappedCollateralToken, there is missing input validation on setter functions
  • [U-01] Unclear motivation behind the TotalDebtCannotBeLowerThanMinDebt error - After creating a unit test for the hyphothetical scenario where a liquidation would leave the protocol with collateral below the minimum debt value, I was not able to validate this case, as liquidate already prevents the last position from being liquidated, which serves a similar purpose as TotalCollateralCannotBeLowerThanMinCollateral
  • [U-02]
  • [U-03] In ChainlinkPriceOracleRETH.sol, calculating the price A/C through A/B and B/C require some additional considerations. In particular, taking into account the difference in price feed decimals (rETH/ETH 18 decimals vs ETH/USD 8 decimals) (_formatPrice already does that). The difference in deviation between rETH/ETH (2%) and ETH/USD (0.5%). The difference in heartbeat/timeout between rETH/ETH (24 hours) and ETH/USD (1 hour). The impact of the deviation of ETH/USD on the final rETH/USD price (it should "compound", i.e., deviation(A/C) = deviation(A,B) * deviation(B,C))). The fact that Tellor does not seem to have a rETH/ETH or rETH/USD price feed, so the PriceFeed should be deployed with the same address for both primary and secondary.
  • [U-04] In ERC20Indexable.sol, it is unclear if mint and burn should both round up for the Raft debt token and collateral token. Generally speaking, when considering rounding errors, user's losses should be always be rounded up and gains always rounded down (see "Keep the change" rule of thumb by Runtime Verification). This means the debt token should always round up on mint and down on burn. The collateral token should have the opposite logic: it should round down on mint, and up on burn. A further research could be carried out to analyze the impact of these rounding errors in the context of the protocol.
Severity Description
H High
M Medium
L Low
I Informational
U Undefined
N/A Not Applicable

Methodology

  • Review changes v1.0..master commit by commit
  • Review full diff side by side
  • Review codebase as a whole at the updated version

Commit changes review

Commit hash Description Findings
4ac809e Add Certora Prover rules [N/A] Not in scope
64d170d Add events to PositionManagerStETH.sol [I-01] Information in StETHPositionChanged event might not be sufficient for off-chain monitoring systems, as stETHCollateralChange is converted to wstETHCollateralChange, which is the actual value used by IPositionManager to change the position
bd2bd4f Add new error to disallow redeeming collateral in a way that the system would be left with a total collateral value below the minimum debt value [I-02] Redundant comparison to trigger TotalCollateralCannotBeLowerThanMinCollateral, since `x == 0
18d79fb Add events to OneStepLeverageStETH.sol [I-01] Information in StETHLeveragedPositionChange might not be sufficient for off-chain monitoring systems, as stETHCollateralChange is converted to wstETHCollateralChange, which is the actual value used by _manageLeveragedPosition to change the leveraged position
28f7a67 Change SplitLiquidationCollateral calculation for collateral given to liquidator. The documentation has been updated to match the code implementation. [I-03] collateralToSentToLiquidator now returns totalCollateral, which effectively sets the liquidation reward as 100%. Further analysis (see here) should be performed in order to properly assess how this change will impact the protocol cryptoeconomic incentives, comparing it to other protocols such as AAVE or Compound.
5f60502 Generalize flash liquidations interface. Implement new FlashLoanLiquidator interface that utilizes AAVE for flash loans. [N/A] Not in scope
[L-02] FlashLoanLiquidator contains hardcoded constants that could be passed in as constructor or function arguments, such as the address for the IPoolAddressesProvider and the TARGET_CR. In particular, the hardcoded TARGET_CR means the liquidator will not optimize their returns, as the flashloaned collateral might be more than what's necessary to pay the position's debt (higher premium).
[I-04] Unnecessary rToken.approve to own token address on FlashLoanLiquidator constructor: this is only required when performing a R-token flashloan, such as in FlashMintLiquidator or OneStepLeverage
93669c1 Add support for 1inch Router v5 Balancer [N/A] Not in scope
226ea50 Add WETH position manager [M-01] managePositionETH does not use return values actualCollateralChange and actualDebtChange of managePosition
[L-03] Mistakenly msg.value sent to contract will be trapped if isCollateralIncrease && collateralChange > 0 check does not pass (e.g. if collateralChange is zero but msg.value is not zero)
[U-02] Verify that adding WETH as collateral does not create any issues with the "phantom permit" in PositionManager (either through phishing or user mistake, it is possible to sign a message with WETH as permitSugnature.token and collateralChange equals 0, making PermitHelper.applyPermit no-op and the debt increase succeed, but neither the user or the attacker should benefit from this)
fdcaad5 Refactor oracle contracts. Functions moved around and contracts generalized [N/A]
a35ad63 Add WETH oracle fetching data from ETH/USD data feeds. Code generalized [N/A]
7cac57e Refactor oracle contracts to use immutable instead of constant variables for query ID. Functions moved around [N/A]
308568b Fix PositionManagerWETH to use return values from managePosition [M-01] Fixed
5a60456 Add wrapped collateral token contract [L-04] Token transfers can exceed a single user's max balance
[I-05] Missing input validation on setter functions of WrappedCollateralToken
85464a8 Add position manager for wrapped collateral token. Logic derived from existing position managers [N/A]
cb346f2 Refactor oracle contracts. Functions moved around [N/A]
6930cae Add redeemCollateral in PositionManagerWrappedCollateralToken [L-05] Users can sweep excess wrappedCollateralToken in PositionManagerWrappedCollateralToken trapped there by mistake due to redeemCollateral transfering the whole contract's balance. A possible solution would be to have IPositionManager.redeemCollateral return the actual collateral redeemed to the caller.
[L-06] Unchecked return value of _underlyingCollateralToken.transferFrom (use SafeERC20.safeTransferFrom or manually validate collateral token contracts to guarantee that they revert on failure)
a8d1446 Change oracle timeout from constant to immutable. Functions moved around [N/A]
7b8e8bc Add rETH oracles [U-03] Calculating price A/C through A/B and B/C require some additional considerations. In particular, taking into account the difference in price feed decimals (rETH/ETH 18 decimals vs ETH/USD 8 decimals) (_formatPrice already does that). The difference in deviation between rETH/ETH (2%) and ETH/USD (0.5%). The difference in heartbeat/timeout between rETH/ETH (24 hours) and ETH/USD (1 hour). The impact of the deviation (ignored here) of ETH/USD affects the rETH/USD price (it should "compound", i.e., deviation(A/C) = deviation(A,B) * deviation(B,C))). The fact that Tellor does not seem to have a rETH/ETH or rETH/USD price feed, so the PriceFeed should be deployed with the same address for both primary and secondary.
4441ce8 Rename files [N/A]
0704ff8 Support wrapped collateral position manager for WETH [N/A]
7e35630 Add error when WETH collateral change amount does not match msg.value [L-03] Not fixed. msg.value check should be moved to the beginning of the function.
805bc2c Support wrapped collateral position manager for stETH [N/A]
96affd4 Refactor WrappedCollateralToken balance checks [L-04] Fixed
d5ca8fe Add tests [N/A]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment