LockstakeClipper.take() might use a stale value of chost and allows an invalid partial purchase, leaving a debt that can potentially not be able to cover, loss of funds for the protocol.

Summary

LockstakeClipper.take() might use a stale value of chost and allows an invalid partial purchase, leaving a debt that can potentially not be able to cover, loss of funds for the protocol.

If lot < 20 chost, then the loss of funds might be > 5%. For smaller lot, the lost can be more. Besides, this can occur each time chost is updated. Therefore, I mark this finding as medium.

Root Cause

LockstakeClipper.take() fails to call upchost() to bring chost up to date, therefore it might use a stale value of chost. As a result, invalid partial purchase is possible - purchase that leaves remaining debt that is greater than the old chost but smaller than the new chost.

Internal pre-conditions

First of all, chost is mutable, if either _dust or dog.chop(ilk) has changed, then chost will change. This is why the function upchost() was designed to bring chost up to date.

An internal precodition is that either _dust or dog.chop(ilk) is increased, but uphost() is not called to bring chost up to date. In other words, the actual chost has increased, but the chost variable in LockstakeClipper is stale and has a smaller value than the actual chost value.

External pre-conditions

A user makes a partial purchase of the auction, which should not be allowed if chost was brought up to date, but is allowed with the stale value of chost. As a result, not-supposed-to-happen debt might never be covered, leading to a loss to the protocol.

Attack Path

Suppose we have chost = 1,000 RAD (initial value) Ongoing auction: tab = 2,000 RAD lot = 10 WAD (collateral to sell) price = 200 RAD per WAD of collateral

Now _dust is doubled, as a result, the actual chost should be updated to 2,000 RAD. However, upchost() is not called. The state variable chost remains 1,000 RAD.

Suppose Alice takes a bid with 5 WAD to raise 1,000 RAD, leaving a remaining tab of 1,000 RAD. This is allowed since the remaining tab is equal to the stale chost. If chost had been updated, then Alice had to buy the whole thing, and would have left no remaining debt to cover.

Now the remaining 1,000 RAD debt is very likely hard to cover due to two reasons: 1) the price will decrease as the auction proceeds; 2) nobody will be incentivized to reset the auction since there is no incentive when tab < _chost (see redo()). The auction might be stuck until authorized intervention. In any case, the protocol will lose funds as a result of allowing invalid partial purchase.

Impact

PoC

In the following, an auction is set up as follows:

sale.pos: 0 sale.tab: 20000000000000000000000000000000000000000000000 sale.lot: 2000000000000000000000 sale.tot: 2000000000000000000000 sale.usr: 0x1a38b0201C9B6acBfadAD17af8d1062F63285413 sale.tic: 604411200 sale.top: 5000000000000000000000000000

The stale chost is: 10000000000000000000000000000000000000000000000

