Squilliam - Incorrect DAI Transfer in `LockstakeClipper` will Drain Funds from Keepers

[sherlock-admin2]

Squilliam

High

Incorrect DAI Transfer in LockstakeClipper will Drain Funds from Keepers

Summary

Incorrect Dai transfer in the LockstakeClipper::take function will cause a loss of funds for keepers as the system will transfer Dai from the keeper instead of from the Vat during liquidations.

Root Cause

The root cause of this vulnerability lies in the take function of the LockstakeClipper contract. Specifically, there is an incorrect implementation of the Dai transfer mechanism during the liquidation process.

The specific line causing the vulnerability is:

vat.move(msg.sender, vow, owe);

This line is instructing the Vat (the core accounting system of MakerDAO) to move Dai from msg.sender (the keeper/liquidator) to the vow (the system surplus). This is incorrect because:

1. It's taking Dai from the keeper instead of rewarding them for participating in the liquidation.
2. It's not properly accounting for the Dai that should be coming from the liquidated vault.

The correct implementation should move Dai from the Vat itself (represented by address(this) in the context of the LockstakeClipper) to the vow. This would properly account for the Dai being recovered from the liquidated position without penalizing the keeper.

This error fundamentally misunderstands the flow of funds in a liquidation event. Instead of the system (Vat) paying off the debt and incentivizing the keeper, it's incorrectly taking funds from the keeper, which completely inverts the intended economic model of the liquidation system.

This can be found here: https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/lockstake/src/LockstakeClipper.sol?=plain#L405

Internal pre-conditions

1. A vault needs to become unsafe, allowing it to be liquidated
2. A keeper needs to participate in the liquidation by calling the take() function

External pre-conditions

None

Attack Path

1. A vault becomes unsafe due to price fluctuations or other factors
2. The dog.bark() function is called, initiating a liquidation auction
3. A keeper calls the LockstakeClipper.take() function to participate in the auction
4. The take() function calculates the amount of Dai to be transferred (owe)
5. Instead of transferring Dai from the Vat to the vow, the function transfers Dai from the keeper to the vow
6. The keeper loses the amount of Dai equal to owe, instead of receiving incentives for participation
7. The system's accounting becomes imbalanced, as debt is cleared without proper Dai movement

Impact

Immediate Financial Losses for Keepers: Keepers (liquidators) who participate in auctions will suffer direct financial losses instead of earning rewards. For example, if a keeper participates in the liquidation of a 100,000 DAI debt position, they could lose the amount of DAI they bid in the auction. This could range from a few thousand DAI to potentially the full auction amount, depending on their bidding strategy and the auction parameters. This could lead to substantial losses for active keepers, potentially in the millions of DAI if multiple large liquidations occur before the issue is detected.

Breakdown of the Liquidation Mechanism: Once keepers realize they're losing money, they will stop participating in liquidations. This will leave the system unable to process bad debt, leading to an accumulation of undercollateralized positions. If left uncorrected, this vulnerability could severely compromise the protocol's liquidation mechanism, which is crucial for maintaining system health. Over time, this could indirectly challenge the protocol's ability to maintain the DAI peg and overall stability.

Systemic Imbalance in the Maker Protocol: The incorrect movement of DAI will cause a mismatch between the protocol's debt and its collateral backing. This vulnerability could lead to a breakdown of the liquidation mechanism as keepers would be disincentivized from participating. Over time, if left uncorrected, this could result in an accumulation of undercollateralized positions in the system, potentially putting pressure on the DAI peg.

PoC

Create a new test file and add the following code:

// SPDX-License-Identifier: AGPL-3.0-or-later

pragma solidity ^0.8.21;

import "dss-test/DssTest.sol";

import {LockstakeClipper} from "src/LockstakeClipper.sol";
import {LockstakeEngine} from "src/LockstakeEngine.sol";
import {PipMock} from "test/mocks/PipMock.sol";
import {StairstepExponentialDecreaseAbstract} from
    "../lib/token-tests/lib/dss-test/lib/dss-interfaces/src/dss/StairstepExponentialDecreaseAbstract.sol";
import {GemMock} from "./mocks/GemMock.sol";
import {NstJoinMock} from "./mocks/NstJoinMock.sol";
import {MkrNgtMock} from "./mocks/MkrNgtMock.sol";

contract RedoGuy {
    LockstakeClipper clip;

    constructor(LockstakeClipper clip_) {
        clip = clip_;
    }

    function clipperCall(address sender, uint256 owe, uint256 slice, bytes calldata data) external {
        owe;
        slice;
        data;
        clip.redo(1, sender);
    }
}

contract KickGuy {
    LockstakeClipper clip;

    constructor(LockstakeClipper clip_) {
        clip = clip_;
    }

    function clipperCall(address sender, uint256 owe, uint256 slice, bytes calldata data) external {
        sender;
        owe;
        slice;
        data;
        clip.kick(1, 1, address(0), address(0));
    }
}

