Ethereum Credit Guild - HighDuty's results

Lines of code

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

Vulnerability details

Impact

ProfitManager is responsible for allocating and distributing rewards to all parties in the system, including GUILD holders, which participate in gauge support and earn rewards on payed interest. The contract has a function to claim rewards pending for given gauge, but it never check when the participant has allocated his funds to the given gauge. This can result in allocating weight after a profit for the term and getting the reward, which another staker deserves. Worse case is when GUILD transferability is enabled and a user, which owns 10% of the allocated tokens for the term can withdraw rewards for the other 90% of the profit by transferring his tokens to another address owned by him, allocating weight to this gauge and claiming reward. This would result stealing reward funds, which are meant for early gauge supporters

• This could also block SurplusGuildMinter functionality if a malicious user owns some GUILD tokens and perform the scenario from above. ProfitManager won't have enough funds to send to SurplusGuildMinter when calling getRewards , which will block stake/unstake

(uint256 lastGaugeLoss, UserStake memory userStake, ) = getRewards(
            msg.sender,
            term
        );

1 -> 2 -> 3 -> potential revert

Proof of Concept

• Coded PoC, which should be placed inside test/unit/governance/ProfitManager.t.sol and executed with forge test --match-test testProfitDistributionRewardsStealing -vv

  https://gist.github.com/NicolaMirchev/66f6fa840485e899164401b9c9386e73

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

• Implement logic, which check the timestamp a user has allocated weight to a given gauge and a state variable, which stores the time a profit has been accrued. This way if a weight is allocated after a gauge win, there won't be rewards for the new stake.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

SurplusGuildMinter provide functionality to users to stake their CREDIT tokens and in return mint GUILD tokens and allocate corresponding weight to a given gauge. Before any operation getRewards() function is called to claim all standing rewards for the given user, before his stake/unstake. We can note something very suspicious inside first lines of getRewards function regarding checking if the user should be marked as slashed. We compare last loss for the given gauge with the default value of unit256, because the variable userStake is still empty:

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;
        }

        // if the user is not staking, do nothing
        userStake = _stakes[user][term]; 
        ...

We can see that we fetch userStake variable from storage after checking lastGaugeLoss > uint256(userStake.lastGaugeLoss).

• This means that when a loss is accrued and has been applied. All followed stakes which are healthy would be marked as slashed and as a result participants won’t receive their GUILD tokens as a reward
• Users, which has entered the system after the loss has been applied would lose their funds, because they won’t be able to unstake:

