Ethereum Credit Guild - kaden's results

Lines of code

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

Vulnerability details

Impact

Attackers can drain the ProfitManager by taking advantage of the userGaugeProfitIndex accumulator not getting initially set when first incrementing their gauge weight.

Proof of Concept

In claimGaugeRewards, if a user has yet to claim rewards, their userGaugeProfitIndex is 0, which is converted to 1e18.

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

Upon initially voting for a gauge, it's crucial that the user's gauge weight is set to the current weight, otherwise they can claim the full relative amount of profits since inception at the gaugeProfitIndex of 1e18. We can see how this is the case by seeing how it simply finds the delta between userGaugeProfitIndex and gaugeProfitIndex to determine the reward amount.

uint256 deltaIndex = _gaugeProfitIndex - _userGaugeProfitIndex;
if (deltaIndex != 0) {
    creditEarned = (_userGaugeWeight * deltaIndex) / 1e18;
    userGaugeProfitIndex[user][gauge] = _gaugeProfitIndex;
}
if (creditEarned != 0) {
    emit ClaimRewards(block.timestamp, user, gauge, creditEarned);
    CreditToken(credit).transfer(user, creditEarned);
}

The problem is that GuildToken._incrementGaugeWeight calls claimGaugeRewards before incrementing the gauge weight and if the userGaugeWeight is 0, claimGaugeRewards simply returns 0 instead of setting the necessary state.

// @audit returns early if no gauge weight yet
if (_userGaugeWeight == 0) {
    return 0;
}

As a result, the next time the user claims gauge rewards, they will receive their share of all the profits since inception, rather than just their share of the profits since they first voted for the gauge.

An attacker can simply wait for a gauge to grow its profit index then buy or flashloan a large sum of GUILD, increment their gauge weight on that gauge and atomically claimGaugeRewards (giving them their share of all the past rewards) then decrement and sell or repay the flashloan. This can be repeated for every gauge until the ProfitManager is drained of rewards.

Tools Used

• Manual review

Recommended Mitigation Steps

userGaugeProfitIndex must be updated regardless of whether the user has any gauge weight.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Attackers can transfer CREDIT to the same address they're transferring from to print CREDIT. Can be repeated to inflate the supply by an unlimited amount.

Proof of Concept

When transferring rebasing tokens, we have to take some extra steps to ensure that each participant ends up with the correct amount of tokens. To do this, in ERC20RebaseDistributor.transfer, we take the following steps:

1. Retrieve initial state
2. Settle msg.sender balance
3. Execute underlying transfer
4. Update msg.sender finalized state
5. Update to finalized state
6. Update total rebasing shares

In steps 4 and 5, we calculate the new amount of shares and underlying tokens for msg.sender and to based on the account balances after the underlying transfer, the amount transferred, and the number of shares of the accounts.

// if `from` is rebasing, update its number of shares
int256 sharesDelta;
if (rebasingStateFrom.isRebasing == 1) {
    uint256 fromBalanceAfter = fromBalanceBefore - amount;
    uint256 fromSharesAfter = _balance2shares(
        fromBalanceAfter,
        _rebasingSharePrice
    );
    uint256 sharesSpent = rebasingStateFrom.nShares - fromSharesAfter;
    sharesDelta -= int256(sharesSpent);
    rebasingState[msg.sender] = RebasingState({
        isRebasing: 1,
        nShares: uint248(fromSharesAfter)
    });
}