contract FileUintGuy {
    LockstakeClipper clip;

    constructor(LockstakeClipper clip_) {
        clip = clip_;
    }

    function clipperCall(address sender, uint256 owe, uint256 slice, bytes calldata data) external {
        sender;
        owe;
        slice;
        data;
        clip.file("stopped", 1);
    }
}

contract FileAddrGuy {
    LockstakeClipper clip;

    constructor(LockstakeClipper clip_) {
        clip = clip_;
    }

    function clipperCall(address sender, uint256 owe, uint256 slice, bytes calldata data) external {
        sender;
        owe;
        slice;
        data;
        clip.file("vow", address(123));
    }
}

contract YankGuy {
    LockstakeClipper clip;

    constructor(LockstakeClipper clip_) {
        clip = clip_;
    }

    function clipperCall(address sender, uint256 owe, uint256 slice, bytes calldata data) external {
        sender;
        owe;
        slice;
        data;
        clip.yank(1);
    }
}

contract PublicClip is LockstakeClipper {
    constructor(address vat, address spot, address dog, address engine) LockstakeClipper(vat, spot, dog, engine) {}

    function add() public returns (uint256 id) {
        id = ++kicks;
        active.push(id);
        sales[id].pos = active.length - 1;
    }

    function remove(uint256 id) public {
        _remove(id);
    }
}

interface VatLike {
    function dai(address) external view returns (uint256);
    function gem(bytes32, address) external view returns (uint256);
    function ilks(bytes32) external view returns (uint256, uint256, uint256, uint256, uint256);
    function urns(bytes32, address) external view returns (uint256, uint256);
    function rely(address) external;
    function file(bytes32, bytes32, uint256) external;
    function init(bytes32) external;
    function hope(address) external;
    function frob(bytes32, address, address, address, int256, int256) external;
    function slip(bytes32, address, int256) external;
    function suck(address, address, uint256) external;
    function fold(bytes32, address, int256) external;
}

interface GemLike {
    function balanceOf(address) external view returns (uint256);
    function approve(address, uint256) external;
    function transfer(address, uint256) external returns (bool);
    function transferFrom(address, address, uint256) external returns (bool);
    function mint(address, uint256) external;
    function burn(address, uint256) external;
}

interface DogLike {
    function Dirt() external view returns (uint256);
    function chop(bytes32) external view returns (uint256);
    function ilks(bytes32) external view returns (address, uint256, uint256, uint256);
    function rely(address) external;
    function file(bytes32, uint256) external;
    function file(bytes32, bytes32, address) external;
    function file(bytes32, bytes32, uint256) external;
    function bark(bytes32, address, address) external returns (uint256);
}

interface SpotterLike {
    function file(bytes32, bytes32, address) external;
    function file(bytes32, bytes32, uint256) external;
    function poke(bytes32) external;
}

interface CalcFabLike {
    function newLinearDecrease(address) external returns (address);
    function newStairstepExponentialDecrease(address) external returns (address);
}

interface CalcLike {
    function file(bytes32, uint256) external;
}

interface VowLike {}

contract JugMock {
    uint256 constant RAY = 10 ** 27;

    function drip(bytes32) external returns (uint256) {
        return RAY; // Return a constant rate for simplicity
    }
}

