Ethereum Credit Guild - nocoder's results

Lines of code

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

Vulnerability details

Impact

GUILD tokens voting for a gauge cannot be transferred if the gauge is currently using all its debt ceiling. The transfer/transferFrom functions in GuildToken.sol call _decrementGaugeWeight (via a call to _decrementWeightUntilFree) for each gauge a user has voted for. _decrementGaugeWeight intends to confirm that, taking the gauge decrement into account, the debt ceiling is not exceeded - otherwise the transfer of GUILD won't be permitted.

But a miscalculation in LendingTerm.debtCeiling() can lead to a transfer being wrongfully rejected. debtCeiling takes gaugeWeightDelta (the gauge decrement) as a parameter. debtCeiling has a variable gaugeWeight which equals the gauge's pre-existing weight minus gaugeWeightDelta. debtCeiling also has a variable totalWeight equal to the pre-existing weight of all gauges of its type. But, unlike gaugeWeight, totalWeight does not subtract gaugeWeightDelta. gaugeWeight is adjusted as if the decrement has already happened while totalWeight is calculated as if the decrement has not happened.

debtCeiling calculates debtCeilingBefore which = (totalBorrowedCredit * toleratedGaugeWeight) / totalWeight. (Note: toleratedGaugeWeight is just gaugeWeight adjusted by gaugeWeightTolerance). So debtCeilingBefore is not accurate because toleratedGaugeWeight has been decremented by gaugeWeightDelta but totalWeight - which by definition contains gaugeWeight has not been decremented.

This makes debtCeilingBefore larger (since toleratedGaugeWeight in the numerator has been shrunk by gaugeWeightDelta). This can cause the if statement in the next line, which is if (_issuance >= debtCeilingBefore) {return debtCeilingBefore}, to be a false positive and return debtCeilingBefore when it should actually continue and can cause _decrementGaugeWeight to revert (due to its requirement that issuance be less than or equal to what debtCeiling returns), preventing the token transfer.

*debtCeilingBefore probably intends to use the pre-existing gauge weight before subtracting the gaugeWeightDelta.

##Additional Impacts This also causes other problems when _decrementGaugeWeight calls debtCeiling - e.g., like the following else if statement not being triggered when it should be. Let's say there was just one gauge with a weight of 50 and a user wants to decrement it by, say, 20...the following statement should be triggered since there is only one gauge, but gaugeWeight would be 30 and totalWeight would be 50, so this statement wouldn't be triggered. It also can lead to miscalculations of remainingDebtCeiling and otherGaugesWeight.

