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

5 stars 3 forks source link

chaduke - LockstakeClipper.take() might use an actual auction price that is greater than ```max```, as a result, the slippage control fails and the taker pays more than he expects - loss of funds. #10

Closed sherlock-admin2 closed 3 months ago

sherlock-admin2 commented 3 months ago

chaduke

Medium

LockstakeClipper.take() might use an actual auction price that is greater than max, as a result, the slippage control fails and the taker pays more than he expects - loss of funds.

Summary

LockstakeClipper.take() might use an actual auction price that is greater than max, as a result, the slippage control fails and the taker pays more than he expects - loss of funds.

Although we expect such loss is small for each transaction, consider that this might occur repeatedly and indefinitely for different users, I mark this as medium.

Root Cause

LockstakeClipper.take() uses max`` as the slippage control to ensure that the taker will not bid for a price greater thanmax``. This is accomplished in L355.

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

Unfortunately, this might not be the final price that is used since after slice and owe are determined, the actual slice and owe might be readjusted. In particular, the following rounding down error (L373 & L383) for calculating slice might lead to an actual price owe/slice that is greater than max. This new actual price owe/slice is never compared to max. So it is possible, the taker will use a larger price than max. The slipper control might fail.

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

Internal pre-conditions

at L373 and L383, we have a rounding down error for owe / price.

External pre-conditions

None

Attack Path

Consider the following setup for auction: sale.pos: 0 sale.tab: 110000000000000000000000000000000000000000000000 sale.lot: 40000000000000000000 sale.tot: 40000000000000000000 sale.usr: 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496 sale.tic: 604411200 sale.top: 5000000000000000001250000000

and we call LockstakeClipper.take with: id 1 amt: 25000000000000000000 max: 5000000000000000001250000000 who: 0x000000000000000000000000000000000000006F

The initial auction price calculated is: 5000000000000000001250000000

Therefore, the slippage control check will be passed at L355.

After adjustments, we have: final owe: 110000000000000000000000000000000000000000000000 final slice: 21999999999999999994 real price (owe/slice): 5000000000000000001363636363

The real price > max.

In summary, the slipper control fails since the final real price is greater than max.

Impact

LockstakeClipper.take() might use an actual auction price thta is greater than max, as a result, the slippage control fails and the taker pays more than he expects - loss of funds.

PoC

The following is the POC to show that the slippage control for max fails and the taker might use an actual price higher than max:

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

    DssInstance dss;
    address     pauseProxy;
    PipMock     pip;
    GemLike     dai;

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

    modifier takeSetup {
        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....");

        (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);
        console2.log("sale.top: ", sale.top);
        assertEq(sale.top, 5000000000000000001250000000); // $4 plus 25%

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

        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.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 + 1); // 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));
    }

    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 testTake1() public takeSetup {

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

        assertEq(dss.vat.gem(ilk, ali), 21999999999999999994);  // Didn't take whole lot
        assertEq(dss.vat.dai(ali), rad(890 ether)); // Didn't pay more than tab (110)
        assertEq(dss.vat.gem(ilk, address(this)),  1000 ether - 21999999999999999994); // 960 + (40 - 22) returned to usr

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

No response

Mitigation

In the take() function, we should compare the actual price to max:

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) {

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

+        uint256 actualPrice = owe/slice;
+        require(max >= actualPrice, "LockstakeClipper/too-expensive");

            // 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);
    }
telome commented 3 months ago

Out of scope as this code is unchanged from the original clipper. The submissions also fails to show quantifiable impact.