// if `to` is rebasing, update its number of shares
if (rebasingStateTo.isRebasing == 1) {
    // compute rebased balance
    uint256 rawToBalanceAfter = ERC20.balanceOf(to);
    uint256 toBalanceAfter = _shares2balance(
        rebasingStateTo.nShares,
        _rebasingSharePrice,
        amount,
        rawToBalanceAfter
    );

    // update number of shares
    uint256 toSharesAfter = _balance2shares(
        toBalanceAfter,
        _rebasingSharePrice
    );
    uint256 sharesReceived = toSharesAfter - rebasingStateTo.nShares;
    sharesDelta += int256(sharesReceived);
    rebasingState[to] = RebasingState({
        isRebasing: 1,
        nShares: uint248(toSharesAfter)
    });

    // "realize" unminted rebase rewards
    uint256 mintAmount = toBalanceAfter - rawToBalanceAfter;
    if (mintAmount != 0) {
        ERC20._mint(to, mintAmount);
        decreaseUnmintedRebaseRewards(mintAmount);
        emit RebaseReward(to, block.timestamp, mintAmount);
    }
}

The problem with this logic is that in step 5, we use the to account's rebasing state, rebasingStateTo, which was retrieved in step 1 to compute the updated rebasing state after the transfer. As a result, if the to address is the same as the msg.sender, we will have reduced the sender balance in step 4 but then ignore that reduction in step 5 by using the account state from before the transfer.

This allows an attacker to execute a self-transfer where it will retrieve the account's rebasing state before the transfer then compute its end state using the amount received from the transfer. As a result, there should be no balance change, but instead their number of shares increases according to the amount transferred, which is then used to mint the corresponding amount of underlying tokens. The attacker can repeat this process indefinitely, inflating the supply and redeeming the credit for the peg token in the SimplePSM until the SimplePSM is drained.

Tools Used

• Manual Review

Recommended Mitigation Steps

A simple solution is to simply disallow self-transfers in transfer and transferFrom.

Assessed type

ERC20

Lines of code

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

Vulnerability details

Impact

The lastGaugeLoss for the term to stake/unstake/getRewards for is incorrectly compared to an undefined variable. This causes any non-zero value for lastGaugeLoss, i.e. any gauge loss ever having happened, to result in all positions being slashed, even if they already had been slashed since the lastGaugeLoss.

Proof of Concept

In getRewards, called at the start of stake and unstake, we retrieve the lastGaugeLoss for the given term. If lastGaugeLoss > userStake.lastGaugeLoss, we mark the user as slashed, which causes them to have their stake slashed.

// @audit userStake is empty at this point
if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
    // @audit will be true any time there has been a loss
    slashed = true;
}

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

The problem with this logic is that userStake is undefined until after we do this check. As a result, any time lastGaugeLoss is non-zero, the user will be slashed. In the case that a new user stakes after the lastGaugeLoss, where they should be able to stake and unstake without issue, due to this vulnerability the user will be able to stake but will be unable to unstake and instead will get slashed.

Tools Used

• Manual review

Recommended Mitigation Steps

Define userStake before executing the lastGaugeLoss check:

lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
userStake = _stakes[user][term];
if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
    slashed = true;
}

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

If a LendingTerm is re-onboarded after being offboarded but before being cleaned up with LendingTermOffboarding.cleanup, the canOffboard[term] mapping will be set to true, allowing the LendingTerm to be offboarded at any time by anyone. This can be done to grief borrowers and gauge voters.

Proof of Concept

When a LendingTerm is offboarded, it's important that it goes through the two step process of offboard and cleanup. This is necessary because cleanup marks canOffboard[term] as false, preventing offboarding of the term without a successful proposal.

// LendingTermOffboarding.cleanup()
canOffboard[term] = false;

// LendingTermOffboarding.offboard()
require(canOffboard[term], "LendingTermOffboarding: quorum not met");

Unfortunately, it's reasonably possible that a term gets re-onboarded without first being cleaned up because:

1. cleanup can only be executed after all existing loans are called and auctioned
2. More importantly, onboarding doesn't validate that the term has been cleaned up

As a result, an unclean term could reasonably end up getting re-onboarded, in which case anyone could immediately offboard it at any time, causing loss to term participants.

Tools Used

• Manual review

Recommended Mitigation Steps

LendingTermOnboarding should validate that the proposed term returns false from canOffboard[term].

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

