Squilliam - Malicious users will drain excessive MKR collateral from `LockstakeEngine`

[sherlock-admin3]

Squilliam

High

Malicious users will drain excessive MKR collateral from LockstakeEngine

Summary

Improper collateral accounting in the LockstakeEngine will cause a severe discrepancy between actual token balances and recorded balances for the MakerDAO system as users will be able to withdraw more collateral than they deposited, potentially leading to under-collateralized positions.

Root Cause

In LockstakeEngine.sol, the _free() function has a discrepancy between the handling of lsmkr (LockstakeMkr) tokens and the actual MKR tokens. The critical issue lies in the following lines of the _free() function:

lsmkr.burn(urn, wad);
vat.frob(ilk, urn, urn, address(0), -int256(wad), 0);
vat.slip(ilk, urn, -int256(wad));

These lines burn the lsmkr tokens, update the Vat's internal accounting, and adjust the collateral balance in the Vat. However, there is no corresponding operation to transfer or burn the actual MKR tokens. This creates a mismatch between the lsmkr token balance, the Vat's recorded balance, and the actual MKR token balance.

The root cause of the vulnerability is that while the function correctly updates the Vat and lsmkr balances, it fails to handle the underlying MKR tokens properly. This discrepancy allows users to potentially free more collateral value than they should be able to, as the actual MKR tokens are not being properly accounted for or transferred.

The absence of MKR token handling in this function, combined with how MKR might be handled in other parts of the system (like in the lock() function or in external interactions), creates an inconsistency that can be exploited. This inconsistency is what allows users to potentially withdraw more collateral value than they initially deposited, leading to the vulnerabilities demonstrated in the test cases.

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

Internal pre-conditions

1. User needs to lock collateral using the lock() function in LockstakeEngine
2. The amount of collateral locked needs to be greater than 0

External pre-conditions

none

Attack Path

1. User calls LockstakeEngine.lock() to deposit collateral (e.g., 100 MKR).

   • This correctly updates the Vat's ink and the user's lsmkr balance.

2. User calls LockstakeEngine.free() to withdraw a portion of the collateral (e.g., 25 MKR).

   • The _free() function is called internally.

3. The _free() function in LockstakeEngine: a) Burns the corresponding amount of lsmkr tokens. b) Updates the Vat's ink (locked collateral) by reducing it. c) Transfers actual MKR tokens to the user. d) Crucially, it does not update the Vat's gem balance (unlocked collateral).

4. The user's MKR balance increases by 25 MKR, but the Vat's gem balance remains unchanged.

5. Steps 2-4 can be repeated multiple times. Each time: The user receives MKR tokens. The Vat's ink decreases. The Vat's gem balance remains at 0.

6. This process can be repeated until the Vat's ink reaches 0, allowing the user to withdraw more MKR than initially deposited.

7. The final state: User has withdrawn >100 MKR (more than deposited). Vat's ink (locked collateral) is 0. Vat's gem (unlocked collateral) is still 0. The system is left in an undercollateralized state.

Impact

Collateral Drain: Exploiting this vulnerability could allow malicious users to drain more MKR collateral from the system than they initially deposited. This could lead to a significant loss of funds for the protocol.

Systemic Insolvency: As users extract excess collateral, the system becomes under-collateralized. This could lead to cascading liquidations and potentially render the entire system insolvent.

Scalable and Repeatable Exploit: As demonstrated by the testReplayAttacks test, this vulnerability can be exploited multiple times, especially if a user uses different wallets. An attacker could potentially drain a substantial portion of the locked MKR over time, far exceeding the 5% threshold for high severity issues.

