Utilizing LockStateClipper#yank will result in DOS of collateral being liquidated

Summary

LockStateClipper#yank uses vat.flux to internally transfer the lsMKR ilk. Since it does not follow the same pattern as other ilk and lacks a join contract, this internal balance cannot be converted into real collateral.

Vulnerability Detail

LockstakeClipper.sol#L475-L484

// Cancel an auction during End.cage or via other governance action.
function yank(uint256 id) external auth lock {
    require(sales[id].usr != address(0), "LockstakeClipper/not-running-auction");
    dog.digs(ilk, sales[id].tab);
    uint256 lot = sales[id].lot;
    vat.flux(ilk, address(this), msg.sender, lot);
    engine.onRemove(sales[id].usr, 0, 0);
    _remove(id);
    emit Yank(id);
}

Above we see that yank utilizes the flux command to move the internal collateral balance from the clipper to msg.sender.

LockstakeInit.sol#L241-L251

    IlkRegistryLike(dss.chainlog.getAddress("ILK_REGISTRY")).put(
        cfg.ilk,
        address(0), //@audit-info no join address
        cfg.mkr,
        18,
        7, // New class
        pip,
        address(clipper),
        cfg.name,
        cfg.symbol
    );

We also see that lsMKR does not have a join since the lockstake engine effectively functions as it's join. As a result this internal is impossible to access. Instead a separate governance action is needed to call onTake to recover the underlying MKR. This could lead to a substantial delay in the liquidation leading to large amounts of bad debt being accumulated in the system to due a flaw in the contract.

Although for this contest it is assumed that ESM is never triggered, as stated in the contract comments it is expected that I could be called as part of other governance actions.

Impact

Since these are funds directly queued for liquidation, the delay could lead to large amounts of excess bad debt in the system.

Code Snippet

LockstakeClipper.sol#L475-L484

Tool used

Manual Review

Recommendation

The methodology should be changed to utilize vat.slip and engine.onTake to effectively recover the collateral:

    // Cancel an auction during End.cage or via other governance action.
    function yank(uint256 id) external auth lock {
        require(sales[id].usr != address(0), "LockstakeClipper/not-running-auction");
        dog.digs(ilk, sales[id].tab);
        uint256 lot = sales[id].lot;
--      vat.flux(ilk, address(this), msg.sender, lot);
++      vat.slip(ilk, address(this), -int256(lot));
++      engine.onTake(sales[id].usr, msg.sender, lot);
        engine.onRemove(sales[id].usr, 0, 0);
        _remove(id);
        emit Yank(id);
    }

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

0x52 - Utilizing LockStateClipper#yank will result in DOS of collateral being liquidated #88