ProfitManager works with only one CREDIT token and there are intended to be multiple CREDIT tokens. However, GuildToken only has reference to one profitManager contract. As a result, GUILD will only work with one ProfitManager, preventing the ability to increment/decrement weight to other CREDIT gauges or claim rewards for gauge voting.

Proof of Concept

We know that there are intended to be multiple different CREDIT tokens in the system. We also know that ProfitManager only works with one CREDIT token. See below from ProfitManager.sol:

/// @notice reference to CREDIT token.
address public credit;

As a result, it's necessary to have several deployed ProfitManager contracts, one for each CREDIT contract.

The problem with this is that GuildToken can only ever reference one profitManager. See below from GuildToken.sol:

/// @notice reference to ProfitManager
address public profitManager;

This means that in its current form, the system can only support one CREDIT token. Since GuildToken is non-upgradeable, a redeployment would be necessary to integrate more CREDIT tokens into the system.

Tools Used

• Manual review

Recommended Mitigation Steps

Instead of a single profitManager, include an array in storage which can only be added to by a trusted party, and allow references to any of those contracts where necessary.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Reasonably large profits, as accounted for in the ProfitManager, can be sandwiched by staking and unstaking in the SurplusGuildManager to take a significant share of the profits without taking on any risk.

Note that the same exploit can be performed by incrementing and decrementing gauge weight via holding the Guild token directly.

Proof of Concept

When a repayment or liquidation of a lending position results in profit for the protocol, the ProfitManager distributes that profit, with a portion of it going to gauge voters according to their gauge weight by incrementing the gaugeProfitIndex.

gaugeProfitIndex[gauge] =
    _gaugeProfitIndex +
    (amountForGuild * 1e18) /
    _gaugeWeight;

Since we're increasing the gaugeProfitIndex in a stepwise fashion, it's possible to sandwich this increase to take a share of the profits without incurring any of the risk of gauge voting.

Consider, for example, a circumstance where the amountForGuild is 10000 tokens, perhaps because a large position was liquidated. An MEV bot monitoring the mempool can see that this is about to occur and execute the following attack:

• Borrow a million peg tokens from Compound or Aave
• Mint CREDIT with the peg tokens in the SimplePSM
• Stake the CREDIT in the SurplusGuildMinter for the gauge which the profit was received from
  • Increases user gauge weight to 1,000,000, increases total gauge weight to e.g. 2,000,000
• Bundle notifyPnL transaction here
• Unstake the CREDIT
  • Attacker receives half the amountForGuild = 5000 tokens
• Redeem CREDIT for peg tokens in the SimplePSM
• Repay loan

Since the attacker only had their stake active for one transaction, which they knew was profitable, their position incurred absolutely 0 risk, stealing the profits from legitimate voters which are actually incurring risk. This disincentivizes actually staking/gauge voting which reduces the debt ceiling of LendingTerms and significantly detrimentally affects the entire protocol.

Tools Used

• Manual review

Recommended Mitigation Steps

Consider either enforcing positions to be locked for a period of time to ensure users all incur risk, or distribute profits over a period of time similarly to how profits are distributed to rebasing CREDIT.

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

The debtCeiling() of LendingTerm's is limited by the creditMinterBuffer. Since creditMinterBuffer represents a remaining amount available to mint while debtCeiling represents an amount available to borrow + total issuance, limiting the debtCeiling to creditMinterBuffer may limit the debtCeiling much more than intended. This can result in some gauge weight from all gauges being locked, preventing gauge voters from decrementing their gauge weight since the debtCeiling will be too low.

Proof of Concept

RateLimitedMinter.buffer() returns the current amount of tokens available to be minted.

function buffer() public view returns (uint256) {
        uint256 elapsed = block.timestamp.safeCastTo32() - lastBufferUsedTime;
        return
                Math.min(bufferStored + (rateLimitPerSecond * elapsed), bufferCap);
}