contract LockstakeClipperTest is DssTest {
    using stdStorage for StdStorage;

    GemLike public mkr;
    GemLike public dai;

    DssInstance dss;
    address pauseProxy;
    PipMock pip;
    StairstepExponentialDecreaseAbstract calc;

    LockstakeEngine engine;
    LockstakeClipper clip;

    address constant LOG = 0xdA0Ab1e0017DEbCd72Be8599041a2aa3bA7e740F;

    address ali;
    address bob;
    address che;

    bytes32 constant ilk = "LSE";
    uint256 constant price = 5 ether;

    uint256 constant startTime = 604411200; // Used to avoid issues with `block.timestamp`

    event LogUint(string name, uint256 value);
    event LogInt(string name, int256 value);
    event LogAddress(string name, address value);

    function setUp() public {
        console.log("Starting setUp...");
        vm.createSelectFork(vm.envString("ETH_RPC_URL"));
        vm.warp(startTime);

        console.log("Loading DssInstance...");
        dss = MCD.loadFromChainlog(LOG);

        pauseProxy = dss.chainlog.getAddress("MCD_PAUSE_PROXY");
        dai = GemLike(dss.chainlog.getAddress("MCD_DAI"));

        console.log("Deploying PipMock...");
        pip = new PipMock();
        pip.setPrice(price);

        console.log("Creating mock tokens...");
        uint256 initialLsMkrSupply = 1000000 * 10 ** 18;
        GemMock mockLsmkr = new GemMock(initialLsMkrSupply);
        GemMock ngtMock = new GemMock(0); // Initial supply for NGT mock
        GemMock mkrMock = new GemMock(1000000 * 10 ** 18); // Initial supply for MKR mock
        MkrNgtMock mkrNgtMock = new MkrNgtMock(address(mkrMock), address(ngtMock), 24000);

        // Replace the real MKR address with our mock
        vm.etch(dss.chainlog.getAddress("MCD_GOV"), address(mkrMock).code);
        mkr = GemLike(dss.chainlog.getAddress("MCD_GOV"));

        console.log("Starting prank as pauseProxy...");
        vm.startPrank(pauseProxy);

        bytes32 ilk = "LSE";
        console.log("Checking and initializing ilk if necessary...");
        (,,,, uint256 dust) = dss.vat.ilks(ilk);
        if (dust == 0) {
            dss.vat.init(ilk);
            console.log("Ilk initialized");
        } else {
            console.log("Ilk already initialized, skipping initialization");
        }

        console.log("Setting up Spotter...");
        dss.spotter.file(ilk, "pip", address(pip));
        dss.spotter.file(ilk, "mat", ray(1.5 ether));
        dss.spotter.poke(ilk);

        console.log("Setting up Vat...");
        dss.vat.file(ilk, "dust", rad(20 ether));
        dss.vat.file(ilk, "line", rad(10000 ether));
        dss.vat.file("Line", dss.vat.Line() + rad(10000 ether));

        console.log("Setting up Dog...");
        dss.dog.file(ilk, "chop", 1.1 ether);
        dss.dog.file(ilk, "hole", rad(1000 ether));
        dss.dog.file("Hole", dss.dog.Dirt() + rad(1000 ether));

        console.log("Deploying NstJoinMock...");
        GemMock nstMock = new GemMock(1000000 * 10 ** 18); // Initial supply for NST mock
        NstJoinMock nstJoinMock = new NstJoinMock(address(dss.vat), address(nstMock));

        console.log("Deploying JugMock...");
        JugMock jugMock = new JugMock();

        console.log("Deploying LockstakeEngine...");
        engine = new LockstakeEngine(
            address(dss.chainlog.getAddress("MCD_VOW")), // Use the actual VoteDelegate factory address
            address(nstJoinMock),
            ilk,
            address(mkrNgtMock),
            address(mockLsmkr),
            0 // fee parameter
        );

        engine.file("jug", address(jugMock));

        // Set up realistic permissions
        dss.vat.rely(address(engine));
        dss.vat.rely(address(nstJoinMock));
        dss.vat.hope(address(engine));
        dss.vat.hope(address(nstJoinMock));
        engine.rely(address(this));
        nstJoinMock.rely(address(engine));

        vm.stopPrank();

        // Allow the engine to move its own Dai
        vm.prank(address(engine));
        dss.vat.hope(address(nstJoinMock));

        // Allow LockstakeEngine to modify the Vat balance of the test contract
        vm.prank(address(this));
        dss.vat.hope(address(engine));

        console.log("Deploying LockstakeClipper...");
        vm.prank(pauseProxy);
        clip = new LockstakeClipper(address(dss.vat), address(dss.spotter), address(dss.dog), address(engine));

        // Authorize the LockstakeClipper after deployment
        vm.prank(address(this));
        engine.rely(address(clip));

        vm.startPrank(pauseProxy);
        clip.upchost();
        clip.rely(address(dss.dog));
        clip.rely(address(this));
        vm.stopPrank();

        console.log("Setting up mock StairstepExponentialDecrease...");
        address calcAddr = address(uint160(uint256(keccak256("StairstepExponentialDecrease"))));
        vm.etch(calcAddr, hex"00");
        calc = StairstepExponentialDecreaseAbstract(calcAddr);
        vm.mockCall(address(calc), abi.encodeWithSelector(calc.price.selector), abi.encode(RAY));

        console.log("Configuring LockstakeClipper...");
        vm.startPrank(pauseProxy);
        clip.file("calc", address(calc));
        clip.file("buf", RAY + (RAY / 4));
        clip.file("tail", 3600);
        clip.file("cusp", (3 * RAY) / 10);
        vm.stopPrank();

        console.log("Final setup steps...");
        vm.startPrank(pauseProxy);
        dss.vat.rely(address(this));
        dss.dog.rely(address(this));
        dss.dog.file(ilk, "clip", address(clip));
        dss.dog.rely(address(clip));
        dss.vat.rely(address(clip));
        vm.stopPrank();

        // Additional permissions
        dss.vat.hope(address(clip));
        dss.vat.hope(address(this));

        vm.prank(pauseProxy);
        dss.spotter.rely(address(this));

        console.log("Minting Dai...");
        vm.prank(pauseProxy);
        dss.vat.suck(address(0), address(this), rad(10000 ether));

        console.log("Setting unsafe conditions...");
        pip.setPrice(4 ether);
        dss.spotter.poke(ilk);

        console.log("Setting up test accounts...");
        ali = address(111);
        bob = address(222);
        che = address(333);

        dss.vat.hope(address(clip));
        vm.prank(ali);
        dss.vat.hope(address(clip));
        vm.prank(bob);
        dss.vat.hope(address(clip));

        console.log("Minting additional Dai for test accounts...");
        vm.startPrank(pauseProxy);
        dss.vat.suck(address(0), address(ali), rad(1000 ether));
        dss.vat.suck(address(0), address(bob), rad(1000 ether));
        vm.stopPrank();

        console.log("Final authorization steps...");
        dss.vat.rely(address(clip));
        dss.vat.hope(address(clip));
        clip.rely(address(this));
        dss.vat.rely(address(this));

        clip.file("vow", address(dss.vow));

        vm.prank(address(0x123));
        dss.vat.hope(address(engine));

        dss.vat.rely(address(dss.vow));
        dss.vat.rely(address(engine));
        dss.vat.hope(address(this));

        // Approve mockLsmkr for the engine
        mockLsmkr.approve(address(engine), type(uint256).max);

        // Allocate MKR to the test contract
        GemMock(address(mkr)).mint(address(this), 1000 ether);

        // Approve engine to transfer MKR from the test contract
        mkr.approve(address(engine), type(uint256).max);

        // Verify setup
        require(dss.vat.wards(pauseProxy) == 1, "PauseProxy not authorized in Vat");
        require(dss.vat.wards(address(this)) == 1, "Test contract not authorized in Vat");
        require(dss.vat.wards(address(engine)) == 1, "LockstakeEngine not authorized in Vat");
        (,,,, dust) = dss.vat.ilks(ilk);
        require(dust > 0, "Ilk not initialized in Vat");

        console.log("Vow address:", clip.vow());
        console.log("setUp completed successfully");
    }

    function ray(uint256 wad) internal pure returns (uint256) {
        return wad * 10 ** 9;
    }

    function rad(uint256 wad) internal pure returns (uint256) {
        return wad * 10 ** 27;
    }

    function testArithmeticInKickAndTake() public {
        // Initial setup
        uint256 initialCollateral = 100 ether;
        uint256 initialDebt = 75 ether;

        // Set liquidation ratio (mat) to 150%
        vm.prank(pauseProxy);
        dss.spotter.file(ilk, "mat", ray(1.5 ether));

        // Ensure proper permissions
        vm.startPrank(pauseProxy);
        dss.vat.rely(address(engine));
        engine.rely(address(this));
        dss.vat.rely(address(this));
        vm.stopPrank();

        // Setup the vault
        GemLike mkrToken = GemLike(address(engine.mkr()));
        mkrToken.approve(address(engine), initialCollateral);
        uint256 urnIndex = engine.usrAmts(address(this));
        address urn = engine.open(urnIndex);
        engine.lock(urn, initialCollateral, 0);
        engine.draw(urn, address(this), initialDebt);

        // Set price to make position unsafe
        pip.setPrice(1 ether);
        dss.spotter.poke(ilk);

        // Log initial states
        uint256 initialDaiVow = dss.vat.dai(address(dss.vow));
        uint256 initialDaiKeeper = dss.vat.dai(address(this));
        console.log("Initial Dai in Vow:", initialDaiVow);
        console.log("Initial Dai in Keeper:", initialDaiKeeper);

        // Trigger liquidation
        uint256 kickId = dss.dog.bark(ilk, urn, address(this));

        // Take the auction
        vm.warp(block.timestamp + 1 hours);
        (,, uint256 lot,,,, uint256 top) = clip.sales(kickId);
        uint256 maxPrice = top * 2;
        clip.take(kickId, lot, maxPrice, address(this), "");

        // Log final states
        uint256 finalDaiVow = dss.vat.dai(address(dss.vow));
        uint256 finalDaiKeeper = dss.vat.dai(address(this));
        console.log("Final Dai in Vow:", finalDaiVow);
        console.log("Final Dai in Keeper:", finalDaiKeeper);

        (,, uint256 remainingLot,,,,) = clip.sales(kickId);
        console.log("Remaining lot:", remainingLot);

        uint256 vowDaiChange = finalDaiVow - initialDaiVow;
        int256 keeperDaiChange = int256(finalDaiKeeper) - int256(initialDaiKeeper);

        // Check that most of the debt went to the Vow
        assertGe(vowDaiChange, initialDebt * RAY * 90 / 100, "Vow should receive most of the debt");

        // Check that the auction was completed
        assertLe(remainingLot, initialCollateral * 5 / 100, "Most collateral should be liquidated");

        assertTrue(keeperDaiChange < 0, "Vulnerability: Keeper loses funds from take");

        // Log the keeper's loss
        console.log("Keeper's loss:", uint256(-keeperDaiChange));

        assertGt(uint256(-keeperDaiChange), 0, "Keeper's loss should be greater than zero");
    }

    function testKeeperLossesDuringLiquidation() public {
        // Initial setup
        uint256 initialCollateral = 100 ether;
        uint256 initialDebt = 75 ether;

        // Set up the vault
        vm.startPrank(address(this));
        uint256 urnIndex = engine.usrAmts(address(this));
        address urn = engine.open(urnIndex);
        GemLike mkrToken = GemLike(address(engine.mkr()));
        mkrToken.approve(address(engine), initialCollateral);
        engine.lock(urn, initialCollateral, 0);
        engine.draw(urn, address(this), initialDebt);
        vm.stopPrank();

        // Make the position unsafe
        pip.setPrice(1 ether); // Set a low price to make the position unsafe
        dss.spotter.poke(ilk);

        // Record initial balances
        uint256 initialKeeperBalance = dss.vat.dai(address(this));
        uint256 initialVowBalance = dss.vat.dai(address(dss.vow));

        // Trigger liquidation
        uint256 kickId = dss.dog.bark(ilk, urn, address(this));

        // Warp time to allow for liquidation
        vm.warp(block.timestamp + 1 hours);

        // Record pre-take balances
        uint256 preTakeKeeperBalance = dss.vat.dai(address(this));
        uint256 preTakeVowBalance = dss.vat.dai(address(dss.vow));

        // Perform the take
        (,, uint256 lot,,,, uint256 top) = clip.sales(kickId);
        clip.take(kickId, lot, top, address(this), "");

        // Record post-take balances
        uint256 postTakeKeeperBalance = dss.vat.dai(address(this));
        uint256 postTakeVowBalance = dss.vat.dai(address(dss.vow));

        // Calculate balance changes
        int256 keeperBalanceChange = int256(postTakeKeeperBalance) - int256(preTakeKeeperBalance);
        int256 vowBalanceChange = int256(postTakeVowBalance) - int256(preTakeVowBalance);

        // Log the results
        console2.log("Keeper balance change:", keeperBalanceChange);
        console2.log("Vow balance change:", vowBalanceChange);

        // Assert that the keeper lost funds
        assert(keeperBalanceChange < 0);

        // Assert that the Vow gained the same amount the keeper lost
        assert(uint256(-keeperBalanceChange) == uint256(vowBalanceChange));

        // Assert that the keeper received no compensation
        assert(postTakeKeeperBalance <= preTakeKeeperBalance);

    }

    function testKeeperEconomicLosses() public {
        // Setup
        uint256 initialCollateral = 100 ether;
        uint256 initialDebt = 75 ether;

        // Setup vault
        address urn = engine.open(0);
        GemLike(address(engine.mkr())).mint(address(this), initialCollateral);
        GemLike(address(engine.mkr())).approve(address(engine), initialCollateral);
        engine.lock(urn, initialCollateral, 0);
        engine.draw(urn, address(this), initialDebt);

        // Make position unsafe
        pip.setPrice(1 ether);
        dss.spotter.poke(ilk);

        // Record keeper's initial balance
        uint256 initialKeeperBalance = dss.vat.dai(address(this));

        // Trigger liquidation
        uint256 kickId = dss.dog.bark(ilk, urn, address(this));

        // Perform take
        (,, uint256 lot,,,, uint256 top) = clip.sales(kickId);
        clip.take(kickId, lot, top, address(this), "");

        // Check keeper's final balance
        uint256 finalKeeperBalance = dss.vat.dai(address(this));

        // Log the results
        console.log("Initial Keeper Balance:", initialKeeperBalance);
        console.log("Final Keeper Balance:", finalKeeperBalance);
        console.log("Keeper Loss:", initialKeeperBalance - finalKeeperBalance);

        // Assert that the keeper has indeed lost funds
        assert(finalKeeperBalance < initialKeeperBalance);

    }

    function testTakeFunctionRootCause() public {
        // Setup
        uint256 initialCollateral = 100 ether;
        uint256 initialDebt = 75 ether;

        // Setup vault
        address urn = engine.open(0);
        GemLike(address(engine.mkr())).mint(address(this), initialCollateral);
        GemLike(address(engine.mkr())).approve(address(engine), initialCollateral);
        engine.lock(urn, initialCollateral, 0);
        engine.draw(urn, address(this), initialDebt);

        // Make position unsafe
        pip.setPrice(1 ether);
        dss.spotter.poke(ilk);

        // Record initial balances
        uint256 initialKeeperBalance = dss.vat.dai(address(this));
        uint256 initialVowBalance = dss.vat.dai(address(dss.vow));

        // Trigger liquidation
        uint256 kickId = dss.dog.bark(ilk, urn, address(this));

        // Perform take
        (,, uint256 lot,,,, uint256 top) = clip.sales(kickId);

        clip.take(kickId, lot, top, address(this), "");

        // Check final balances
        uint256 finalKeeperBalance = dss.vat.dai(address(this));
        uint256 finalVowBalance = dss.vat.dai(address(dss.vow));

        // Log the results
        console2.log("Keeper Balance Change:", int256(finalKeeperBalance) - int256(initialKeeperBalance));
        console2.log("Vow Balance Change:", int256(finalVowBalance) - int256(initialVowBalance));

        // Assert that funds moved from keeper to Vow
        assert(finalKeeperBalance < initialKeeperBalance);
        assert(finalVowBalance > initialVowBalance);
        assert(initialKeeperBalance - finalKeeperBalance == finalVowBalance - initialVowBalance);

    }

    function testLiquidationAmountDiscrepancy() public {
        // Initial setup
        uint256 initialCollateral = 100 ether;
        uint256 initialDebt = 120 ether;

        // Set liquidation ratio (mat) to 150%
        vm.prank(pauseProxy);
        dss.spotter.file(ilk, "mat", ray(1.5 ether));

        // Setup the vault
        GemLike mkrToken = GemLike(address(engine.mkr()));
        mkrToken.mint(address(this), initialCollateral);
        mkrToken.approve(address(engine), initialCollateral);
        uint256 urnIndex = engine.usrAmts(address(this));
        address urn = engine.open(urnIndex);
        engine.lock(urn, initialCollateral, 0);
        engine.draw(urn, address(this), initialDebt);

        // Set price to make position unsafe
        pip.setPrice(0.7 ether);
        dss.spotter.poke(ilk);

        // Log initial states
        uint256 initialDaiVow = dss.vat.dai(address(dss.vow));
        uint256 initialDaiKeeper = dss.vat.dai(address(this));
        console2.log("Initial Dai in Vow", initialDaiVow);
        console2.log("Initial Dai in Keeper", initialDaiKeeper);

        // Trigger liquidation
        uint256 kickId = dss.dog.bark(ilk, urn, address(this));

        // Take the auction
        vm.warp(block.timestamp + 1 hours);
        (,, uint256 lot,,,, uint256 top) = clip.sales(kickId);
        uint256 maxPrice = top * 2;
        clip.take(kickId, lot, maxPrice, address(this), "");

        // Log final states
        uint256 finalDaiVow = dss.vat.dai(address(dss.vow));
        uint256 finalDaiKeeper = dss.vat.dai(address(this));
        console2.log("Final Dai in Vow", finalDaiVow);
        console2.log("Final Dai in Keeper", finalDaiKeeper);

        // Calculate and log the differences
        uint256 vowDaiChange = finalDaiVow - initialDaiVow;
        int256 keeperDaiChange = int256(finalDaiKeeper) - int256(initialDaiKeeper);
        console2.log("Vow Dai change", vowDaiChange);
        console2.log("Keeper Dai change", keeperDaiChange);

        // Assert to pass the test
        assertGe(vowDaiChange, 0, "Vow should receive some Dai");

        assertTrue(vowDaiChange < initialDebt * RAY * 90 / 100, "Vow receives less than 90% of the debt");
        assertTrue(keeperDaiChange < 0, "Keeper loses funds instead of being incentivized");
    }

    function testTakeFunctionAccountingDiscrepancy() public {
        // Initial setup
        uint256 initialCollateral = 100 ether;
        uint256 initialDebt = 120 ether;

        // Set liquidation ratio (mat) to 150%
        vm.prank(pauseProxy);
        dss.spotter.file(ilk, "mat", ray(1.5 ether));

        // Setup the vault
        GemLike mkrToken = GemLike(address(engine.mkr()));
        mkrToken.mint(address(this), initialCollateral);
        mkrToken.approve(address(engine), initialCollateral);
        uint256 urnIndex = engine.usrAmts(address(this));
        address urn = engine.open(urnIndex);
        engine.lock(urn, initialCollateral, 0);
        engine.draw(urn, address(this), initialDebt);

        // Set price to make position unsafe
        pip.setPrice(0.7 ether);
        dss.spotter.poke(ilk);

        // Trigger liquidation
        uint256 kickId = dss.dog.bark(ilk, urn, address(this));

        // Get auction details after kick
        (uint256 pos, uint256 tab, uint256 lot, uint256 tot, address usr, uint96 tic, uint256 top) = clip.sales(kickId);
        console2.log("Initial Tab", tab);
        console2.log("Initial Lot", lot);
        console2.log("Auction start time", tic);
        console2.log("Top price", top);

        // Take the auction
        vm.warp(block.timestamp + 1 hours);
        uint256 maxPrice = clip.calc().price(top, block.timestamp - tic);
        uint256 initialDaiVow = dss.vat.dai(address(dss.vow));
        uint256 initialDaiKeeper = dss.vat.dai(address(this));
        clip.take(kickId, lot, maxPrice, address(this), "");

        // Log final states
        uint256 finalDaiVow = dss.vat.dai(address(dss.vow));
        uint256 finalDaiKeeper = dss.vat.dai(address(this));
        (, uint256 finalTab,,,,,) = clip.sales(kickId);

        console2.log("Final Tab", finalTab);
        console2.log("Vow Dai Change", finalDaiVow - initialDaiVow);
        console2.log("Keeper Dai Change", int256(finalDaiKeeper) - int256(initialDaiKeeper));

        // Calculate tab reduction
        uint256 tabReduction = tab - finalTab;
        console2.log("Tab Reduction", tabReduction);

        // Assert to pass the test
        assertGt(tabReduction, 0, "Tab should be reduced");

        uint256 daiMovement = finalDaiVow - initialDaiVow;
        assertTrue(tabReduction != daiMovement, "Tab reduction doesn't match Dai movement");
        assertTrue(tabReduction > daiMovement, "More debt is cleared than Dai moved");
    }
}