else if (gaugeWeight == totalWeight) {
            // one gauge, unlimited debt ceiling
            // returns min(hardCap, creditMinterBuffer)
            return
                _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;

 

Proof of Concept

The _decrementGaugeWeight function in GuildToken.sol, which is called before a token transfer is allowed, calls LendingTerm.debtCeiling() with the gauge weight decrease as a parameter link to code:

function _decrementGaugeWeight(
        address user,
        address gauge,
        uint256 weight
    ) internal override {
        uint256 _lastGaugeLoss = lastGaugeLoss[gauge];
        uint256 _lastGaugeLossApplied = lastGaugeLossApplied[gauge][user];
        require(
            _lastGaugeLossApplied >= _lastGaugeLoss,
            "GuildToken: pending loss"
        );

        // update the user profit index and claim rewards
        ProfitManager(profitManager).claimGaugeRewards(user, gauge);

        // check if gauge is currently using its allocated debt ceiling.
        // To decrement gauge weight, guild holders might have to call loans if the debt ceiling is used.
        uint256 issuance = LendingTerm(gauge).issuance();
        if (issuance != 0) {
            uint256 debtCeilingAfterDecrement = LendingTerm(gauge).debtCeiling(-int256(weight));
            require(
                issuance <= debtCeilingAfterDecrement,
                "GuildToken: debt ceiling used"
            );
        }

        super._decrementGaugeWeight(user, gauge, weight);
    }

The debtCeiling function in LendingTerm.sol takes gaugeWeightDelta as a parameter (which in this case is the decrease in weight resulting from the decrement), and it subtracts that gaugeWeightDelta from the variable gaugeWeight but not from the variable totalWeight link to code:

function debtCeiling(
        int256 gaugeWeightDelta
    ) public view returns (uint256) {
        address _guildToken = refs.guildToken; // cached SLOAD
        uint256 gaugeWeight = GuildToken(_guildToken).getGaugeWeight(
            address(this)
        );
        gaugeWeight = uint256(int256(gaugeWeight) + gaugeWeightDelta);
        uint256 gaugeType = GuildToken(_guildToken).gaugeType(address(this));
        uint256 totalWeight = GuildToken(_guildToken).totalTypeWeight(
            gaugeType
        );
        uint256 creditMinterBuffer = RateLimitedMinter(refs.creditMinter)
            .buffer();
        uint256 _hardCap = params.hardCap; // cached SLOAD
        if (gaugeWeight == 0) {
            return 0; // no gauge vote, 0 debt ceiling
        } else if (gaugeWeight == totalWeight) {
            // one gauge, unlimited debt ceiling
            // returns min(hardCap, creditMinterBuffer)
            return
                _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;
        }

Here is the portion of the debtCeiling function where things go awry due to toleratedGaugeWeight being decreased by gaugeWeightDelta but totalWeight not being so decremented link to code:

      uint256 debtCeilingBefore = (totalBorrowedCredit *
          toleratedGaugeWeight) / totalWeight;
      if (_issuance >= debtCeilingBefore) {
          return debtCeilingBefore; // no more borrows allowed
      }
      uint256 remainingDebtCeiling = debtCeilingBefore - _issuance; // always >0
      if (toleratedGaugeWeight >= totalWeight) {
          // if the gauge weight is above 100% when we include tolerance,
          // the gauge relative debt ceilings are not constraining.
          return
              _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;
      }
      uint256 otherGaugesWeight = totalWeight - toleratedGaugeWeight; // always >0
      uint256 maxBorrow = (remainingDebtCeiling * totalWeight) /
          otherGaugesWeight;
      uint256 _debtCeiling = _issuance + maxBorrow;
      // return min(creditMinterBuffer, hardCap, debtCeiling)
      if (creditMinterBuffer < _debtCeiling) {
          return creditMinterBuffer;
      }
      if (_hardCap < _debtCeiling) {
          return _hardCap;
      }
      return _debtCeiling;
  }

Tools Used

Manual review

Recommended Mitigation Steps

Have variables existingGaugeWeight and adjustedGaugeWeight and existingTotalWeight and adjustedTotalWeight so that gaugeWeight and totalWeight are both either their pre-existing amounts or their adjusted amounts, as needed.

Here is what I would do:

function debtCeiling(
        int256 gaugeWeightDelta
    ) public view returns (uint256) {
        address _guildToken = refs.guildToken; // cached SLOAD
        uint256 existingGaugeWeight = GuildToken(_guildToken).getGaugeWeight(
            address(this)
        );
        adjustedGaugeWeight = uint256(int256(existingGaugeWeight) + gaugeWeightDelta);
        uint256 gaugeType = GuildToken(_guildToken).gaugeType(address(this));
        uint256 existingTotalWeight = GuildToken(_guildToken).totalTypeWeight(
            gaugeType
        );
        uint256 adjustedTotalWeight = uint256(int256(existingTotalWeight) + gaugeWeightDelta);

        uint256 creditMinterBuffer = RateLimitedMinter(refs.creditMinter)
            .buffer();
        uint256 _hardCap = params.hardCap; // cached SLOAD
        if (adjustedGaugeWeight == 0) {
            return 0; // no gauge vote, 0 debt ceiling
        } else if (existingGaugeWeight == existingTotalWeight) {
            // one gauge, unlimited debt ceiling
            // returns min(hardCap, creditMinterBuffer)
            return
                _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;
        }
        uint256 _issuance = issuance; // cached SLOAD
        uint256 totalBorrowedCredit = ProfitManager(refs.profitManager)
            .totalBorrowedCredit();
        uint256 gaugeWeightTolerance = ProfitManager(refs.profitManager)
            .gaugeWeightTolerance();
        if (totalBorrowedCredit == 0 && existingGaugeWeight != 0) {
            // first-ever CREDIT mint on a non-zero gauge weight term
            // does not check the relative debt ceilings
            // returns min(hardCap, creditMinterBuffer)
            return
                _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;
        }
        uint256 toleratedExistingGaugeWeight = (existingGaugeWeight * gaugeWeightTolerance) /
            1e18;
        uint256 debtCeilingBefore = (totalBorrowedCredit *
            toleratedExistingGaugeWeight) / existingTotalWeight;
        if (_issuance >= debtCeilingBefore) {
            return debtCeilingBefore; // no more borrows allowed
        }
        uint256 remainingDebtCeiling = debtCeilingBefore - _issuance; // always >0
        uint256 toleratedAdjustedGaugeWeight = (adjustedGaugeWeight * gaugeWeightTolerance) /
            1e18;        
          
            if (toleratedAdjustedGaugeWeight >= adjustedTotalWeight) {
            // if the gauge weight is above 100% when we include tolerance,
            // the gauge relative debt ceilings are not constraining.
            return
                _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;
        }
        uint256 otherGaugesWeight = adjustedTotalWeight - toleratedAdjustedGaugeWeight; // always >0
        uint256 maxBorrow = (remainingDebtCeiling * adjustedTotalWeight) /
            otherGaugesWeight;
        uint256 _debtCeiling = _issuance + maxBorrow;
        // return min(creditMinterBuffer, hardCap, debtCeiling)
        if (creditMinterBuffer < _debtCeiling) {
            return creditMinterBuffer;
        }
        if (_hardCap < _debtCeiling) {
            return _hardCap;
        }
        return _debtCeiling;
    }

Assessed type

DoS