The buffer is used in LendingTerm.debtCeiling to limit the debtCeiling to be no greater than the buffer. Of course, as mentioned, the problem with this is that the debtCeiling represents the current issuance + the amount of tokens available to mint, while the buffer only represents the amount of tokens available to mint.

When actually borrowing, users can borrow up to the buffer amount, which is as intended. This however, can result in a circumstance where the debtCeiling() returns an incorrect value which is less than the current issuance. In this circumstance, since gauge voters cannot decrement their gauge weight if it results in the debtCeiling < issuance, gauge voters may have their positions unexpectedly stuck.

// From GuildToken._decrementGaugeWeight
if (issuance != 0) {
    uint256 debtCeilingAfterDecrement = LendingTerm(gauge).debtCeiling(-int256(weight));
    require(
        issuance <= debtCeilingAfterDecrement,
        "GuildToken: debt ceiling used"
    );
}

Since the creditMinterBuffer is used throughout every gauge for the given credit, many gauge voters can be DoS'd at the same time.

Tools Used

• Manual review

Recommended Mitigation Steps

debtCeiling should be limited by creditMinterBuffer + issuance rather than just creditMinterBuffer.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

The _hardCap, used to enforce a maximum debt ceiling for the LendingTerm may be exceeded in the case that creditMinterBuffer is greater than the _hardCap and less than the computed _debtCeiling.

Proof of Concept

We can see at the bottom of LendingTerm.debtCeiling in this comment that the intent is to return min(creditMinterBuffer, hardCap, debtCeiling).

// return min(creditMinterBuffer, hardCap, debtCeiling)

However, in the following logic, we're actually just returning the first of creditMinterBuffer and _hardCap to be less than _debtCeiling.

// @audit doesn't return min, just first one less than _debtCeiling
// return min(creditMinterBuffer, hardCap, debtCeiling)
if (creditMinterBuffer < _debtCeiling) {
    return creditMinterBuffer;
}
if (_hardCap < _debtCeiling) {
    return _hardCap;
}
return _debtCeiling;

As we can see above, if creditMinterBuffer < _debtCeiling, we return creditMinterBuffer, even if _hardCap < creditMinterBuffer and would be the correct return value.

Tools Used

• Manual Review

Recommended Mitigation Steps

We can replace the problem code with the following:

// return min(creditMinterBuffer, hardCap, debtCeiling)
if (creditMinterBuffer < _debtCeiling && creditMinterBuffer < _hardCap) {
    return creditMinterBuffer;
}
if (_hardCap < _debtCeiling) {
    return _hardCap;
}
return _debtCeiling;

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The gaugeWeightDelta param, used by LendingTerm.debtCeiling, is used to update the weight of the gauge, but not the totalWeight which is used to compute the debtCeiling based on relative weights. This results in the computed debtCeiling being higher than it should, preventing users from decrementing their gauge weight when they should be able to.

Proof of Concept

We can see in LendingTerm.debtCeiling that we use the gaugeWeightDelta to update the gaugeWeight, but not the totalWeight.

gaugeWeight = uint256(int256(gaugeWeight) + gaugeWeightDelta);
uint256 gaugeType = GuildToken(_guildToken).gaugeType(address(this));
// @audit should be adding the delta to the totalWeight here too
uint256 totalWeight = GuildToken(_guildToken).totalTypeWeight(
     gaugeType
);

The debtCeiling is then computed in part by the relative value of gaugeWeight/totalWeight.

uint256 toleratedGaugeWeight = (gaugeWeight * gaugeWeightTolerance) /
    1e18;
uint256 debtCeilingBefore = (totalBorrowedCredit *
    toleratedGaugeWeight) / totalWeight;

totalWeight should be the sum of the gaugeWeights of the given gauge type, but since we apply the delta to gaugeWeight and not totalWeight, this invariant is broken, causing the debtCeiling to be incorrectly computed.

The debt ceiling is used by GuildToken._decrementGaugeWeight to enforce that the debt ceiling will never be less than the issuance of the gauge.