function unstake(address term, uint256 amount) external {
        // apply pending rewards
        (, UserStake memory userStake, bool slashed) = getRewards(
            msg.sender,
            term
        );

        // if the user has been slashed, there is nothing to do
        if (slashed) return;
        ...

• userStake for a user, who has entered the system after the loss would be set to 0, which will forever delete his stake, because of those lines inside getRewards:

if (slashed) {
            emit Unstake(block.timestamp, term, uint256(userStake.credit));
            userStake = UserStake({
                stakeTime: uint48(0),
                lastGaugeLoss: uint48(0),
                profitIndex: uint160(0),
                credit: uint128(0),
                guild: uint128(0)
            });
            updateState = true;
        }

        // store the updated stake, if needed
        if (updateState) {
            _stakes[user][term] = userStake;
        }

Proof of Concept

Run the following test inside test/unit/loan/SurplusGuildMinter.t.sol with command forge test --match-test testUnstakeWithLoss -vv

https://gist.github.com/cholakovvv/e8514283ad7bd8bbbd2da87bd7bb0b1e

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

First assign UserStake memory userStake to the storage variable for the given user are then compare it with lastGaugeLoss

• NOTE that you should also implement a logic to assign value to userStake.lastGaugeLoss somehow. Currently this field is never assigned to a value different from 0 in the whole contract, which leads to mistakes.
• Only the following change won’t solve the problem, because you never set the field. But you should think of a good way how to interpret this field, when stakers enter the system after a loss and also update it when staker were in the system when you slash him.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

To offboard a term GUILD holders should agree on that. After a offboard is accepted, the gauge is removed from active gauges list and redemption inside SimplePSM is paused until all loans are paid. To unpause the redemption, LendingTermOffboarding::cleanup should be called. But we can note that it is a valid scenario to re-onboard a term, before all conditions for cleanup are met. But lets examine what would be the consequences from such an action.

1. When offboard is called nOffboardingsInProgress is incremented by 1 and SimplePSM redemption will be paused as long as nOffboardingsInProgress > 0

2. But if a term is re-onboarded, before all his loans has been repaid, or nobody has called cleanup function we can notice another concern:

   2.1 To offboard a term we need canOffboard[term] to be true, which is set back to false inside cleanup function, which has never been called

   2.2 This means that after re-onboarding a single person can offboard it again by simply calling offboard

   2.3 Which would lead to the worst impact, which is incrementing nOffboardingsInProgress again for the same lending term. This means that now it is impossible to set nOffboardingsInProgress back to 0, because cleanup can be called only once for this term, which will decrement progress variable by only 1. The result is constantly paused SimplePSM and blocked funds for stakers.

   NOTE there is a way for community to vote on unpausing the PSM, but this would take a lot of time, during which all PSM functionalities (mint/redeem) would be blocked and even after it’s unblocking, when another term is off-boarded, we again enter in long pause, which is only changeable after long GOV vote and Timelock waiting period.

• Impact is inconsistencies between important state variables inside LendingTermOffboarding and blocked functionality and funds of SimplePSM

Proof of Concept

I have provided instructions in the following gist on how and where to run the coded PoC.

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

• One solution is to reset canOffboard for the term, which is being re-onboarded, when this happens. The implementation may be to implement a logic, which would check canOffboard when a proposal execute is called and based on that to remove it and decrement pending offboardings, or do nothing

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/AuctionHouse.sol#L118

Vulnerability details

Impact

In combination with USDT/USDC blacklist and block stuffing a malicious user can intentionally generate bad debt for successful term and slash all GUILD holders as a result.

Tokens like USDT and USDC have functions that allow them to blacklist an address. The consequence of this action is that a blacklisted user can no longer transfer or receive tokens, which will make the first phase of the auction always revert when trying to send the remaining collateral to the borrower, which will completely block the first phase.

After that when the midPoint passes, the malicious user can delay the second phase making use of block stuffing spaming the network with transactions for one or more blocks, to delay the debt repayment from active participants of the protocol. The result of this action would generate bad debt, which will slash all GUILD holders no matter if this is the best lending term out there and GUILD liquidity is worth a lot. This is huge loss of participants capital, without any real reason.

Proof of Concept

Coded PoC, which should be placed inside test/unit/loan/AuctionHouse.t.sol and executed with forge test --match-test testSlashGUILDTermBlacklistPlusBlockStuffing -vv

https://gist.github.com/NicolaMirchev/3a9d1cb926c6239493980c92136e5da8

Tools Used

Manual review

Recommended Mitigation Steps

• Remove the transfer to the borrower inside LendingTerm::onBid so this function won’t be dependant on external ERC20 logic(blacklists)
• You can introduce a state variable mapping(address -> uint256) collateralToBeRepayed or other name and a function ``withdrawRepayedLoanCollatelwhich will send the pending reward to the msg.sender` based on the mapping and decrement it.
• The change inside onBid function may look like this:

// send collateral to borrower
        if (collateralToBorrower != 0) {
-             IERC20(params.collateralToken).safeTransfer(
-               loans[loanId].borrower,
-               collateralToBorrower
-            );
+              collateralToBeRepayed(loans[loanId].borrower) += collateralToBorrower;
        }





## Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

Lets examine how if statement can be skipped and the for loop inside _decrementWeightUntilFree would stuck, because of the i increment inside if statement.

      address gauge = gaugeList[i];
            uint256 userGaugeWeight = getUserGaugeWeight[user][gauge];
            if (userGaugeWeight != 0) {
                userFreed += userGaugeWeight;
                _decrementGaugeWeight(user, gauge, userGaugeWeight);

                // If the gauge is live (not deprecated), include its weight in the total to remove
                if (!_deprecatedGauges.contains(gauge)) {
                    totalTypeWeight[gaugeType[gauge]] -= userGaugeWeight;
                    totalFreed += userGaugeWeight;
                }

                unchecked {
                    ++i;
                }
            }

• If we have a gauge inside gaugeList, which weight is 0, if branch will be skipped
• A user can call incrementGauge with 0 amount, which will add this gauge to a user's gauge list with the value of 0.
• If this is the first gauge in user's list, then transfer/transferFrom/burn would be blocked, when userFreeWeight < weight
• This is wrong behaviour for the system and a leak point, which could lead to major impacts in future.

Proof of Concept

User allocating 0 amount to a gauge -> transfer/burn tokens, when amount is > all allocated weight for user -> stucked loop

Tools Used

Manual Review

Recommended Mitigation Steps

Move i increment outside the if:

        for (
            uint256 i = 0;
            i < size && (userFreeWeight + userFreed) < weight;

        ) {
            address gauge = gaugeList[i];
            uint256 userGaugeWeight = getUserGaugeWeight[user][gauge];
            if (userGaugeWeight != 0) {
                userFreed += userGaugeWeight;
                _decrementGaugeWeight(user, gauge, userGaugeWeight);

                // If the gauge is live (not deprecated), include its weight in the total to remove
                if (!_deprecatedGauges.contains(gauge)) {
                    totalTypeWeight[gaugeType[gauge]] -= userGaugeWeight;
                    totalFreed += userGaugeWeight;
                }

-                unchecked {
-                    ++i;
-                }
            }
        }
       
        totalWeight -= totalFreed;
+        unchecked {
+                   ++i;
+                }
    }

Assessed type

Token-Transfer