LockstakeClipper.kick() does not add incentives for keepers to tab, the amount of fund to be raised in the auction. As a result, the Vow contract might get into insolvent state eventually.

Summary

LockstakeClipper.kick() does not add incentives for keepers to tab, the amount of fund to be raised in the auction. As a result, the Vow contract might get into insolvent state eventually. This is because the Vow will accumulate more debt in Vat.sin than the fund raised during auction, which is supposed to cover the increased debt. It also might lead to overflow and then stalk the protocol, the user's fund might get stuck in the protocol.

This will occur for each kick() call and loss of funds for the protocol each time, therefore I mark this as high.

Root Cause

The following code in kick() will increase vat.sin(vow) by the incentive amount, however, such incentive (coin) is never added to tab. As a result, tab amount of fund will be raised, but it might not be sufficient to cover due + coin, where due = mul(dart, rate) and coin is the incentive for the keeper.

https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/dba30d7a676c20dfed3bda8c52fd6702e2e85bb1/lockstake/src/LockstakeClipper.sol#L229-L271

One might hope that the incentive part will be covered by tab as well since additional penalty has been included in tab = mul(mul(dart, rate), milk.chop) / WAD. This assumption might not be true since it is possible that due + coin > tab. In other words, the raised fund tab`` might not cover the increased debt forvowinvat.sin(vow), which isdue + coin```.

Internal pre-conditions

When due + coin > tab where tab = mul(due, milk.chop) / WAD. In other words, the raised fund tab`` does not cover the increased debt forvowinvat.sin(vow), which isdue + coin```.

External pre-conditions

None.

Attack Path

When due + coin > tab, each time we liquidate a position, we might have a deficit in Vow. The call workflow is: Dog.bark() -> LockstakeClipper.kick(), we then call clip.take(). See more in the following POC.

Impact

LockstakeClipper.kick() does not add incentives for keepers to tab, the amount of fund to be raised in the auction. As a result, the Vow contract might get into insolvent state eventually. Since vat.sin(vow) will keep increase due to deficit, there might be an overflow and the protocol might stalk, user's funds might get stuck.

PoC

We show that due to not adjusting tab according to the incentive coin to keepers. Finally, there is a deficit for Vow. This might occur frequently and as a result, the Vow contract might become insolvent.