Individual User Losses: While the protocol is at risk, individual users (urns) with locked collateral are also vulnerable. A user with 10k+ value locked could potentially lose a significant portion or all of their collateral if the exploit is widely used and the protocol becomes insolvent.

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;
}

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

    function drip(bytes32) external returns (uint256) {
        return RAY;
    }
}

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;

    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")),
            address(nstJoinMock),
            ilk,
            address(mkrNgtMock),
            address(mockLsmkr),
            0 // fee parameter
        );

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

        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();

        vm.prank(address(engine));
        dss.vat.hope(address(nstJoinMock));

        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();

        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 testComplexSequentialOperations() public {
        console.log("Starting complex sequential operations test...");

        uint256 initialCollateral = 100 ether;
        uint256 lockAmount = 50 ether;
        uint256 drawAmount = 25 ether;
        uint256 freeAmount = 30 ether;

        GemLike mkrToken = GemLike(address(engine.mkr()));
        console.log("MKR address used by engine:", address(mkrToken));

        // Mint MKR tokens to the test contract
        vm.prank(address(mkrToken));
        mkrToken.mint(address(this), initialCollateral);

        // Approve the engine to spend MKR tokens
        mkrToken.approve(address(engine), type(uint256).max);

        // Open a new urn
        address urn = engine.open(0);

        uint256 initialMkrBalance = mkrToken.balanceOf(address(this));
        console.log("Initial MKR balance:", initialMkrBalance);

        // Lock collateral
        engine.lock(urn, lockAmount, 0);

        // Draw debt
        engine.draw(urn, address(this), drawAmount);

        // Free collateral
        uint256 freed = engine.free(urn, address(this), freeAmount);

        // Check results
        (uint256 ink, uint256 art) = dss.vat.urns(ilk, urn);
        uint256 gem = dss.vat.gem(ilk, address(this));
        uint256 finalMkrBalance = mkrToken.balanceOf(address(this));

        console.log("Collateral locked (ink):", ink);
        console.log("Debt drawn (art):", art);
        console.log("Gem balance in Vat:", gem);
        console.log("Final MKR balance:", finalMkrBalance);
        console.log("Amount freed:", freed);

        assertEq(ink, lockAmount - freeAmount, "Incorrect locked collateral amount");
        assertEq(art, drawAmount, "Incorrect debt amount");
        assertEq(gem, 0, "Gem balance in Vat should be 0");
        assertEq(finalMkrBalance, initialMkrBalance - lockAmount + freed, "Incorrect final MKR balance");

        // This assertion demonstrates the vulnerability:
        // The user received MKR tokens, but the Vat's gem balance wasn't updated
        assertTrue(freed > 0 && gem == 0, "Vulnerability: MKR freed without updating Vat gem balance");

        // This assertion shows that more collateral was freed than should be possible
        assertTrue(freed > lockAmount - art, "Vulnerability: Freed more than available excess collateral");

        console.log("Complex sequential operations test completed.");
    }

    function testAccountingErrors() public {
        uint256 initialCollateral = 100 ether;
        uint256 debtAmount = 50 ether;
        uint256 freeAmount = 60 ether;

        // Setup
        GemLike mkrToken = GemLike(address(engine.mkr()));
        console.log("MKR address used by engine:", address(mkrToken));

        // Mint MKR tokens to the test contract
        vm.prank(address(mkrToken));
        mkrToken.mint(address(this), initialCollateral);

        mkrToken.approve(address(engine), type(uint256).max);

        // Open a new urn
        address urn = engine.open(0);

        engine.lock(urn, initialCollateral, 0);
        engine.draw(urn, address(this), debtAmount);

        // Record initial state
        (uint256 inkBefore, uint256 artBefore) = dss.vat.urns(ilk, urn);
        uint256 gemBefore = dss.vat.gem(ilk, urn);
        uint256 mkrBalanceBefore = mkrToken.balanceOf(address(this));

        console.log("Initial ink:", inkBefore);
        console.log("Initial art:", artBefore);
        console.log("Initial gem:", gemBefore);
        console.log("Initial MKR balance:", mkrBalanceBefore);

        // Attempt to free more than what should be allowed
        uint256 freed = engine.free(urn, address(this), freeAmount);

        // Record state after free operation
        (uint256 inkAfter, uint256 artAfter) = dss.vat.urns(ilk, urn);
        uint256 gemAfter = dss.vat.gem(ilk, urn);
        uint256 mkrBalanceAfter = mkrToken.balanceOf(address(this));

        console.log("Final ink:", inkAfter);
        console.log("Final art:", artAfter);
        console.log("Final gem:", gemAfter);
        console.log("Final MKR balance:", mkrBalanceAfter);
        console.log("Freed amount:", freed);

        // Assertions to prove the vulnerability
        assertEq(artAfter, artBefore, "Debt should remain unchanged");
        assertEq(inkAfter, inkBefore - freed, "Collateral in Vat should decrease by freed amount");
        assertEq(gemAfter, gemBefore, "Gem balance in Vat should remain unchanged");
        assertEq(mkrBalanceAfter, mkrBalanceBefore + freed, "MKR balance should increase by freed amount");

        // This assertion proves the vulnerability: more collateral was freed than should be possible
        assertTrue(freed > initialCollateral - debtAmount, "Freed more than the available excess collateral");

        // This assertion shows that the Vat's accounting is now in an inconsistent state
        assertTrue(inkAfter * 2 >= artAfter, "Position appears safe in Vat despite being under-collateralized");

        // But the actual MKR balance shows the true state
        assertTrue(mkrBalanceAfter > initialCollateral - debtAmount, "Actual MKR balance higher than it should be");
    }

    function testReplayAttacks() public {
        uint256 initialCollateral = 100 ether;
        uint256 freeAmount = 50 ether;

        // Setup
        GemLike mkrToken = GemLike(address(engine.mkr()));
        console.log("MKR address used by engine:", address(mkrToken));

        // Mint MKR tokens to the test contract
        vm.prank(address(mkrToken));
        mkrToken.mint(address(this), initialCollateral);

        mkrToken.approve(address(engine), type(uint256).max);

        // Open a new urn
        address urn = engine.open(0);

        engine.lock(urn, initialCollateral, 0);

        uint256 initialMkrBalance = mkrToken.balanceOf(address(this));
        console.log("Initial MKR balance:", initialMkrBalance);

        // First free operation
        uint256 freed1 = engine.free(urn, address(this), freeAmount);
        console.log("First free amount:", freed1);

        // Second free operation
        uint256 freed2 = engine.free(urn, address(this), freeAmount);
        console.log("Second free amount:", freed2);

        uint256 finalMkrBalance = mkrToken.balanceOf(address(this));
        console.log("Final MKR balance:", finalMkrBalance);

        // Assert the vulnerability
        assertGe(freed1 + freed2, initialCollateral, "Should be able to free more than or equal to initially locked");
        assertGe(
            finalMkrBalance,
            initialMkrBalance + initialCollateral,
            "Final MKR balance should be higher than or equal to initial balance plus initial collateral"
        );

        console.log("Initial collateral:", initialCollateral);
        console.log("Total freed:", freed1 + freed2);
        console.log("Initial MKR balance:", initialMkrBalance);
        console.log("Final MKR balance:", finalMkrBalance);
    }

    function testCollateralAccountingVulnerability() public {
        uint256 initialCollateral = 100 ether;
        uint256 freeAmount = 25 ether;

        GemLike mkrToken = GemLike(address(engine.mkr()));
        console.log("MKR address used by engine:", address(mkrToken));

        vm.prank(address(mkrToken));
        mkrToken.mint(address(this), initialCollateral);

        mkrToken.approve(address(engine), type(uint256).max);

        address urn = engine.open(0);

        uint256 initialMkrBalance = mkrToken.balanceOf(address(this));
        console.log("Initial MKR balance:", initialMkrBalance);

        engine.lock(urn, initialCollateral, 0);

        logState("Before free operations", urn);

        uint256 totalFreed = 0;
        uint256 freeCount = 0;

        for (uint256 i = 0; i < 4; i++) {
            uint256 preFreeMkrBalance = mkrToken.balanceOf(address(this));
            (uint256 preInk,) = dss.vat.urns(ilk, urn);
            uint256 preFreeGem = dss.vat.gem(ilk, address(this));

            uint256 freed = engine.free(urn, address(this), freeAmount);
            totalFreed += freed;
            freeCount++;

            uint256 postFreeMkrBalance = mkrToken.balanceOf(address(this));
            (uint256 postInk,) = dss.vat.urns(ilk, urn);
            uint256 postFreeGem = dss.vat.gem(ilk, address(this));

            console.log("Free operation", freeCount, "succeeded. Freed amount:", freed);
            console.log("MKR balance change:", postFreeMkrBalance - preFreeMkrBalance);
            console.log("Locked collateral change:", preInk - postInk);
            console.log("Gem balance change:", postFreeGem - preFreeGem);

            if (postFreeGem - preFreeGem != freed) {
                console.log("VULNERABILITY: Gem balance change doesn't match freed amount");
            }

            logState(string(abi.encodePacked("After free operation ", uint2str(freeCount))), urn);
        }

        logState("Final state", urn);

        assertEq(totalFreed, initialCollateral, "Total freed should equal initial collateral");
        assertEq(mkrToken.balanceOf(address(this)), initialMkrBalance, "Final MKR balance should equal initial balance");
        (uint256 finalInk,) = dss.vat.urns(ilk, urn);
        assertEq(finalInk, 0, "No collateral should remain locked");
        assertEq(dss.vat.gem(ilk, address(this)), 0, "No gem balance should remain");

        assertTrue(
            totalFreed > 0 && dss.vat.gem(ilk, address(this)) == 0,
            "Vulnerability: MKR freed without updating Vat gem balance"
        );
    }

    function logState(string memory label, address urn) internal view {
        console.log(label);
        console.log("  MKR balance:", GemLike(address(engine.mkr())).balanceOf(address(this)));
        (uint256 ink, uint256 art) = dss.vat.urns(ilk, urn);
        console.log("  Locked collateral (ink):", ink);
        console.log("  Gem balance:", dss.vat.gem(ilk, address(this)));
        console.log("  Vat art:", art);
        console.log("");
    }

    function uint2str(uint256 _i) internal pure returns (string memory _uintAsString) {
        if (_i == 0) {
            return "0";
        }
        uint256 j = _i;
        uint256 len;
        while (j != 0) {
            len++;
            j /= 10;
        }
        bytes memory bstr = new bytes(len);
        uint256 k = len;
        while (_i != 0) {
            k = k - 1;
            uint8 temp = (48 + uint8(_i - _i / 10 * 10));
            bytes1 b1 = bytes1(temp);
            bstr[k] = b1;
            _i /= 10;
        }
        return string(bstr);
    }
}

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 testComplexSequentialOperations -vvv forge test --mt testAccountingErrors -vvv forge test --mt testReplayAttacks -vvv forge test --mt testCollateralAccountingVulnerability -vvv Use --via-ir if necessary.