uint256 issuance = LendingTerm(gauge).issuance();
if (issuance != 0) {
    uint256 debtCeilingAfterDecrement = LendingTerm(gauge).debtCeiling(-int256(weight));
    require(
        issuance <= debtCeilingAfterDecrement,
        "GuildToken: debt ceiling used"
    );
}

Since we have a negative gaugeWeightDelta provided to debtCeiling in _decrementGaugeWeight, we'll reduce the numerator and not the denominator, causing us to underestimate the actual debt ceiling. As a result, _decrementGaugeWeight may revert even when the actual debt ceiling is >= issuance

Tools Used

• Manual review

Recommended Mitigation Steps

Update the totalWeight by the delta just as is done with the gaugeWeight.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

The debt ceiling as calculated by LendingTerm.debtCeiling() doesn't match the specified debt ceiling calculation as is used in LendingTerm._borrow. As a result, debtCeiling() always overestimates the debt ceiling, causing GuildToken._decrementGaugeWeight to succeed when it shouldn't.

Proof of Concept

As described in the comment above debtCeiling(), the debt ceiling is intended to be computed with the following formula.

/// @dev this solves the following equation :
/// borrowAmount + issuance <=
/// (totalBorrowedCredit + borrowAmount) * gaugeWeight * gaugeWeightTolerance / totalWeight / 1e18

i.e. debt ceiling = (totalBorrowedCredit + borrowAmount) * gaugeWeight * gaugeWeightTolerance / totalWeight / 1e18

We can see in _borrow that this is aligned with the implemented logic.

// LendingTerm._borrow()
uint256 _debtCeiling = (GuildToken(refs.guildToken)
    .calculateGaugeAllocation(
        address(this),
        totalBorrowedCredit + borrowAmount
    ) * gaugeWeightTolerance) / 1e18;

// ERC20Gauges.calculateGaugeAllocation()
function calculateGaugeAllocation(
    address gauge,
    uint256 quantity
) external view returns (uint256) {
    if (_deprecatedGauges.contains(gauge)) return 0;

    uint256 total = totalTypeWeight[gaugeType[gauge]];
    if (total == 0) return 0;
    uint256 weight = getGaugeWeight[gauge];

    return (quantity * weight) / total;
}

However, the logic used in LendingTerm.debtCeiling() fails to conform to this formula. Instead, it uses a much more convoluted formula which I've simplified from the code down to: issuance + (((tbc * (gaugeWeight + delta) * 1.2 / totalWeight) - issuance) * totalWeight / (totalWeight - ((gaugeWeight + delta) * 1.2))), which can be further simplified to: issuance + (debt ceiling - issuance) * totalWeight / (gaugeWeight + delta) * 1.2.

We can test this by adding the following lines to the bottom of testBorrowSuccess in LendingTerm.t.sol:

uint256 computedDebtCeiling = term.debtCeiling();
uint256 actualDebtCeiling = (GuildToken(guild)
    .calculateGaugeAllocation(
        address(term),
        profitManager.totalBorrowedCredit() + borrowAmount
    ) * profitManager.gaugeWeightTolerance()) / 1e18;

assertEq(computedDebtCeiling, actualDebtCeiling);

We can see by the output that the computedDebtCeiling is 2e25 while the actualDebtCeiling is just 4.8e22. This is a difference of >400x, which breaks the invariant of gauge voters being unable to decrement their gauge weight if it results in the debt ceiling being less than the term issuance. Breaking this invariant significantly reduces the risk taken by gauge voters and thus applies that risk to the protocol.

Tools Used

• Manual review
• Forge

Recommended Mitigation Steps

We can extract the logic used in calculateGaugeAllocation, making sure to add the gaugeWeightDelta to the weight and total. Then from there we just need to ensure that our other limiters, hardCap and creditMinterBuffer are also used. Additionally, fuzzing and invariant tests should be incorporated to ensure that for all matching variables in debtCeiling and _borrow, that the computed debt ceiling matches.

Assessed type

Math