QA Report

[code423n4]

[L-01] Admin Address Change should be a 2-Step Process

Summary
The setAdmin function changes an admin account with single process.
This can be a concern since an admin role has a high privilege in this contract and
mistakenly setting a new admin to an incorrect address can end up locking the system.

Mitigation
This can be fixed by implementing 2-step process. We can do this by following.
First make the setAdmin function approve a new address as a pending admin.
Next that pending admin has to claim the ownership in a separate transaction to be a new admin.

Issue found at
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L129-L132
https://github.com/code-423n4/2022-06-illuminate/blob/main/marketplace/MarketPlace.sol#L109-L112
https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L62-L65

[N-01] Overall Naming Convention and Use of a Lowercase Letter "l"

Summary
Single lowercase letter is used for function parameter name throughout this contract.
Even though naming convention is not rules and are guidelines, I still recommend not using
single lowercase letter as a name because it reduce readability and is hard to debug or audit in general.
However I strongly recommend never to use "l" (lowercase letter el) as a name since they are
often indistinguishable from the numerals one.

This "l" is specifically addressed as one of names to avoid in solidity documents.
link: https://docs.soliditylang.org/en/v0.8.15/style-guide.html#names-to-avoid

Issue found at
https://github.com/code-423n4/2022-06-illuminate/blob/main/marketplace/MarketPlace.sol#L53
https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L45
https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L81

[N-02] Best Practices of Source File Layout

Summary
I recommend following best practices of solidity source file layout for readability.
This best practices is to layout a contract in following order: type declarations, state variables, events, modifiers and functions.
link: https://docs.soliditylang.org/en/v0.8.15/style-guide.html#order-of-layout

Issue found at

Lender.sol
Marketplace.sol
Redeemer.sol

[N-03] Event is Missing Indexed Fields

Summary
Each event should have 3 indexed fields if there are 3 or more fields.

Issue found at

./Redeemer.sol:36:    event Redeem(uint8 principal, address indexed underlying, uint256 indexed maturity, uint256 amount);
./Lender.sol:48:    event Lend(uint8 principal, address indexed underlying, uint256 indexed maturity, uint256 returned);
./Lender.sol:50:    event Mint(uint8 principal, address indexed underlying, uint256 indexed maturity, uint256 amount);
./Lender.sol:52:    event ScheduleWithdrawal(address indexed token, uint256 hold);

[N-04] TYPO

Summary
Some typo was found in the following

Issue found at

1. Lender.sol

Change "withdrawl" to "withdrawal"
22: /// @notice minimum amount of time the admin must wait before executing a withdrawl

Change "unsighed" to "unsigned"
83: // max is the maximum integer value for a 256 unsighed integer

Change "gauruntee" to "guarantee"
487: if (token.underlying() != u) { // gauruntee the input token is the right token

Change "remaing" to "remaining"
631: // send the remaing amount to the given yield pool

2. MarketPlace.sol

Change "avaialable" to "available"
11: /// @notice This contract is in charge of managing the avaialable principals for each loan market.

Change "intializes" to "initializes"
50: /// @notice intializes the MarketPlace contract

3. Redeemer.sol

Change "prinicpal" to "principle"
125: // Burn the prinicipal token from Illuminate

Change "prinicpal" to "principle"
189: // Redeems prinicipal tokens from yield

Change "prinicpal" to "principle"
237: /// @param d Sense contract that splits the loan's prinicpal and yield

[N-05] Incomplete NatSpec

It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces.
However there were places where this NatSpec was incomplete.

Issue found at
no @param for aave
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L520-L528

[N-06] NatSpec but No Corresponding Event

Maybe the event was deleted but forgot to delete the NatSpec.
I recommend to delete this NatSpec as well.

Issue found at
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L55