Although _dust is doubled, so the actual chost should be: 20000000000000000000000000000000000000000000000, since clip.upchost() is not called inside take(). Ali calls clip.take() to take an invalid partial purchase with the stale chost value. Ali might have to buy the whole debt if `chost has been brought up to date.

vm.prank(ali); clip.take({ id: 1, amt: 2 ether, max: 5000000000000000000000000000, who: address(ali), // sli is the keeper data: "" });

leaving the following auction:

sale.pos: 0 sale.tab: 10000000000000000000000000000000000000000000000 sale.lot: 1998000000000000000000 sale.tot: 2000000000000000000000 sale.usr: 0x1a38b0201C9B6acBfadAD17af8d1062F63285413 sale.tic: 604411200 sale.top: 5000000000000000000000000000

with a tab < the actual chost of 20000000000000000000000000000000000000000000000

Now suppose clip.upchost() is called, and now chost is updated to: 20000000000000000000000000000000000000000000000.

The remaining debt might not be covered due to the decrease of price or no incentive for reset of the auction. The debt might become a bad debt and become a loss of the protocol.

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

pragma solidity ^0.8.21;

import "dss-test/DssTest.sol";

import "dss-interfaces/Interfaces.sol";
import { LockstakeClipper } from "src/LockstakeClipper.sol";
import { LockstakeEngineMock } from "test/mocks/LockstakeEngineMock.sol";
import { PipMock } from "test/mocks/PipMock.sol";

import { LockstakeEngine } from "src/LockstakeEngine.sol";
import { VoteDelegateFactoryMock, VoteDelegateMock } from "test/mocks/VoteDelegateMock.sol";
import { NstJoinMock } from "test/mocks/NstJoinMock.sol";
import { NstMock } from "test/mocks/NstMock.sol";
import { MkrNgtMock } from "test/mocks/MkrNgtMock.sol";
import { LockstakeMkr } from "src/LockstakeMkr.sol";
import { GemMock } from "test/mocks/GemMock.sol";

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;

    DSTokenAbstract         mkr;
    VoteDelegateFactoryMock voteDelegateFactory;
    NstMock nst;
    MkrNgtMock              mkrNgt;
    LockstakeMkr      lsMkr;
    NstJoinMock nstJoin;
    GemMock                 ngt;

    LockstakeEngine engine;
    LockstakeClipper clip;
    LockstakeMkr lsmkr;

    // Exchange exchange;

    address constant LOG = 0xdA0Ab1e0017DEbCd72Be8599041a2aa3bA7e740F;

    address ali;

    bytes32 constant ilk = "LSE";

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

        mkr = DSTokenAbstract(dss.chainlog.getAddress("MCD_GOV"));
        voteDelegateFactory = new VoteDelegateFactoryMock(address(mkr));
        nst = new NstMock();
        nstJoin = new NstJoinMock(address(vat), address(nst));
        ngt = new GemMock(0);
        mkrNgt = new MkrNgtMock(address(mkr), address(ngt), 24_000);
        lsmkr   = new LockstakeMkr();

        vm.startPrank(pauseProxy);
        dss.vat.init(ilk);                  // init the collateral info identified by ```ilk```
        dss.vat.fold(ilk, address(dss.vow), 1*10**27);       // set initial rate for ilk

/*
 function poke(bytes32 ilk) external {
        (bytes32 val, bool has) = ilks[ilk].pip.peek();
        uint256 spot = has ? rdiv(rdiv(mul(uint(val), 10 ** 9), par), ilks[ilk].mat) : 0;  par = 1 RAY, 
        vat.file(ilk, "spot", spot);
        emit Poke(ilk, val, spot);
    }
*/
        // set spot price for ilk via set price for pip price

        console2.log("set  spot price $$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$");
        pip = new PipMock();                      
        pip.setPrice(4 ether); 
        dss.spotter.file(ilk, "pip", address(pip)); // oracle
        dss.spotter.file(ilk, "mat", ray(2 ether)); // 200% liquidation ratio for easier test calcs
        dss.spotter.poke(ilk);
        (,, uint256 spot1,,) = dss.vat.ilks(ilk);         // so spot price = poke price / 2
        console2.log("spot price: ", spot1); // 4 ether * 10 ** 9  

        dss.vat.file(ilk, "dust", rad(10 ether)); 
        dss.vat.file(ilk, "line", rad(1000000 ether));
        dss.vat.file("Line",      dss.vat.Line() + rad(1000000 ether));

        dss.dog.file(ilk, "chop", 1 ether); // no chop let's say
        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);           // let's use a real one ?????

        engine =  new LockstakeEngine(address(voteDelegateFactory), address(nstJoin), ilk, address(mkrNgt), address(lsmkr),  15 * WAD / 100);
        engine.file("jug", address(dss.jug));
        dss.jug.init(ilk);
        dss.jug.file(ilk, "duty", 100000001 * 10**27 / 100000000); // decides rate

        dss.vat.rely(address(engine));
        vm.stopPrank();

        lsmkr.rely(address(engine));

        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

        // 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("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);        
        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
        console.log("chost: ", clip.chost());

        clip.rely(address(dss.dog));

        vm.startPrank(pauseProxy);
        engine.rely(address(clip));
        vm.stopPrank();

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

        ali = address(111);

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

        vm.startPrank(pauseProxy);
        dss.vat.suck(address(0), address(ali),  rad(1000 ether));
        vm.stopPrank();
    }

    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 printBalance(address a, string memory name) public{
        console2.log("**************************************************");
        console2.log("Vat info for ", name);
        console2.log("mkr bal: ", mkr.balanceOf(a));
        console2.log("nst bal: ", nst.balanceOf(a));
        console2.log("vat.dai: ", dss.vat.dai(a));
        console2.log("vat.sin: ", dss.vat.sin(a));
        console2.log("vat.gem(ilk, a): ", dss.vat.gem(ilk, a));
        (uint256 ink, uint256 art) = dss.vat.urns(ilk, a);
        console2.log("ink: ", ink);
        console2.log("art: ", art);
        console2.log("**************************************************");
    }

/* lead 1: 
*/

    function testTake1() public  {
        console2.log("jug", address(dss.jug));

        address Bob = address(123);
        address kpr = address(456);

        deal(address(mkr), Bob, 100_000 * 10**18);             // 100_000 ether
        deal(address(nst), Bob, 123_000 * 10**18); 
        deal(address(ngt), Bob, 100_000 * 24_000 * 10**18);

        //printBalance(Bob, "Bob");
        //printBalance(address(engine), "engine");

        vm.startPrank(Bob);                // 1. open a urn
        address urn = engine.open(0);
        mkr.approve(address(engine), 100_000 * 10**18);
        engine.lock(urn, 2000 * 10**18, 0);
        //printBalance(address(urn), "urn");
        //printBalance(address(engine), "engine"); // where is mkr and lsmkr? 

        engine.draw(urn, address(Bob), 20 ether);     
        vm.stopPrank();

        console2.log("\n after draw.....................");
        //printBalance(Bob, "Bob");
        //printBalance(address(urn), "urn");
        //printBalance(address(engine), "engine"); // where is mkr and lsmkr? 

        // lower the price of collaterl:
        vm.startPrank(pauseProxy);
        vat.file(ilk, "spot", 123);
        vm.stopPrank();      // now we can liquidate urn

        console2.log("\n starto bark$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$");

        assertEq(clip.kicks(), 0);
        dss.dog.bark(ilk, urn, kpr);     
        assertEq(clip.kicks(), 1);
        console2.log("\n after barking..............");
        printAuction(1);
        //printBalance(Bob, "Bob");
        //printBalance(address(urn), "urn");
        //printBalance(address(clip), "clip"); // where is mkr and lsmkr? 
        //printBalance(address(engine), "engine"); // where is mkr and lsmkr? 

        // Bid so owe (= 25 * 5 = 125 RAD) > tab (= 110 RAD)
        // Readjusts slice to be tab/top = 25
        vm.startPrank(pauseProxy);
        dss.vat.file(ilk, "dust", rad(20 ether));  // so the actual chost should be douled
        vm.stopPrank();

        vm.prank(ali); 
        clip.take({
            id:  1,
            amt: 2 ether,
            max:  5000000000000000000000000000,
            who: address(ali),                      // sli is the keeper
            data: ""
        });

        console2.log("After clip.take....");
        clip.upchost();
        console2.log("The actual chost: ", clip.chost());

        printAuction(1);
        //printBalance(Bob, "Bob");
        //printBalance(address(urn), "urn");
        //printBalance(address(clip), "clip"); // where is mkr and lsmkr? 
        //printBalance(address(engine), "engine"); // where is mkr and lsmkr? 

    }

}

Mitigation

Call upchost in the beginning of take():

  function take(
        uint256 id,           // Auction id
        uint256 amt,          // Upper limit on amount of collateral to buy  [wad]
        uint256 max,          // Maximum acceptable price (DAI / collateral) [ray]
        address who,          // Receiver of collateral and external call address
        bytes calldata data   // Data to pass in external call; if length 0, no call is done
    ) external lock isStopped(3) {

+     upchost();
        address usr = sales[id].usr;
        uint96  tic = sales[id].tic;

        require(usr != address(0), "LockstakeClipper/not-running-auction");

        uint256 price;
        {
            bool done;
            (done, price) = status(tic, sales[id].top);

            // Check that auction doesn't need reset
            require(!done, "LockstakeClipper/needs-reset");
        }

        // Ensure price is acceptable to buyer
        require(max >= price, "LockstakeClipper/too-expensive");

        uint256 lot = sales[id].lot;
        uint256 tab = sales[id].tab;
        uint256 owe;

        {
            // Purchase as much as possible, up to amt
            uint256 slice = min(lot, amt);  // slice <= lot

            // DAI needed to buy a slice of this sale
            owe = slice * price;

            // Don't collect more than tab of DAI
            if (owe > tab) {
                // Total debt will be paid
                owe = tab;                  // owe' <= owe
                // Adjust slice
                slice = owe / price;        // slice' = owe' / price <= owe / price == slice <= lot
            } else if (owe < tab && slice < lot) {
                // If slice == lot => auction completed => dust doesn't matter
                uint256 _chost = chost;
                if (tab - owe < _chost) {    // safe as owe < tab
                    // If tab <= chost, buyers have to take the entire lot.
                    require(tab > _chost, "LockstakeClipper/no-partial-purchase");
                    // Adjust amount to pay
                    owe = tab - _chost;      // owe' <= owe
                    // Adjust slice
                    slice = owe / price;     // slice' = owe' / price < owe / price == slice < lot
                }
            }

            // Calculate remaining tab after operation
            tab = tab - owe;  // safe since owe <= tab
            // Calculate remaining lot after operation
            lot = lot - slice;

            // Send collateral to who
            vat.slip(ilk, address(this), -int256(slice));
            engine.onTake(usr, who, slice);

            // Do external call (if data is defined) but to be
            // extremely careful we don't allow to do it to the three
            // contracts which the LockstakeClipper needs to be authorized
            DogLike dog_ = dog;
            if (data.length > 0 && who != address(vat) && who != address(dog_) && who != address(engine)) {
                ClipperCallee(who).clipperCall(msg.sender, owe, slice, data);
            }

            // Get DAI from caller
            vat.move(msg.sender, vow, owe);

            // Removes Dai out for liquidation from accumulator
            dog_.digs(ilk, lot == 0 ? tab + owe : owe);
        }

        if (lot == 0) {
            uint256 tot = sales[id].tot;
            engine.onRemove(usr, tot, 0);
            _remove(id);
        } else if (tab == 0) {
            uint256 tot = sales[id].tot;
            vat.slip(ilk, address(this), -int256(lot));
            engine.onRemove(usr, tot - lot, lot);
            _remove(id);
        } else {
            sales[id].tab = tab;
            sales[id].lot = lot;
        }

        emit Take(id, max, price, owe, tab, lot, usr);
    }

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

chaduke - LockstakeClipper.take() might use a stale value of chost and allows an invalid partial purchase, leaving a debt that can potentially not be able to cover, loss of funds for the protocol. #60