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

1 stars 1 forks source link

ZeroTrust - An attacker can prevent liquidation by using the frontfun voteDelegate::lock() function when their position is being liquidated. #101

Closed sherlock-admin3 closed 1 month ago

sherlock-admin3 commented 1 month ago

ZeroTrust

Medium

An attacker can prevent liquidation by using the frontfun voteDelegate::lock() function when their position is being liquidated.

Summary

voteDelegate::lock() allows setting zero amount, enabling attackers to front-run the liquidation function with this function to block the liquidation.

Root Cause

And within VoteDelegateLike(voteDelegate).lock(), it calls chief.lock(wad);. https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/vote-delegate/src/VoteDelegate.sol#L85

  function lock(uint256 wad) external {
        require(block.number == hatchTrigger || block.number > hatchTrigger + HATCH_SIZE,
                "VoteDelegate/no-lock-during-hatch");
        gov.transferFrom(msg.sender, address(this), wad);
@>        chief.lock(wad);
        stake[msg.sender] += wad;

        emit Lock(msg.sender, wad);
    }

In the chief contract, chief.free() cannot be called in the same block as chief.lock(). https://vscode.blockscan.com/ethereum/0x0a3f6849f78076aefaDf113F5BED87720274dDC0

function lock(uint wad)
        public
        note
    {
@>        last[msg.sender] = block.number;
        GOV.pull(msg.sender, wad);
        IOU.mint(msg.sender, wad);
        deposits[msg.sender] = add(deposits[msg.sender], wad);
        addWeight(wad, votes[msg.sender]);
    }

    function free(uint wad)
        public
        note
    {
@>        require(block.number > last[msg.sender]);
        deposits[msg.sender] = sub(deposits[msg.sender], wad);
        subWeight(wad, votes[msg.sender]);
        IOU.burn(msg.sender, wad);
        GOV.push(msg.sender, wad);
    }

Therefore, calling voteDelegate::lock(0) within the same block results in a revert. This will cause the liquidation process: Dog::bark() -> LockstakeClipper::kick() -> LockstakeEngine::onKick() to revert.

https://vscode.blockscan.com/ethereum/0x135954d155898D42C90D2a57824C690e0c7BEf1B

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

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) {
      //-------skip-----
        // Trigger engine liquidation call-back
    @>>    engine.onKick(usr, lot);

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

https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/lockstake/src/LockstakeEngine.sol#L422

   function onKick(address urn, uint256 wad) external auth {
        // Urn confiscation happens in Dog contract where ilk vat.gem is sent to the LockstakeClipper
        (uint256 ink,) = vat.urns(ilk, urn);
        uint256 inkBeforeKick = ink + wad;
@>>        _selectVoteDelegate(urn, inkBeforeKick, urnVoteDelegates[urn], address(0));
        _selectFarm(urn, inkBeforeKick, urnFarms[urn], address(0), 0);
        lsmkr.burn(urn, wad);
        urnAuctions[urn]++;
        emit OnKick(urn, wad);
    }

Thus, liquidation can be blocked .

Internal pre-conditions

No response

External pre-conditions

1.  The position has borrowed a large amount of DAI, leading to the possibility of liquidation.
2.  The MKR Oracle price drops, causing the position to meet the conditions for liquidation.

Attack Path

1.  Stake a large amount of MKR or NGT.
2.  Borrow a large amount of DAI.
3.  The MKR Oracle price drops, causing the position to meet the conditions for liquidation.
4.  Detect the transaction that initiates the liquidation of the position.
5.  Front-run the liquidation by calling voteDelegate::lock(0).
6.  The liquidation fails,
7.  Repeat steps 4 and 5. making it possible for the position to never be liquidated.

Impact

This can make the position never be liquidated, causing the Maker protocol to incur losses.

PoC

function testOnKickRevertBecauseFrontrun() public{
         address urn = _urnSetUp(false, false);
        uint256 lsmkrInitialSupply = lsmkr.totalSupply();

        //Force liquidation
        vm.store(address(pip), bytes32(uint256(1)), bytes32(uint256(0.05 * 10**18))); // Force liquidation
        dss.spotter.poke(ilk);
        assertEq(clip.kicks(), 0);
        assertEq(engine.urnAuctions(urn), 0);
        (,, uint256 hole,) = dss.dog.ilks(ilk);
        uint256 kicked = hole < 2_000 * 10**45 ? 100_000 * 10**18 * hole / (2_000 * 10**45) : 100_000 * 10**18;

        //frontrun bark
        VoteDelegateMock(voteDelegate).lock(0);

        //liquidation bark
        vm.expectEmit(true, true, true, true);
        emit OnKick(urn, kicked);
      uint256   id = dss.dog.bark(ilk, address(urn), address(this));
        assertEq(clip.kicks(), 1);
        assertEq(engine.urnAuctions(urn), 1);

    }

Mitigation

No response

Duplicate of #62

telome commented 1 month ago

The reserveHatch mechanism was introduced for this. It allows reserving a window where liquidations are possible.