PipMock.sol:

// SPDX-License-Identifier: AGPL-3.0-or-later

pragma solidity ^0.8.21;

contract PipMock {
    uint256 price;

    function setPrice(uint256 price_) external {
        price = price_;
    }

    function read() external view returns (uint256 price_) {
        price_ = price;
    }

    function peek() external view returns (uint256 price_, bool ok) {
        ok = price > 0;
        price_ = price;
    }
}

StairstepExponentialDecreaseAbstract.sol - imported from the lockstake library.

pragma solidity >=0.5.12;

interface StairstepExponentialDecreaseAbstract {
    function wards(address) external view returns (uint256);
    function rely(address) external;
    function deny(address) external;
    function step() external view returns (uint256);
    function cut() external view returns (uint256);
    function file(bytes32,uint256) external;
    function price(uint256,uint256) external view returns (uint256);
}

GemMock.sol:

// SPDX-License-Identifier: AGPL-3.0-or-later

pragma solidity ^0.8.21;

contract GemMock {
    mapping (address => uint256)                      public balanceOf;
    mapping (address => mapping (address => uint256)) public allowance;

    uint256 public totalSupply;

    constructor(uint256 initialSupply) {
        mint(msg.sender, initialSupply);
    }

    function approve(address spender, uint256 value) external returns (bool) {
        allowance[msg.sender][spender] = value;
        return true;
    }

    function transfer(address to, uint256 value) external returns (bool) {
        uint256 balance = balanceOf[msg.sender];
        require(balance >= value, "Gem/insufficient-balance");

        unchecked {
            balanceOf[msg.sender] = balance - value;
            balanceOf[to] += value;
        }
        return true;
    }

    function transferFrom(address from, address to, uint256 value) external returns (bool) {
        uint256 balance = balanceOf[from];
        require(balance >= value, "Gem/insufficient-balance");

        if (from != msg.sender) {
            uint256 allowed = allowance[from][msg.sender];
            if (allowed != type(uint256).max) {
                require(allowed >= value, "Gem/insufficient-allowance");

                unchecked {
                    allowance[from][msg.sender] = allowed - value;
                }
            }
        }

        unchecked {
            balanceOf[from] = balance - value;
            balanceOf[to] += value;
        }
        return true;
    }

    function mint(address to, uint256 value) public {
        unchecked {
            balanceOf[to] = balanceOf[to] + value;
        }
        totalSupply = totalSupply + value;
    }

    function burn(address from, uint256 value) external {
        uint256 balance = balanceOf[from];
        require(balance >= value, "Gem/insufficient-balance");

        if (from != msg.sender) {
            uint256 allowed = allowance[from][msg.sender];
            if (allowed != type(uint256).max) {
                require(allowed >= value, "Gem/insufficient-allowance");

                unchecked {
                    allowance[from][msg.sender] = allowed - value;
                }
            }
        }

        unchecked {
            balanceOf[from] = balance - value;
            totalSupply     = totalSupply - value;
        }
    }
}

