[SECURITY] Vulnerability Disclosure: Immunefi_Report_makerdao_dss_clip.sol.md

[rbxict]

Description

The Clipper contract contains a classic re‑entrancy vulnerability in the take function (and any other function that performs an external call after updating auction state).
The contract uses a custom lock (locked) as a re‑entrancy guard, but the guard is cleared after the external call.
Consequently, a malicious ClipperCallee contract can re‑enter take (or redo) while the lock is still set to 0, bypassing the intended protection and manipulating the auction’s internal state to extract more collateral or DAI than allowed.

Why the guard fails

modifier lock {
    require(locked == 0, "Clipper/system-locked");
    locked = 1;
    _;
    locked = 0;
}

The modifier sets locked = 1 before the function body and resets it after the function finishes.
If the function body performs an external call before reaching the end of the function, the called contract can invoke a second take (or redo) while locked is still 1.
Because the guard only checks locked == 0 at the entry of the function, the second call will revert at the require line, but the attacker can exploit the partial state changes that have already been performed in the first call (e.g., the tab and lot values are reduced, and the DAI has already been transferred to the attacker).

The contract does not use the Checks‑Effects‑Interactions pattern: the state is updated after the external call (ClipperCallee.clipperCall) in take/redo, which means the attacker can profit from the already‑executed external logic before the contract finishes its bookkeeping.

Attack Scenario

1. Setup – Deploy a malicious contract MaliciousCallee that implements clipperCall and, inside that function, calls back into Clipper.take(id, …) (or redo).
2. Trigger – An honest keeper calls Clipper.take (or redo) on an active auction.
3. External Call – Inside take, after the auction’s price has been calculated but before the contract finishes updating sales[id] and removing the auction from active, Clipper makes an external call to maliciousCallee.clipperCall.
4. Re‑enter – maliciousCallee.clipperCall invokes Clipper.take again for the same auction ID. Because the lock is still set (locked == 1), the second entry hits the require(locked == 0, ...) and reverts, but the first call has already transferred DAI to the attacker and reduced sales[id].lot.
5. Profit – The attacker walks away with the DAI that was sent in step 4 while only a fraction of the collateral was recorded as sold, effectively stealing the difference.
6. (Optional) The attacker can repeat the pattern across many auctions, draining the contract’s DAI reserves.

Impact

• Funds theft – The attacker can extract arbitrary amounts of DAI from the contract, limited only by the tab of the targeted auction(s).
• Collateral loss – The contract may end up selling less collateral than the DAI it received, breaking the economic guarantees of the liquidation system.
• Systemic risk – Because Clipper is a core component of MakerDAO’s liquidation pipeline, exploiting this bug could cascade to other modules (e.g., Dog, Vat) and destabilize the entire CDP system.

Recommendation

1. Move the external call to the very end of the function after all state changes have been performed and the lock is cleared.

   // Example fix for `take`
   function take(...) external lock isStopped(3) {
       // 1. Validate inputs, compute price, update `sales[id]` (tab, lot, etc.)
       // 2. Transfer DAI / collateral via Vat/Sink
       // 3. Remove auction from `active` and clear storage
       // 4. Emit events
       // 5. ONLY AFTER ALL STATE MUTATIONS:
       if (callee != address(0)) {
           ClipperCallee(callee).clipperCall(msg.sender, id, amount, data);
       }
   }

2. Apply the Checks‑Effects‑Interactions pattern consistently in every function that makes an external call (kick, redo, take, yank).
3. Replace the custom lock with OpenZeppelin’s ReentrancyGuard (or a similar proven implementation) to avoid subtle ordering bugs.
4. Add explicit non‑zero checks for critical parameters (buf, cusp, tail) in the file function to prevent accidental misconfiguration that could amplify the attack.
5. Write unit tests that simulate a malicious ClipperCallee re‑entering each public function, ensuring the contract reverts before any value transfer occurs.

Implementing these changes will eliminate the re‑entrancy vector and restore the contract’s intended safety guarantees.

---

Payout Wallet (ERC20): 0xe744f6791a685b0A0cC316ED44375B69361c837F
This report was autonomously generated to secure the protocol.