Overt Garnet Dog - MakerDAO allows users to fill in someone's delegate vote address on `selectVoteDelegate`. As a result, the user cannot perform `vote`.

[sherlock-admin2]

Overt Garnet Dog

Low/Info

MakerDAO allows users to fill in someone's delegate vote address on selectVoteDelegate. As a result, the user cannot perform vote.

Summary

When a users call LockstakeEngine#selectVoteDelegate with someone's delegatevote address, it will works.

But the user cannot participate in the vote, because they are not the delegate of that delegatevote contract address.

Root Cause

A variable delegate is filled as msg.sender when user create VoteDelegate.

# VoteDelegate.sol

  constructor(address chief_, address polling_, address delegate_) {
        chief = ChiefLike(chief_);
        polling = PollingLike(polling_);
        delegate = delegate_;

/////////////////////////////////

# VoteDelegateFactory.sol
 function create() external returns (address voteDelegate) {
        voteDelegate = address(new VoteDelegate{salt: bytes32(uint256(uint160(msg.sender)))}(chief, polling, msg.sender));
        created[voteDelegate] = 1;

        emit CreateVoteDelegate(msg.sender, voteDelegate);
    }

And to participate in a vote / call vote on a VoteDelegate contract, it must have access control delegate_auth:

modifier delegate_auth() {
        require(msg.sender == delegate, "VoteDelegate/sender-not-delegate");
        _;
    }

/////////////////

 function vote(address[] memory yays) external delegate_auth

 function vote(bytes32 slate) external delegate_auth {

    function votePoll(uint256 pollId, uint256 optionId) external delegate_auth {

    function votePoll(uint256[] calldata pollIds, uint256[] calldata optionIds) external delegate_auth {

While the LockstakeEngine#selectVoteDelegate contract allows users to fill in someone's VoteDelegate.

This is because LockstakeEngine#selectVoteDelegate does not verify that the caller must be a delegate of the VoteDelegate address they are filling in.

 function selectVoteDelegate(address urn, address voteDelegate) external urnAuth(urn) {
        require(urnAuctions[urn] == 0, "LockstakeEngine/urn-in-auction");
 >=       require(voteDelegate == address(0) || voteDelegateFactory.created(voteDelegate) == 1, "LockstakeEngine/not-valid-vote-delegate");
        address prevVoteDelegate = urnVoteDelegates[urn];
        require(prevVoteDelegate != voteDelegate, "LockstakeEngine/same-vote-delegate");
        (uint256 ink, uint256 art) = vat.urns(ilk, urn);
        if (art > 0 && voteDelegate != address(0)) {
            (, uint256 rate, uint256 spot,,) = vat.ilks(ilk);
            require(ink * spot >= art * rate, "LockstakeEngine/urn-unsafe");
        }
        _selectVoteDelegate(urn, ink, prevVoteDelegate, voteDelegate);
        emit SelectVoteDelegate(urn, voteDelegate);
    }

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

As a result, when users perform selectVoteDelegate(user's urn, someone's voteDelegate) transaction which is the "lock" token on VoteDelegate contact it's becoming useless.

Because the user cannot participate in the "vote" / cannot call functions that have "delegate_auth"

PoC

First, add these changes to VoteDelegateMock.sol for the delegate_auth test

contract VoteDelegateFactoryMock {
    mapping(address => uint256) public created;
    address immutable private gov;

    constructor(address _gov) {
        gov = _gov;
    }

    function create() external returns (address voteDelegate) {
 +       voteDelegate = address(new VoteDelegateMock(gov, msg.sender));
        created[voteDelegate] = 1;
    }
}

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

    GemLike immutable public gov;
+    address     immutable public delegate;

+    constructor(address gov_, address _delegate) {
        gov = GemLike(gov_);
+        delegate = _delegate;
    }
+    modifier delegate_auth() {
+        require(msg.sender == delegate, "VoteDelegate/sender-not-delegate");
+        _;
+    }
    // --- GOV owner functions

    function lock(uint256 wad) external {
        gov.transferFrom(msg.sender, address(this), wad);
        stake[msg.sender] += wad;
    }

    function free(uint256 wad) external {
        stake[msg.sender] -= wad;
        gov.transfer(msg.sender, wad);
    }

+    function vote(bytes32 slate) external delegate_auth {}
}

Then, paste the PoC code below into LockstakeEngine.t.sol

- run with forge test --match-test test_wrong_address_voteDelegate_cant_vote

 function test_wrong_address_voteDelegate_cant_vote() public {
        address andi = makeAddr("andi");
        vm.startPrank(andi); 
        address voteDelegate_andi = voteDelegateFactory.create();
        vm.stopPrank();

        address urn = engine.open(0);
        mkr.approve(address(engine), 100_000 * 10**18);
        engine.lock(urn, 100_000 * 10**18, 5);
        emit SelectVoteDelegate(urn, voteDelegate_andi);
        engine.selectVoteDelegate(urn, voteDelegate_andi);

        vm.expectRevert("VoteDelegate/sender-not-delegate");
        VoteDelegateMock(voteDelegate_andi).vote(0);

    }

Mitigation

There is a helper function to check the creator or the delegate of the VoteDelegate on the VoteDelegateFactory contract.

 function delegates(address usr) external view returns (address voteDelegate) {
        voteDelegate = getAddress(usr);
        if (created[voteDelegate] == 0) { voteDelegate = address(0); }
    }

So, use that to verify in LockstakeEngine#selectVoteDelegate like this:

  function selectVoteDelegate(
        address urn,
        address voteDelegate
    ) external urnAuth(urn) {
+      address urnOwner = urnOwners[urn];
        require(urnAuctions[urn] == 0, "LockstakeEngine/urn-in-auction");
+        require(
            voteDelegateFactory.delegates(urnOwner) == voteDelegate,
            "Not a Delegate"
        );