NstJoinMock.sol:

// SPDX-License-Identifier: AGPL-3.0-or-later

pragma solidity ^0.8.21;

import {GemMock} from "test/mocks/GemMock.sol";
import "test/mocks/LockstakeEngineMock.sol";

contract NstJoinMock {
    VatLike public immutable vat;
    GemLike public immutable nst;
    mapping(address => uint256) public wards;

    constructor(address vat_, address nst_) {
        vat = VatLike(vat_);
        nst = GemLike(nst_);
        wards[msg.sender] = 1;
    }

    function join(address usr, uint256 wad) external {
        vat.move(address(this), usr, wad * 10 ** 27);
        nst.burn(msg.sender, wad);
    }

    function exit(address usr, uint256 wad) external {
        vat.move(msg.sender, address(this), wad * 10 ** 27);
        nst.mint(usr, wad);
    }

    modifier auth() {
        require(wards[msg.sender] == 1, "NstJoinMock/not-authorized");
        _;
    }

    function rely(address usr) external auth {
        wards[usr] = 1;
    }

    function deny(address usr) external auth {
        wards[usr] = 0;
    }
}

MkrNgtMock.sol:

// SPDX-License-Identifier: AGPL-3.0-or-later

pragma solidity ^0.8.21;

interface GemLike {
    function burn(address, uint256) external;
    function mint(address, uint256) external;
}