Initially, we have vat.sin(vow): 31594496619448079749302857277028575889830976392587267.
We have due = 100000000000000000000000000000000000000000000000 inside Dog.bark().
Dog.bark calls LockstakeClipper.kick() with tab = 110000000000000000000000000000000000000000000000;
The incentive for the keeper is coin = 102200000000000000000000000000000000000000000000.
Therefore, the debt for Vow vat.sin(vow) increases by due + coin = 202200000000000000000000000000000000000000000000.
However, only tab amount will be raised, as a result, we have a deficit of 92200000000000000000000000000000000000000000000.
When such deficit occurs in more and more auctions, Vow accumulates more debt and become insolvent or leading to overflow and stalk the protocol (users' fund might get stuck in the protocol).

The POC is as follows. Run forge test --match-test testTake1 -vv to verify all numbers and deficit.

// 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 { LockstakeEngineMock } from "test/mocks/LockstakeEngineMock.sol";
import { PipMock } from "test/mocks/PipMock.sol";

contract BadGuy {
    LockstakeClipper clip;

    constructor(LockstakeClipper clip_) {
        clip = clip_;
    }

    function clipperCall(address sender, uint256 owe, uint256 slice, bytes calldata data)
        external {
        sender; owe; slice; data;
        clip.take({ // attempt reentrancy
            id: 1,
            amt: 25 ether,
            max: 5 ether * 10E27,
            who: address(this),
            data: ""
        });
    }
}

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

interface VowLike {
    function flap() external returns (uint256);
    function Sin() external view returns (uint256);
    function Ash() external view returns (uint256);
    function heal(uint256) external;
    function bump() external view returns (uint256);
    function hump() external view returns (uint256);
}

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 LockstakeClipperTest is DssTest {
    using stdStorage for StdStorage;

    DssInstance dss;
    address     pauseProxy;
    PipMock     pip;
    GemLike     dai;
    VowLike vow;
    VatLike vat;

    LockstakeEngineMock engine;
    LockstakeClipper clip;

    // Exchange exchange;

    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`

    function _ink(bytes32 ilk_, address urn_) internal view returns (uint256) {
        (uint256 ink_,) = dss.vat.urns(ilk_, urn_);
        return ink_;
    }
    function _art(bytes32 ilk_, address urn_) internal view returns (uint256) {
        (,uint256 art_) = dss.vat.urns(ilk_, urn_);
        return art_;
    }

    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 takeSetup() public {
        address calc = CalcFabLike(dss.chainlog.getAddress("CALC_FAB")).newStairstepExponentialDecrease(address(this));
        CalcLike(calc).file("cut",  RAY - ray(0.01 ether));  // 1% decrease
        CalcLike(calc).file("step", 1);                      // Decrease every 1 second

        clip.file("buf",  ray(1.25 ether));   // 25% Initial price buffer
        clip.file("calc", address(calc));     // File price contract
        clip.file("cusp", ray(0.3 ether));    // 70% drop before reset
        clip.file("tail", 3600);              // 1 hour before reset

        (uint256 ink, uint256 art) = dss.vat.urns(ilk, address(this));
        assertEq(ink, 40 ether);
        assertEq(art, 100 ether);

        console2.log("ink: ", ink);
        console2.log("art: ", art);

        assertEq(clip.kicks(), 0);
        dss.dog.bark(ilk, address(this), address(this));                 // ???????
        assertEq(clip.kicks(), 1);

        console2.log("end of bark....");
        printVowInfo();

        (ink, art) = dss.vat.urns(ilk, address(this));
        assertEq(ink, 0);
        assertEq(art, 0);

        LockstakeClipper.Sale memory sale;
        (sale.pos, sale.tab, sale.lot, sale.tot, sale.usr, sale.tic, sale.top) = clip.sales(1);
        assertEq(sale.pos, 0);
        assertEq(sale.tab, rad(110 ether));
        assertEq(sale.lot, 40 ether);
        assertEq(sale.tot, 40 ether);
        assertEq(sale.usr, address(this));
        assertEq(sale.tic, block.timestamp);

        assertEq(dss.vat.gem(ilk, ali), 0);
        assertEq(dss.vat.dai(ali), rad(1000 ether));
        assertEq(dss.vat.gem(ilk, bob), 0);
        assertEq(dss.vat.dai(bob), rad(1000 ether));
    }

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

        dss = MCD.loadFromChainlog(LOG);

        pauseProxy = dss.chainlog.getAddress("MCD_PAUSE_PROXY");
        dai = GemLike(dss.chainlog.getAddress("MCD_DAI"));
        vow = VowLike(dss.chainlog.getAddress("MCD_VOW"));
        vat = VatLike(dss.chainlog.getAddress("MCD_VAT"));

        printVowInfo();

        pip = new PipMock();
        pip.setPrice(price); // Spot = $2.5
        console2.log("pip price: ", price);

        vm.startPrank(pauseProxy);
        dss.vat.init(ilk);                  // init the collateral info identified by ```ilk```

        // check source code for spotter
        dss.spotter.file(ilk, "pip", address(pip)); // oracle
        dss.spotter.file(ilk, "mat", ray(2 ether)); // 200% liquidation ratio for easier test calcs
        console2.log("ray(2 ether):", ray(2 ether)); // 2*10**18 * 10 ** 9
        dss.spotter.poke(ilk);
        (,, uint256 spot1,,) = dss.vat.ilks(ilk);
        console2.log("spot price: ", spot1); // 2.5 ether * 10**9

        dss.vat.file(ilk, "dust", rad(20 ether)); // $20 dust, 20 ether * 10 ** 27
        console2.log("rad(20 ETHER): ", rad(20 ether));

        dss.vat.file(ilk, "line", rad(10000 ether));
        dss.vat.file("Line",      dss.vat.Line() + rad(10000 ether));

        dss.dog.file(ilk, "chop", 1.1 ether); // 10% chop
        dss.dog.file(ilk, "hole", rad(1000 ether));
        dss.dog.file("Hole",      dss.dog.Dirt() + rad(1000 ether));

        engine = new LockstakeEngineMock(address(dss.vat), ilk);
        dss.vat.rely(address(engine));
        vm.stopPrank();

        // dust and chop filed previously so clip.chost will be set correctly
        clip = new LockstakeClipper(address(dss.vat), address(dss.spotter), address(dss.dog), address(engine));
        clip.file("vow", address(vow));
        clip.file("tip",  rad(100 ether)); // Flat fee of 100 DAI
        clip.file("chip", 0.02 ether);     // Linear increase of 2% of tab
        clip.upchost(); // what does it do?      // dust: 20 ether * 10**27, chop: 1.1 ether = chost = 22 ether * 10**27

        console2.log("chost: ", clip.chost());
        clip.rely(address(dss.dog));

        vm.startPrank(pauseProxy);
        dss.dog.file(ilk, "clip", address(clip)); // 
        dss.dog.rely(address(clip));
        dss.vat.rely(address(clip));

        // add collateral for pauseProxy
        dss.vat.slip(ilk, address(this), int256(1000 ether)); // simply gem[ilk][usr] = add(gem[ilk][usr], wad);

        vm.stopPrank();

        assertEq(dss.vat.gem(ilk, address(this)), 1000 ether); 
        assertEq(dss.vat.dai(address(this)), 0);

        // move collateran between u and v and move debt between u and w
        dss.vat.frob(ilk, address(this), address(this), address(this), 40 ether, 100 ether); // add collateral and add debt, u, v, w: v will pay the collateral, w will get the 
        assertEq(dss.vat.gem(ilk, address(this)), 960 ether);       // move 40 ether from v to u as collateral, so remaining 960 gem
        assertEq(dss.vat.dai(address(this)), rad(100 ether));        // borrow 100 ether DAI (and thus debt) and save it to v as vat.dai balance
        // as a result, more collateral and more debt for u, and the loanded dai is in w

        pip.setPrice(4 ether); // Spot = $2 /// ???????
        dss.spotter.poke(ilk); // Now unsafe
         (,, uint256 spot2,,) = dss.vat.ilks(ilk);
        console2.log("spot price: ", spot2); // 2.5 ether * 10**9, 2 000000000000000000 000000000

        ali = address(111);
        bob = address(222);
        che = address(333);

        dss.vat.hope(address(clip)); // can[address(this)， clip] = 1
        vm.prank(ali); dss.vat.hope(address(clip)); // can[ali, clip] = 1
        vm.prank(bob); dss.vat.hope(address(clip)); // can[bob, clip] =  1

        vm.startPrank(pauseProxy);
        dss.vat.suck(address(0), address(this), rad(1000 ether)); // increase rad(1000 ether) debt and unback DAI as the same time
        dss.vat.suck(address(0), address(ali),  rad(1000 ether)); // increase debt and unbacked DAI
        dss.vat.suck(address(0), address(bob),  rad(1000 ether));
        console2.log("rad(1000 ether: ", rad(1000 ether)); // rad means multiple by 10**27
        vm.stopPrank();

        console.log("\n chop: ", dss.dog.chop(ilk));

        console2.log("after set up...");
        printVowInfo();
    }

    function printAuction(uint id) public
    {
        LockstakeClipper.Sale memory sale;

        (sale.pos, sale.tab, sale.lot, sale.tot, sale.usr, sale.tic, sale.top) = clip.sales(id);
        console2.log("\n***************************************************************");
        console2.log("sale.pos: ", sale.pos);
        console2.log("sale.tab: ", sale.tab);
        console2.log("sale.lot: ", sale.lot);
        console2.log("sale.tot: ", sale.tot);
        console2.log("sale.usr:", sale.usr);
        console2.log("sale.tic: ", sale.tic);
        console2.log("sale.top: ", sale.top);
        console2.log("***************************************************************\n");
    }

    function printVowInfo() public{
        console2.log("\n vow information...............................................");
        console2.log("vat.sin(vow): ", vat.sin(address(vow)));
        console2.log("vat.dai(vow): ", vat.dai(address(vow)));
        console2.log("............................................................\n");
    }

    function testTake1() public  {
        uint256 initialVowSin =  vat.sin(address(vow));
        uint256 initialVowDai = vat.dai(address(vow));

         (,uint256 rate, ,, ) = vat.ilks(ilk);

        uint256 due = 110000000000000000000000000000000000000000000000 * WAD / 1.1 ether;
        console2.log("due: ", due);
        uint256 incentive = 102200000000000000000000000000000000000000000000;
        uint256 tab = 110000000000000000000000000000000000000000000000;

        takeSetup();

        console2.log("before take a bid...$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$");
        printVowInfo();

        printAuction(1);

        // Bid so owe (= 25 * 5 = 125 RAD) > tab (= 110 RAD)
        // Readjusts slice to be tab/top = 25
        vm.prank(ali); 
        clip.take({
            id:  1,
            amt: 25 ether,
            max:  5000000000000000001250000000,
            who: address(ali),
            data: ""
        });

        // Assert auction ends
        LockstakeClipper.Sale memory sale;
        (sale.pos, sale.tab, sale.lot, sale.tot, sale.usr, sale.tic, sale.top) = clip.sales(1);
        assertEq(sale.pos, 0);
        assertEq(sale.tab, 0);
        assertEq(sale.lot, 0);
        assertEq(sale.tot, 0);
        assertEq(sale.usr, address(0));
        assertEq(sale.tic, 0);
        assertEq(sale.top, 0);

        assertEq(dss.dog.Dirt(), 0);

        (,,, uint256 dirt) = dss.dog.ilks(ilk);
        console2.log("dirt: ", dirt);
        assertEq(dirt, 0);

        console2.log("after  a bid...$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$");
        printVowInfo();

        uint256 finalVowSin =  vat.sin(address(vow));
        uint256 finalVowDai = vat.dai(address(vow));

        assertEq(due + incentive, finalVowSin - initialVowSin); // the increases of vat.sin is due to due and incentive
        // we raised tab amont of DAI, 
        console2.log("due + incentive: ", due + incentive);
        assertEq(due + incentive - tab, finalVowSin - initialVowSin - (finalVowDai - initialVowDai));            // deficit
        console2.log("the deficit is: ", finalVowSin - initialVowSin - (finalVowDai - initialVowDai));
    }

}

Mitigation

We need to add incentive for the keeper to tab so that we will raise more fund to cover all debt:

    function kick(
        uint256 tab,  // Debt                   [rad]
        uint256 lot,  // Collateral             [wad]
        address usr,  // Address that will receive any leftover collateral; additionally assumed here to be the liquidated Vault.
        address kpr   // Address that will receive incentives
    ) external auth lock isStopped(1) returns (uint256 id) {
        // Input validation
        require(tab  >                         0, "LockstakeClipper/zero-tab");
        require(lot  >                         0, "LockstakeClipper/zero-lot");
        require(lot <= uint256(type(int256).max), "LockstakeClipper/over-maxint-lot"); // This is ensured by the dog but we still prefer to be explicit
        require(usr !=                address(0), "LockstakeClipper/zero-usr");
        unchecked { id = ++kicks; }
        require(id   >                         0, "LockstakeClipper/overflow");

        active.push(id);

        sales[id].pos = active.length - 1;

        sales[id].tab = tab;
        sales[id].lot = lot;
        sales[id].tot = lot;
        sales[id].usr = usr;
        sales[id].tic = uint96(block.timestamp);

        uint256 top;
        top = rmul(getFeedPrice(), buf);
        require(top > 0, "LockstakeClipper/zero-top-price");
        sales[id].top = top;

        // incentive to kick auction
        uint256 _tip  = tip;
        uint256 _chip = chip;
        uint256 coin;
        if (_tip > 0 || _chip > 0) {
            coin = _tip + wmul(tab, _chip);
            vat.suck(vow, kpr, coin);
+           sales[id].tab = tab + coin;
        }

        // Trigger engine liquidation call-back
        engine.onKick(usr, lot);

        emit Kick(id, top, tab, lot, usr, kpr, coin);
    }

sherlock-audit / 2024-06-makerdao-endgame-judging

chaduke - LockstakeClipper.kick() does not add incentives for keepers to tab, the amount of fund to be raised in the auction. As a result, the Vow contract might get into insolvent state eventually. #43