Illuminate contest - rfa's results

QA

Title: admin should be set in two step

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L129-L132

When we try to set the new admin of smart contract, we better to set the pending admin, then we let the pending admin claim the admin role to minimize the risk of falsely inputting the new admin address. The admin role is quite critical for the smart contract.

Recommended mitigation step

Create new function which set the pendingAdmin and let pending admin claim the ownership in a new other function:

    function setPendingAdmin(address a) external authorized(admin) returns (bool) {
        pendingAdmin = a; //@audit-info: new var at storage
        return true;
    }


    function claimAdmin() external {
	require(msg.sender == pendingAdmin);
	admin = msg.sender;
    }

Title: feeNominator should be named feeDenominator

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L681

nominator is not an appropriate term for bottom part of fraction. Using Denominator is more accurate

Reference

https://math.stackexchange.com/questions/159081/numerator-vs-denominator-vs-nominator

Title: Emit event for critical function

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L129-L140

Changing admin and feenominator is critical change in the smart contract. By emitting event on this, it easier for the off chain system to track any changes.

Gas

Title: Using prefix increment can save gas for i

Occurrences: https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L96

Using prefix increment can save 5 gas per loop

Recommended mitigation step

            unchecked {
                ++i;
            }

Title: Unnecessary else statement

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L227

The function already return the value in if() statement above it, so if the condition p == uint8(MarketPlace.Principals.Yield) == true. The code inside else statement won't be executed

Recommended Mitigation step

Remove else statement:

        if (p == uint8(MarketPlace.Principals.Yield)) {
            // Purchase yield PTs to lender.sol (address(this))
            uint256 returned = yield(u, y, a - calculateFee(a), address(this));
            // Mint and distribute equivalent illuminate PTs
            IERC5095(principalToken(u, m)).mint(msg.sender, returned);
            
            emit Lend(p, u, m, returned);

            return returned;
        }
        
            // Purchase illuminate PTs directly to msg.sender
            uint256 returned = yield(u, y, a - calculateFee(a), msg.sender);

            emit Lend(p, u, m, returned);

            return returned;
        

It can save 24 deployment gas fee

Title: Use calldata to store array

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L251

Using calldata pointer to store read only array args can save gas