Ethereum Credit Guild - lsaudit's results

Lines of code

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

Vulnerability details

Impact

The current flow of offboarding is defined as below:

1. We propose to offboard a given LendingTerm by calling proposeOffboard()
2. We support a pool to offboard by calling supportOffboard()
3. When quorum is reached we are able to call offboard() and offboard a Lending Term.

This flow can be, however, disturbed, when we will re-onboard term.

Malicious user can immediately calls offboard() on re-onboarded term and offboard it without voting.

Proof of Concept

Terms can be re-onboarded. This is confirmed in the comment section of LendingTermOnboarding.sol:

File: LendingTermOnboarding.sol

 		// Check that the term is not already active
        // note that terms that have been offboarded in the past can be re-onboarded
        // and won't fail this check. This is intentional, because some terms might be offboarded
        // due to specific market conditions, and it might be desirable to re-onboard them
        // at a later date.
        bool isGauge = GuildToken(guildToken).isGauge(term);

Now, let's take a look at code at LendingTermOffboarding.sol Whenever we call offboard() - mappings canOffboard, userPollVotes and polls are not being reset.

This means, that when we call offboard(term), userPollVotes[msg.sender][snapshotBlock][term] and polls[snapshotBlock][term] still contains users' votes and canOffboard still points out that it reached the quorum.

1. We call offboard(term) on term. term is being offboarded.
2. Function offboard(term) does not clear canOffboard[term], userPollVotes[msg.sender][snapshotBlock][term], polls[snapshotBlock][term] mappings.
3. Now, we re-onboard the term.
4. Since offboard(term) did not clear canOffboard[term] in step 1 - we are able to call offboard(term) once again, on the re-onboarded term.

This implies, that when term was offboarded and re-onboarded, it's possible to immediately offboard it again. We don't need to propose to offboard (calling proposeOffboard()) and vote for that term again, since canOffboard[term] is already set to true. We can immediately call offboard(term) and offboard it.

Please notice, that above scenario assumes, that we do not call cleanup() after calling offboard(). After offboard(), we re-onboard the term(), without calling cleanup() on it.

Since offbord() function is external, everyone can call it. This means that any user can offboard re-onboarded term. Let's consider above scenario.

1. Genuine user proposes to offboard term 0xA: proposeOffboard(0xA)
2. Genuine users supports the offboard and call supportOffboard(0xA) until quorum is reached and canOffboard[0xA] is set to true.
3. Genuine user calls offboard(0xA) and offboards term 0xA.
4. Now, after some time, term 0xA is being re-onboarded.
5. Malicious user immediately calls offboard(0xA) (please notice, that he does not need to call proposeOffboard() and supportOffboard()). Since canOffboard[0xA] was not reset (it points to true) - term 0xA is being offboarded.

This scenario allows malicious user to immediately offboards re-onboarded terms. He does not need to propose them once again (proposeOffboard()) or vote for them (supportOffboard().). He can just immediately call offboard() on the re-onboarded term and that term will be offboarded again.

Tools Used

Manual code review

Recommended Mitigation Steps

Fixing this issue would require to redesign the whole LendingTermOffboarding.sol mechanism. The quickest solution would be to check again if quorum is met in the offboard() function and reset votes whenever offboard() is called. That way it won't be possible to call offboard() twice without voting on the term again. When the same term will be re-onboarded its vote will be 0 and quorum won't be met - thus it won't be possible to call offboard() again.

Assessed type

Other

[QA-01] Functions which update the value do not verify if the value was really changed

Whenever we update/set a new value of some state variable, it's a good practice to make sure that, the value is being indeed updated. In the current implementation of _setQuorum() in GuildGovernor.sol, even when the _quorum won't be changed, function will still emit an QuorumUpdated() event - which might be misleading to the end-user. E.g. let's assume that _quorum = 123. Calling _setQuorum(123) won't update/set any new quorum, but QuorumUpdated() will still be called.

Our recommendation is to add additional require check, to verify, that provided values are indeed different than the current ones:

require(_quorum  != newQuorum, "Nothing to update");