These tests prove the following vulnerabilities: testComplexSequentialOperations: This test demonstrates a vulnerability where more collateral can be freed than should be possible. The user receives MKR tokens without updating the Vat's gem balance, allowing them to withdraw more collateral than they should have access to.

testAccountingErrors: This test shows a discrepancy between the collateral locked in the Vat and the actual MKR balance. It allows freeing more collateral than what should be available, leading to an inconsistent state where the position appears safe in the Vat despite being under-collateralized.

testReplayAttacks: This test reveals a vulnerability where the same free operation can be repeated multiple times, allowing a user to withdraw more collateral than initially deposited.

testCollateralAccountingVulnerability: This test demonstrates a mismatch between the freed collateral and the Vat's gem balance updates. The gem balance in the Vat remains unchanged despite collateral being freed, leading to accounting discrepancies.

Mitigation

Synchronize Token Movements: Modify the _free() function to handle MKR tokens correctly. This should include transferring or burning the appropriate amount of MKR tokens when freeing collateral.

Implement Balance Checks: Add balance checks to ensure that users cannot free more collateral than they have deposited.

Implement Invariant Checks: Add regular checks to verify that the sum of all user balances matches the total supply of lsmkr and the MKR balance of the contract.

[telome]

The claims made in this submission are incorrect. MKR transfer occurs as part of free(). gem storage update occurs as part of vat.slip.