If there are different offboard proccess at the same time user can reuse his vote to vote in severals procces,

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L116

Vulnerability details

The supportOffboar function is just checking if the userWeight is equal to zero but not if the user already vote in the same snapshotBlock.

  function supportOffboard(
        uint256 snapshotBlock,
        address term
    ) external whenNotPaused {
        ...
        uint256 userWeight = GuildToken(guildToken).getPastVotes(  
            msg.sender,
            snapshotBlock
        );
        require(userWeight != 0, "LendingTermOffboarding: zero weight");    <---------
        require(
            userPollVotes[msg.sender][snapshotBlock][term] == 0,
            "LendingTermOffboarding: already voted"
        );

        userPollVotes[msg.sender][snapshotBlock][term] = userWeight;
        polls[snapshotBlock][term] = _weight + userWeight;
        if (_weight + userWeight >= quorum) {
            canOffboard[term] = true;
        }
       ...
    }

[Link].

This behavior is speccificaly in the LendingTermOffboarding.sol contract

Impact

User can vote with his maximus weigth at the same time in different off boarding procces is this offboarding procces was created at the same time.

Note that two or more offboarding procces can be started at the same time for a bad actor that know about the vulnerability and succesfully large holders can vote twice messing up the protocol, reusing his vote and offboarding more term that it should be.

Proof of Concept

The next POC is demostrating a user triple voting in the same timestamp, Run the test foundry in the file:2023-12-ethereumcreditguild/test/unit/governance/LendingTermOffboarding.t.sol

    function testVoteTwiceSameSnapshop() public {
        //minting tokens and delegating
        guild.mint(bob, _QUORUM);
        vm.prank(bob);
        guild.delegate(bob);

        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 1);

        //creating terms..
        LendingTerm term1 = LendingTerm(Clones.clone(address(new LendingTerm())));
        term1.initialize(
            address(core),
            LendingTerm.LendingTermReferences({
                profitManager: address(profitManager),
                guildToken: address(guild),
                auctionHouse: address(auctionHouse),
                creditMinter: address(rlcm),
                creditToken: address(credit)
            }),
            LendingTerm.LendingTermParams({
                collateralToken: address(collateral),
                maxDebtPerCollateralToken: _CREDIT_PER_COLLATERAL_TOKEN,
                interestRate: _INTEREST_RATE,
                maxDelayBetweenPartialRepay: 0,
                minPartialRepayPercent: 0,
                openingFee: 0,
                hardCap: _HARDCAP
            })
        );

        LendingTerm term2 = LendingTerm(Clones.clone(address(new LendingTerm())));
        term2.initialize(
            address(core),
            LendingTerm.LendingTermReferences({
                profitManager: address(profitManager),
                guildToken: address(guild),
                auctionHouse: address(auctionHouse),
                creditMinter: address(rlcm),
                creditToken: address(credit)
            }),
            LendingTerm.LendingTermParams({
                collateralToken: address(collateral),
                maxDebtPerCollateralToken: _CREDIT_PER_COLLATERAL_TOKEN,
                interestRate: _INTEREST_RATE,
                maxDelayBetweenPartialRepay: 0,
                minPartialRepayPercent: 0,
                openingFee: 0,
                hardCap: _HARDCAP
            })
        );

        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 1);
        //Adding gauge

        guild.addGauge(1, address(term1));
        guild.addGauge(1, address(term2));

        uint256 snapshotBlock = block.number;

        // Proposing offboard
        offboarder.proposeOffboard(address(term));
        offboarder.proposeOffboard(address(term1));
        offboarder.proposeOffboard(address(term2));

        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 1);

        //triple voting

        vm.startPrank(bob);
        offboarder.supportOffboard(snapshotBlock, address(term));
        offboarder.supportOffboard(snapshotBlock, address(term1));
        offboarder.supportOffboard(snapshotBlock, address(term2));

    }

Tools Used

Manual, Foundry

Recommended Mitigation Steps

Consider add a check if user already vote in the same snapshop to prevent users to reuse his vote in the same snapshop:

bool public userVoteAlready;

  function supportOffboard(
        uint256 snapshotBlock,
        address term
    ) external whenNotPaused {
        ...
   
        require(userVoteAlready == false, "user already vote");  
        ...
        userVoteAlready = true;
    }

Assessed type

Other

[0xSorryNotSorry]

require(
            userPollVotes[msg.sender][snapshotBlock][term] == 0,
            "LendingTermOffboarding: already voted"
        );

[c4-pre-sort]

0xSorryNotSorry marked the issue as insufficient quality report

[c4-judge]

Trumpero marked the issue as unsatisfactory:
Invalid

[jorgect207]

@Trumpero thank so much, for review my findings.

As you can read in my Impact User can vote with his maximus weight at the same time in different off boarding process is this offboarding process was created at the same time., what i mean by that is that user can vote in different offboarding(different term) process at the same time as long as the offboarding process were be create at the same snapshotBlock.

require(
            userPollVotes[msg.sender][snapshotBlock][term] == 0,
            "LendingTermOffboarding: already voted"
        );

The above require is just checking if the msg.sender already vote in at specific term, and at specific block. And normally this could be good, but in the etherium credit project user can create easily different offboarding process in different terms, as i say in the last of my impact Note that two or more offboarding process can be started at the same time for a bad actor that know about the vulnerability and successfully large holders can vote twice messing up the protocol, reusing his vote and offboarding more term that it should be.

[Trumpero]

@jorgect207 I don't see any reason why a user should be unable to vote for different off-boarding proposals. Forwarding to the natspec of LendingTermOffboarding:

/// This contract works somewhat similarly to a Veto governor: any GUILD holder can poll for the removal
/// of a lending term, and if enough GUILD holders vote for a removal poll, the term can be offboarded
/// without delay.