The same issue occurs in multiple of instances: CoreRef.sol: _setCore() LendingTermOffboarding.sol: setQuorum() ProfitManager.sol: setMinBorrow(), setGaugeWeightTolerance(), setProfitSharingConfig() GuildVetoGovernor.sol: _setQuorum(), _updateTimelock() ERC20MultiVotes.sol: _setMaxDelegates() GuildToken.sol: setProfitManager() SimplePSM.sol: setRedemptionsPaused(), setMintRatio(), setRewardRatio()

[QA-02] Lack of address(0) check in setCore() in CoreRef.sol

File: CoreRef.sol

  function setCore(
        address newCore
    ) external onlyCoreRole(CoreRoles.GOVERNOR) {
        _setCore(newCore);
    }

[...]

    function _setCore(address newCore) internal {
        address oldCore = address(_core);
        _core = Core(newCore);

        emit CoreUpdate(oldCore, newCore);
    }

There's no additional check which verifies, if newCore address, passed by the user is not address(0). Perform additional check: require(newCore != address(0), "newCore cannot be addr(0)").

[QA-03] Lack of address(0) check in allowImplementation() in LendingTermOnboarding.sol

File: LendingTermOnboarding.sol

    function allowImplementation(
[...]
        implementations[implementation] = allowed;
[...]

There's no additional check which verifies, if implementation address, passed by the user is not address(0). Perform additional check: require(implementation != address(0), "newCore cannot be addr(0)").

[QA-04] Lack of address(0) check in updateTimelock() in GuildVetoGovernor.sol

File: GuildVetoGovernor.sol

 function updateTimelock(
        address newTimelock
    ) external onlyCoreRole(CoreRoles.GOVERNOR) {
        _updateTimelock(newTimelock);
    }

    function _updateTimelock(address newTimelock) private {
        emit TimelockChange(timelock, newTimelock);
        timelock = newTimelock;
    }

There's no additional check which verifies, if newTimelock address, passed by the user is not address(0). Perform additional check: require(newTimelock != address(0), "newTimelock cannot be addr(0)").

[QA-05] Changing critical parameters should emit an event with both old and new value

File: ProfitManager.sol

function setMinBorrow(
        uint256 newValue
    ) external onlyCoreRole(CoreRoles.GOVERNOR) {
        _minBorrow = newValue;
        emit MinBorrowUpdate(block.timestamp, newValue);
    }

MinBorrowUpdate() should emit both old (before the update) and new (after the update) values. The current implementation emits an event only with a new value.

The same issue occurs in multiple of instances: ProfitManager.sol: setGaugeWeightTolerance(), setProfitSharingConfig(). GuildToken.sol: setProfitManager() SimplePSM.sol: setMintRatio(), setRewardRatio()

[R-01] Rename createRole() in Core.sol

File: Core.sol

/// @notice creates a new role to be maintained
    /// @param role the new role id
    /// @param adminRole the admin role id for `role`
    /// @dev can also be used to update admin of existing role
    function createRole(
        bytes32 role,
        bytes32 adminRole
    ) external onlyRole(CoreRoles.GOVERNOR) {
        _setRoleAdmin(role, adminRole);
    }

As the NatSpec comment states, createRole(): can also be used to update admin of existing role. This suggests, that the name of the function should be changed to: createOrUpdateRole().

[R-02] renounceRole() in Core.sol should be overriden to disallow renouncing CoreRoles.GOVERNOR role

Since Core inherits from AccessControlEnumerable, it implements additional AccessControl functions. One of those functions is renounceRole().

CoreRoles.GOVERNOR is the most important role in the protocol. Renouncing this role basically means that protocol won't be able to execute most of the operation. Our recommendation is to override renounceRole() so that it won't be possible to accidently renounce this role:

    function renounceRole(bytes32 role, address callerConfirmation) public override {
       if (callerConfirmation  == CoreRoles.GOVERNOR && role == CoreRoles.GOVERNOR ) revert("Cannot renounce CoreRoles.GOVERNOR!");
       super.renounceRole(role, callerConfirmation);
    }

[R-03] Minimum delay between proposals of onboarding of a given term cannot be updated

File: LendingTermOnboarding.sol

uint256 public constant MIN_DELAY_BETWEEN_PROPOSALS = 7 days;

The minimum delay between proposals of onboarding of a given term is represented as constant variable: MIN_DELAY_BETWEEN_PROPOSALS. This implies, that this value cannot be updated by anyone. Our recommendation is to change this variable to a non-constant one and implement a function which allows to modify its value in the future.

[R-04] Rename allowImplementation() in LendingTermOnboarding.sol

File: LendingTermOnboarding.sol

/// @notice Allow or disallow a given implemenation
    function allowImplementation(
[...]
        implementations[implementation] = allowed;
[...]

Function allowImplementation() can either allow or disallow a given implementation, while its name suggests that it only allows new implementations. Our recommendation is to change the name of this function to: allowOrDisallowImplementation().

[R-05] Minimum number of CREDIT to stake cannot be updated

File: SurplusGuildMinter.sol

uint256 public constant MIN_STAKE = 1e18;

The minimum number of CREDIT to stake is represented as constant variable: MIN_STAKE. This implies, that this value cannot be updated by anyone. Our recommendation is to change this variable to a non-constant one and implement a function which allows to modify its value in the future.

[R-06] Some timestamp-related fields from events can be removed

There's no need to pass block.timestamp as event parameter, since this info can be get directly from off-chain (we can see when exactly event was emitted). There are multiple of events which implements timestamp or when parameter and then pass block.timestamp. These fields can be removed, since there's no need to pass them into events.

LendingTermOffboarding.sol:
    event OffboardSupport(uint256 indexed timestamp, [...]
    event Offboard(uint256 indexed timestamp, address indexed term);
    event Cleanup(uint256 indexed timestamp, address indexed term);

LendingTermOnboarding.sol:
    event ImplementationAllowChanged(uint256 indexed when, [...]
    event TermCreated(uint256 indexed when, [...]

ProfitManager.sol:
    event GaugePnL(address indexed gauge, uint256 indexed when, int256 pnl);
    event SurplusBufferUpdate(uint256 indexed when, uint256 newValue);
    event TermSurplusBufferUpdate(uint256 indexed when, [...]
    event CreditMultiplierUpdate(uint256 indexed when, uint256 newValue);
    event ProfitSharingConfigUpdate(uint256 indexed when, [...]
    event ClaimRewards(uint256 indexed when, [...] 
    event MinBorrowUpdate(uint256 indexed when, uint256 newValue);    
    event GaugeWeightToleranceUpdate(uint256 indexed when, uint256 newValue);

ERC20RebaseDistributor.sol:
    event RebaseEnter(address indexed account, uint256 indexed timestamp);
    event RebaseExit(address indexed account, uint256 indexed timestamp);
    event RebaseDistribution(address indexed source, uint256 indexed timestamp, [...]    
    event RebaseReward(address indexed account, uint256 indexed timestamp, [...]

GuildToken.sol:
	event GaugeLoss(address indexed gauge, uint256 indexed when);
	event GaugeLossApply([...],  uint256 when);
	event TransfersEnabled(uint256 block, uint256 timestamp);
	event ProfitManagerUpdated(uint256 timestamp, address newValue);

AuctionHouse.sol:
	event AuctionStart(uint256 indexed when, [...]
	event AuctionEnd(uint256 indexed when, [...]    

LendingTerm.sol:
	event LoanOpen(uint256 indexed when, [...]
	event LoanCall(uint256 indexed when, bytes32 indexed loanId);
	event LoanClose( uint256 indexed when, [...]
	event LoanAddCollateral(uint256 indexed when,, [...]
	event LoanPartialRepay(uint256 indexed when, [...]

SimplePSM.sol:
	event Mint(uint256 indexed when, [...]
	event RedemptionsPaused(uint256 indexed when, bool status);   

SurplusGuildMinter.sol:
	event Stake(uint256 indexed timestamp, [...]
	event Unstake(uint256 indexed timestamp, [...]
	event GuildReward(uint256 indexed timestamp, [...]
	event MintRatioUpdate(uint256 indexed timestamp, uint256 ratio);
	event RewardRatioUpdate(uint256 indexed timestamp, uint256 ratio);

[NC-01] Sort roles in an alphabetical order in Core.sol

File: Core.sol

 _setRoleAdmin(CoreRoles.GOVERNOR, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.GUARDIAN, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.CREDIT_MINTER, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.RATE_LIMITED_CREDIT_MINTER, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.GUILD_MINTER, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.RATE_LIMITED_GUILD_MINTER, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.GAUGE_ADD, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.GAUGE_REMOVE, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.GAUGE_PARAMETERS, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.GAUGE_PNL_NOTIFIER, CoreRoles.GOVERNOR);
        _setRoleAdmin(
            CoreRoles.GUILD_GOVERNANCE_PARAMETERS,
            CoreRoles.GOVERNOR
        );
        _setRoleAdmin(
            CoreRoles.GUILD_SURPLUS_BUFFER_WITHDRAW,
            CoreRoles.GOVERNOR
        );
        _setRoleAdmin(
            CoreRoles.CREDIT_GOVERNANCE_PARAMETERS,
            CoreRoles.GOVERNOR
        );
        _setRoleAdmin(CoreRoles.CREDIT_REBASE_PARAMETERS, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.TIMELOCK_PROPOSER, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.TIMELOCK_EXECUTOR, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.TIMELOCK_CANCELLER, CoreRoles.GOVERNOR);
        [...]

constructor() sets multiple of roles by calling _setRoleAdmin() function. Since multiple of roles are being set at once, and the order of _setRoleAdmin() does not matter, to increase the code readability, our recommendation is to set those roles in an alphabetical order.

E.g., in the current code-base, the very similar roles RATE_LIMITED_CREDIT_MINTER, RATE_LIMITED_GUILD_MINTER, are separated by _setRoleAdmin(CoreRoles.GUILD_MINTER, CoreRoles.GOVERNOR);. Sorting those roles in an alphabetical order would allow to avoid that issue and make the code readability clearer:

_setRoleAdmin(CoreRoles.CREDIT_GOVERNANCE_PARAMETERS,CoreRoles.GOVERNOR);
_setRoleAdmin(CoreRoles.CREDIT_MINTER, CoreRoles.GOVERNOR);
_setRoleAdmin(CoreRoles.CREDIT_REBASE_PARAMETERS, CoreRoles.GOVERNOR);
_setRoleAdmin(CoreRoles.GAUGE_ADD, CoreRoles.GOVERNOR);
_setRoleAdmin(CoreRoles.GAUGE_PARAMETERS, CoreRoles.GOVERNOR);
_setRoleAdmin(CoreRoles.GAUGE_PNL_NOTIFIER, CoreRoles.GOVERNOR);
_setRoleAdmin(CoreRoles.GAUGE_REMOVE, CoreRoles.GOVERNOR);
_setRoleAdmin(CoreRoles.GOVERNOR, CoreRoles.GOVERNOR);
_setRoleAdmin(CoreRoles.GUARDIAN, CoreRoles.GOVERNOR);
_setRoleAdmin(CoreRoles.GUILD_GOVERNANCE_PARAMETERS, CoreRoles.GOVERNOR);
_setRoleAdmin(CoreRoles.GUILD_MINTER, CoreRoles.GOVERNOR);
_setRoleAdmin(CoreRoles.GUILD_SURPLUS_BUFFER_WITHDRAW, CoreRoles.GOVERNOR);
_setRoleAdmin(CoreRoles.RATE_LIMITED_CREDIT_MINTER, CoreRoles.GOVERNOR);
_setRoleAdmin(CoreRoles.RATE_LIMITED_GUILD_MINTER, CoreRoles.GOVERNOR);
_setRoleAdmin(CoreRoles.TIMELOCK_CANCELLER, CoreRoles.GOVERNOR);
_setRoleAdmin(CoreRoles.TIMELOCK_EXECUTOR, CoreRoles.GOVERNOR);
_setRoleAdmin(CoreRoles.TIMELOCK_PROPOSER, CoreRoles.GOVERNOR);

The same issue occurs in CoreRoles.sol. Defined roles:

[...]
 bytes32 internal constant CREDIT_MINTER = keccak256("CREDIT_MINTER_ROLE");

    /// @notice can mint CREDIT within rate limits & cap
    bytes32 internal constant RATE_LIMITED_CREDIT_MINTER =
        keccak256("RATE_LIMITED_CREDIT_MINTER_ROLE");

    /// @notice can mint GUILD arbitrarily
    bytes32 internal constant GUILD_MINTER = keccak256("GUILD_MINTER_ROLE");

    /// @notice can mint GUILD within rate limits & cap
    bytes32 internal constant RATE_LIMITED_GUILD_MINTER =
        keccak256("RATE_LIMITED_GUILD_MINTER_ROLE");
[...]

should be re-ordered alphabetically.

[NC-02] Add keccak256 values to roles in CoreRoles.sol

It's a good practice which increases the code-readability when keccak256 from constants are pre-calculated and inserted as comments. E.g. below line:

File: CoreRoles.sol

/// @notice can manage add new gauges to the system
    bytes32 internal constant GAUGE_ADD = keccak256("GAUGE_ADD_ROLE");

can be changed to:

/// @notice can manage add new gauges to the system
/// bytes32 internal constant GAUGE_ADD = 0xf213c52f17fbe7c12e60dedf809e77244577de25e041307d406900a545756831;
    bytes32 internal constant GAUGE_ADD = keccak256("GAUGE_ADD_ROLE");

[NC-03] Incorrect Markdown formatting in README.md

File: README.md

| [`governance/GuildTimelockController.sol`](https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/GuildTimelockController.sol| 27 |

Markdown syntax does not contain closing ), which means that the hyperlink is not properly rendered. Above code should be changed to:

| [`governance/GuildTimelockController.sol`](https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/GuildTimelockController.sol)| 27 |

[NC-04] Use previously defined constants instead of magic-numbers

File: LendingTermOnboarding.sol

 // 31557601 comes from the constant LendingTerm.YEAR() + 1
            params.maxDelayBetweenPartialRepay < 31557601, // periodic payment every [0, 1 year]

Use previously defined LendingTerm.YEAR() + 1, instead of magic-number: 31557601. Please notice, that @src/loan/LendingTerm.sol is already imported in LendingTermOnboarding.sol, which implies, we can use LendingTerm.YEAR() straightforwardly in the code-base.

[NC-05] Use camel-case syntax for variable naming in GuildVetoGovernor.sol

Some of the variables does not use camel-case syntax.

File: GuildVetoGovernor.sol
175:	ProposalVote storage proposalvote = _proposalVotes[proposalId]; 
// should be: ProposalVote storage proposalVote = _proposalVotes[proposalId]; 

202:	ProposalVote storage proposalvote = _proposalVotes[proposalId];
// should be: ProposalVote storage proposalVote = _proposalVotes[proposalId];

250:	bytes32 queueid = _timelockIds[proposalId];
// should be: bytes32 queueId = _timelockIds[proposalId];

[NC-06] require message should not be empty

Use descriptive messages which will explain why the function reverted.

File: ERC20MultiVotes.sol

require(signer != address(0));

[NC-07] Comment in LendingTerm.sol does not properly explain YEAR value

File: LendingTerm.sol

/// @notice reference number of seconds in 1 year
uint256 public constant YEAR = 31557600;

The comment suggests, that YEAR constant should be 365 * 24 * 60 * 60, which is 31536000. However, the value of YEAR is 31557600, because it takes into a consideration a leap year. This means, that the calculations are: 365.25 * 24 * 60 * 60, which indeed is 31557600. The comment should be more descriptive about the YEAR value (it should explain, that it takes into consideration a leap year).

[NC-08] Typos

File: SurplusGuildMinter.sol

// if the user has been slashed or isnt staking, there is nothing to do

isnt should be either is not or isn't.