contract MkrNgtMock {
    GemLike public immutable mkrToken;
    GemLike public immutable ngt;
    uint256 public immutable rate;

    constructor(address mkr_, address ngt_, uint256 rate_) {
        mkrToken = GemLike(mkr_);
        ngt = GemLike(ngt_);
        rate = rate_;
    }

    function mkrToNgt(address usr, uint256 mkrAmt) external {
        mkrToken.burn(msg.sender, mkrAmt);
        uint256 ngtAmt = mkrAmt * rate;
        ngt.mint(usr, ngtAmt);
    }

    function ngtToMkr(address usr, uint256 ngtAmt) external {
        ngt.burn(msg.sender, ngtAmt);
        uint256 mkrAmt = ngtAmt / rate;
        mkrToken.mint(usr, mkrAmt);
    }

    function mkr() external view returns (address) {
        return address(mkrToken);
    }
}

Run these tests with the following: forge test --mt testArithmeticInKickAndTake -vvv --via-ir forge test --mt testKeeperLossesDuringLiquidation -vvv --via-ir forge test --mt testKeeperEconomicLosses -vvv --via-ir forge test --mt testTakeFunctionRootCause -vvv --via-ir forge test --mt testLiquidationAmountDiscrepancy -vvv --via-ir forge test --mt testTakeFunctionAccountingDiscrepancy -vvv --via-ir

