[Lesson-25] - Advanced foundry - Redeem collateral suggestion to avoid breaking on health factor

[s3bc40]

Hey 👋

I have been struggling a lot on fuzz testing to understand why my code was breaking when in the course it was not. 99% it was on my side, but after fixing the mintDsc for invariant test in the Handler.t.sol I was getting the same error over and over again.

Here is the mintDsc handler function, just to give some context at what point I did have this error:

function mintDsc(uint256 amount, uint256 addressSeed) public {
        if (usersWithCollateralDeposited.length == 0) {
            return;
        }
        address sender = usersWithCollateralDeposited[addressSeed % usersWithCollateralDeposited.length];
        (uint256 totalDscMinted, uint256 collateralValueInUsd) = dsce.getAccountInformation(sender);
        int256 maxDscToMint = (int256(collateralValueInUsd) / 2) - int256(totalDscMinted);
        if (maxDscToMint < 0) {
            return;
        }

        amount = bound(amount, 0, uint256(maxDscToMint));
        if (amount == 0) {
            return;
        }
        vm.startPrank(sender);
        dsce.mintDsc(amount);
        vm.stopPrank();
        timeMintIsCalled++;
    }

The error: [Revert] DSCEngine__BreaksHealthFactor(702670004378033987 [7.026e17])

  [103634] Handler::redeemCollateral(51540240699854426746100967214 [5.154e28], 1035285307183729588843981678733247299005494427896378025 [1.035e54])
    ├─ [2808] DSCEngine::getCollateralBalanceOfUser(0x00000000000000000000000000000000000375d9, ERC20Mock: [0x90193C961A926261B756D1E5bb255e67ff9498A1]) [staticcall]
    │   └─ ← [Return] 1921311765 [1.921e9]
    ├─ [0] console::log("Bound result", 1889356931 [1.889e9]) [staticcall]
    │   └─ ← [Stop] 
    ├─ [0] VM::startPrank(0x00000000000000000000000000000000000375d9)
    │   └─ ← [Return] 
    ├─ [85785] DSCEngine::redeemCollateral(ERC20Mock: [0x90193C961A926261B756D1E5bb255e67ff9498A1], 1889356931 [1.889e9])
    │   ├─ emit CollateralRedeemed(redeemedFrom: 0x00000000000000000000000000000000000375d9, redeemedTo: 0x00000000000000000000000000000000000375d9, token: ERC20Mock: [0x90193C961A926261B756D1E5bb255e67ff9498A1], amount: 1889356931 [1.889e9])
    │   ├─ [29805] ERC20Mock::transfer(0x00000000000000000000000000000000000375d9, 1889356931 [1.889e9])
    │   │   ├─ emit Transfer(from: DSCEngine: [0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512], to: 0x00000000000000000000000000000000000375d9, value: 1889356931 [1.889e9])
    │   │   └─ ← [Return] true
    │   ├─ [8993] MockV3Aggregator::latestRoundData() [staticcall]
    │   │   └─ ← [Return] 1, 200000000000 [2e11], 1, 1, 1
    │   ├─ [8993] MockV3Aggregator::latestRoundData() [staticcall]
    │   │   └─ ← [Return] 1, 100000000000 [1e11], 1, 1, 1
    │   └─ ← [Revert] DSCEngine__BreaksHealthFactor(702670004378033987 [7.026e17])
    └─ ← [Revert] DSCEngine__BreaksHealthFactor(702670004378033987 [7.026e17])

I have checked all the discussion to see if it was something already fixed or something I have done wrong:

/*===============================================
                     State Variables          
    ===============================================*/
    uint256 private constant ADDITIONAL_FEED_PRECISION = 1e10;
    uint256 private constant PRECISION = 1e18;
    uint256 private constant LIQUIDATION_THRESHOLD = 50; // 200% overcollateralized
    uint256 private constant LIQUIDATION_PRECISION = 100;
    uint256 private constant LIQUIDATION_BONUS = 10; // 10% bonus
    uint256 private constant MIN_HEALTH_FACTOR = 1e18;
    
    [...]
    
        /* 
        1. Check that withdrawing the requested amount doesn't cause the account's `Health Factor` to break (fall below 1)
        2. transfer the requested tokens from the protocol to the user
        DRY. Don't repeat yourself
        Should follow CEI but sometimes it can be gas inefficient
    */
    function redeemCollateral(address tokenCollateralAddress, uint256 amountCollateral)
        public
        moreThanZero(amountCollateral)
        isAllowedToken(tokenCollateralAddress)
        nonReentrant
    {
        _redeemCollateral(msg.sender, msg.sender, tokenCollateralAddress, amountCollateral);
        _revertIfHealthFactorIsBroken(msg.sender);
    }
    
    function _redeemCollateral(address from, address to, address tokenCollateralAddress, uint256 amountCollateral)
        private
    {
        // Checks
        // Effects
        s_collateralDeposited[from][tokenCollateralAddress] -= amountCollateral;
        emit CollateralRedeemed(from, to, tokenCollateralAddress, amountCollateral);
        // Interactions
        bool success = IERC20(tokenCollateralAddress).transfer(to, amountCollateral);
        if (!success) {
            revert DSCEngine__TransferFailed();
        }
    }

Everything seemed fine, even my previous tests (unit or integration) were passing.

A lot of time my BreakHealthFactor was triggered in my fuzz testing, which showed some test reverted (runs: 128, calls: 16384, reverts: 12) but not on the course : so I am curious to understand.

So here is my suggestion, I do not know if it is relevant or not while testing, but I have added a check on the HealthFactor before doing a dsce.redeemCollateral on the bounded amountCollateral:

function redeemCollateral(uint256 collateralSeed, uint256 amountCollateral) public {
        // Compute the max amount of collateral that can be redeemed
        ERC20Mock collateral = _getCollateralFromSeed(collateralSeed);
        uint256 maxCollateralToRedeem = dsce.getCollateralBalanceOfUser(msg.sender, address(collateral));
        amountCollateral = bound(amountCollateral, 0, maxCollateralToRedeem);
        if (amountCollateral == 0) {
            return;
        }
        // Avoid breaking health factor if amount collateral is too high
        (uint256 totalDscMinted,) = dsce.getAccountInformation(msg.sender);
        uint256 healthFactor = dsce.getHealthFactor(totalDscMinted, amountCollateral);
        if (healthFactor < dsce.getMinHealthFactor()) {
            return;
        }
        // Redeem
        vm.startPrank(msg.sender);
        dsce.redeemCollateral(address(collateral), amountCollateral);
        vm.stopPrank();
    }

Now I am reaching the result on the course: (runs: 128, calls: 16384, reverts: 0)

totalSupply:  43140201518346120401720704756233
  wethValue:  612356452108151500454354407764000
  wbtcValue:  284294938700412005897668316124000
  Times Mint Called:  37

Is my solution is considered as cheating since I am checking the health factor before, or does it seem logical ? I am open to understand any suggestion 👍

Thanks for reading!

[EngrPips]

Hello @s3bc40, considering that the revert is coming from your DSCEngine, which is complaining that you are breaking the healthFactor of a user then I must say the contract you are testing is fine, and it is in your invariant test suite where the problem lies so doing that check to prevent the issue in your invariant test suit seem very logical to me.

[s3bc40]

Thanks for your feedback 😄

[EngrPips]

You are welcome.