search

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

1 stars 1 forks source link

chaduke - The race condition between LockstakeClipper.redo() and LockstakeClipper.upchost() might lead to the loss of incentives and the cost of gas fee for a keeper. #61

Closed sherlock-admin2 closed 1 month ago

sherlock-admin2 commented 1 month ago

chaduke

Medium

The race condition between LockstakeClipper.redo() and LockstakeClipper.upchost() might lead to the loss of incentives and the cost of gas fee for a keeper.

Summary

The race condition between LockstakeClipper.redo() and LockstakeClipper.upchost() might lead to the loss of incentives and the cost of gas fee for a keeper. Both functions access chost and the calculation of incentive is based on chost.

The keeper will lose all 100% incentives and gas fee, thus I mark this as medium.

Root Cause

LockstakeClipper.redo() will use chost to calculate the incentive for a keeper. In particular, if the remaiing debt tab is less than chost, then there is no incentive at all.

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

Meanwhile, upchost will update the value of chost.

Therefore, there is a race condition between these two functions.

It is possible that a keeper observes an old value of chost and then proceed with the redo function, but this transaction is frontruned by upchost that increases the value of chost. As a result, the keeper ends up receiving no incentive and waste of his gas, which can not be ignored on a blockchain such as ethererium.

Internal pre-conditions

The remaining debt tab is smaller than the new chost value.

External pre-conditions

Either _dust or dog.chop(ilk) increases such that the remaining debt tab is smaller than the new chost value.

Attack Path

In the following POC, we have an ongoing auction after the bidding by Ali:

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

The old chost is: 10000000000000000000000000000000000000000000000 Frank tries to redo the auction after it is up to reset (time elapsed), expecting to receive this amount of incentive based on what he observes as the od chost: vat.dai: 100200000000000000000000000000000000000000000000

Suppose another transaction upchost() frontruns Frank's transaction and updates chost to: 20000000000000000000000000000000000000000000000

Now when Frank's transaction proceeds with the new chost, he will receive zero incentive since the remaing tab < chost. Frank will only waste of his gas fee, which might be expensive on Ethereum. Such gas fee could have been compensated if there is an incentive.

Impact

The race condition between LockstakeClipper.redo() and LockstakeClipper.upchost() might lead to the loss of incentives and the cost of gas fee for a keeper.

PoC

Please comment/uncomment this line clip.upchost() in the code to simulate race condition. Uncommenting means the front-running scenario, which leads to no incentive.

please run forge test --match-test testTake1 -vv.

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

        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? 

        vm.warp(block.timestamp + 3600);
        (bool needsRedo, uint256 price, uint256 lot, uint256 tab) = clip.getStatus(1);
        console2.log("needsRedo: ", needsRedo); 

        console2.log("current: ", clip.chost()); 

        // clip.upchost();   // comment/uncomment this line to simulate the race condition 
        console2.log("The actual chost: ", clip.chost());

        address frank = address(888);
        clip.redo(1, frank);
        printBalance(frank, "frank");       
    }

}

Mitigation

Add an incentive slipper control to redo such that if the incentive is smaller than the given threshhold, then the transaction will revert.

sunbreak1211 commented 1 month ago

The submission fails to show quantifiable impact to the project or to a user. In any case, this code is unchanged from the original clipper, so out of scope.