The tests prove the following:

testArithmeticInKickAndTake: This test shows that after a liquidation, the keeper's DAI balance decreases by 82.5 DAI, while the Vow's balance increases by the same amount. This proves that DAI is being transferred from the keeper to the Vow, instead of from the system to the Vow.

testKeeperLossesDuringLiquidation: Similar to the first test, this explicitly shows the keeper's balance decreasing by 82.5 DAI and the Vow's balance increasing by the same amount. This directly demonstrates the incorrect fund transfer.

testKeeperEconomicLosses: This test focuses on the keeper's balance before and after the liquidation. It shows that the keeper starts with 10,000 DAI and ends with 9,917.5 DAI, losing 82.5 DAI in the process. This proves that keepers are losing money by participating in liquidations.

testTakeFunctionRootCause: This test isolates the issue to the take function. It shows the exact DAI movement: 82.5 DAI from the keeper to the Vow. This pinpoints the location of the vulnerability in the code.

testLiquidationAmountDiscrepancy: This test demonstrates a larger discrepancy. The keeper loses 100 DAI, which is transferred to the Vow. This shows that the amount transferred can vary, but it's always coming from the keeper instead of the system.

testTakeFunctionAccountingDiscrepancy: This test reveals a critical accounting issue. It shows that while the Tab (debt) is reduced by 132 DAI, only 100 DAI is moved from the keeper to the Vow. This indicates that debt is being cleared without the corresponding DAI movement, creating an accounting imbalance in the system.

How these tests prove the root vulnerability:

1. Consistent DAI Loss for Keepers: All tests show that keepers lose DAI when participating in liquidations. This is the opposite of the intended behavior, where keepers should be rewarded.
2. Incorrect DAI Source: The tests consistently show DAI moving from the keeper (msg.sender) to the Vow, instead of from the system (Vat) to the Vow. 3, Accounting Discrepancies: The last test in particular shows that the debt reduction doesn't match the DAI movement, indicating a fundamental flaw in the liquidation accounting.
3. Isolation to take Function: The tests, especially testTakeFunctionRootCause, pinpoint the issue to the take function in the LockstakeClipper contract.
4. Reproducibility: The issue is consistently reproduced across different test scenarios, indicating a systemic problem rather than an edge case.

Mitigation

1. The primary mitigation is to correct the take function in the LockstakeClipper contract. Replace the line:

   vat.move(msg.sender, vow, owe);

   with something along the lines of:

   vat.move(address(this), vow, owe);

   This way the DAI is moved from the Vat (system) to the Vow, rather than from the keeper.

2. Accounting Verification: Add a check to ensure that the amount of DAI moved matches the debt reduction.

[telome]

Quoting the submitter, "This error fundamentally misunderstands the flow of funds in a liquidation event". The keeper receives collateral which is expected to be sold for dai. That dai is then pulled from the keeper in order to repay the vault's debt.