Illuminate contest - 0x29A's results

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/ERC5095.sol#L105-L119

Vulnerability details

Impact

Anyone can redeem without approval of the owner of the token

Proof of Concept

In this line: require(_allowance[holder][msg.sender] >= underlyingAmount, 'not enough approvals');, the underlyingAmount it's always 0, the require never revert and anyone can get the tokens

Tools Used

Review

Recommended Mitigation Steps

On L116, I recommend that change underlyingAmount to principalAmount

require(_allowance[holder][msg.sender] >= principalAmount, 'not enough approvals');

See the Low-risk vulnerability (submited on my QA And low): [L-01] Repeated code See the Medium-risk vulnerability: [M-01] The allowance does not decrease

With all change:

• Low-risk vulnerability (submited on my QA And low): [L-01] Repeated code
• Medium-risk vulnerability (submited on my QA And low): [M-01] The allowance does not decrease
• High-risk vulnerability: [H-01] Anyone can redeem tokens without approval

    /// @notice At or after maturity, Burns principalAmount from `owner` and sends exactly `underlyingAmount` of underlying tokens to `receiver`.
    /// @param underlyingAmount The amount of underlying tokens withdrawn
    /// @param receiver The receiver of the underlying tokens being withdrawn
    /// @return principalAmount The amount of principal tokens burnt by the withdrawal
    function withdraw(uint256 underlyingAmount, address receiver, address holder) external override returns (uint256 principalAmount) {
        return _redeem(underlyingAmount, receiver, holder);
    }

    /// @notice At or after maturity, burns exactly `principalAmount` of Principal Tokens from `owner` and sends underlyingAmount of underlying tokens to `receiver`.
    /// @param receiver The receiver of the underlying tokens being withdrawn
    /// @return underlyingAmount The amount of underlying tokens distributed by the redemption
    function redeem(uint256 principalAmount, address receiver, address holder) external override returns (uint256 underlyingAmount) {
        return _redeem(principalAmount, receiver, holder);
    }

    function _redeem(uint256 amount, address receiver, address holder) private returns (uint256) {
        if (block.timestamp < maturity) {
            revert Maturity(maturity);
        }
        if (holder == msg.sender) {
            return IRedeemer(redeemer).authRedeem(underlying, maturity, msg.sender, receiver, amount);
        }
        else {
            _decreaseAllowance(holder, amount);
            return IRedeemer(redeemer).authRedeem(underlying, maturity, holder, receiver, amount);
        }
    }

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/ERC5095.sol#L88-L119

Vulnerability details

Impact

When using the withdraw and redeem functions the _allowance, in both, don't decrease

Proof of Concept

L99-L102 and L115-L118: In these else don't decrease _allowance[holder][msg.sender]

Tools Used

Review

Recommended Mitigation Steps

Decrease the _allowance in both functions with _decreaseAllowance(address src, uint wad):

function withdraw(uint256 underlyingAmount, address receiver, address holder) external override returns (uint256 principalAmount){
    if (block.timestamp < maturity) {
        revert Maturity(maturity);
    }
    if (holder == msg.sender) {
        return IRedeemer(redeemer).authRedeem(underlying, maturity, msg.sender, receiver, underlyingAmount);
    }
    else {
+       _decreaseAllowance(holder, underlyingAmount);
        return IRedeemer(redeemer).authRedeem(underlying, maturity, holder, receiver, underlyingAmount);
    }
}

function redeem(uint256 principalAmount, address receiver, address holder) external override returns (uint256 underlyingAmount){
    if (block.timestamp < maturity) {
        revert Maturity(maturity);
    }
    if (holder == msg.sender) {
        return IRedeemer(redeemer).authRedeem(underlying, maturity, msg.sender, receiver, principalAmount);
    }
    else {
+       _decreaseAllowance(holder, principalAmount);
        return IRedeemer(redeemer).authRedeem(underlying, maturity, holder, receiver, principalAmount);
    }
}

See the Low-risk vulnerability: [L-01] Repeated code See the High-risk vulnerability: [H-01] Anyone can redeem tokens without approval

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/redeemer/Redeemer.sol#L97-L151

Vulnerability details

Impact

Anyone with balance of principal token can burn the tokens of the controller and transfer tokens from the lender

Proof of Concept

In the L120 get the balance of the owner, but in the line L126 burn the get balance but from the controller and in the L128 transfer from lender to this and not to the user like the comment says // Transfer the original underlying token back to the user

Tools Used

Manual Review

Recommended Mitigation Steps

The function does not behave according to the natspec documentation I propose this solution based on what I understand you want to do, you should review what really want to do: L116-L131:

if (enumValue == uint8(MarketPlace.Principals.Illuminate)) {
    // Get Illuminate's principal token
    IERC5095 token = IERC5095(principal);
    // Get the amount of tokens to be redeemed from the sender
    uint256 amount = token.balanceOf(msg.sender);
    // Make sure the market has matured
    if (block.timestamp < token.maturity()) {
        revert Invalid('not matured');
    }
    // Burn the prinicipal token from Illuminate
    token.burn(msg.sender, amount);
    // Transfer the original underlying token back to the user
    Safe.transferFrom(IERC20(redeemToken), lender, msg.sender, amount);

    emit Redeem(0, redeemToken, maturity, amount);
}

Non-critical

[N-01] UNUSED LIBRARY

The lender/solmatelib.sol it not used, remove this file

[N-02] MISSING ERROR MESSAGE IN REQUIRE STATMENT

The require in lender/Cast.sol, L09 dont have error message

[N-03] Any is address

In solidity the address data type the equal to Any In lender/Interfaces.sol, L08 don't use an interface like address, use directly address. This affect the code readability, this used in lender/Element.sol, L21, L22 and lender/Lender.sol, L357, L358, L465 files

[N-04] Use representative names for variables

This upgrade the code readability and quality, files include:

• lender/cast.sol
• lender/Lender.sol
• lender/Safe.sol
• marketplace/MarketPlace.sol
• marketplace/Safe.sol
• redeemer/Redeemer.sol
• redeemer/Safe.sol

[N-05] Wrong documentation

In lender/Lender.sol, L153: feenominator should be swivel

Low Risk

[L-01] Repeated code

In ERC5095.sol, L88-L119 The code on the withdraw and redeem functions are similar(almost the same), unify this functions in one

See the Medium-risk vulnerability: [M-01] The allowance does not decrease See the High-risk vulnerability: [H-01] Anyone can redeem tokens without approval

[L-02] USE STANDARDS LIBRERIES

Use @openzeppelin or @rari-capital/solmate to clarify code avoid mistake and don't repeat logic, PLEASE

The marketplace/ERC20.sol implementation don't have severals improves of gas and good practices:

• L32, L33, L34: Don't initialize the name, symbol and decimals variables in the contract, these are initialize in the constructor
• L34 decimals can be immutable
• L115 The _transfer don't check if the src and dst are not equal than address(0)
• L130 The _setAllowance don't check if the owner and spender are not equal than address(0)
• L167 The _mint don't check if the dst are not equal than address(0)
• L187 The _burn don't check if the src are not equal than address(0)

@rari-capital/solmate have a good implementation for marketplace/ERC20Permit.sol inside of the ERC20 implementation

[L-03] Repeated files

The redeemer/Safe.sol, lender/Safe.sol and marketplace/Safe.sol are the same, leave only one Can move redeemer/Interfaces.sol, lender/Interfaces.sol and marketplace/Interfaces.sol in a interfaces folder and separate each interface in a separates files

[L-04] OPEN TODOs

Open TODOs can point to architecture or programming issues that still need to be resolved.

• lender/Cast.sol: L09
• lender/Element.sol: L08, L09, L15

[L-05] Unused declaration (u, m);

address illuminateToken = principalToken(u, m);(u, m); to: address illuminateToken = principalToken(u, m);

[L-06] The sender should be the msg.sender

In sender: address(this), should be sender: msg.sender, to continue the call chain

[L-07] Don't validate the signatures and orders

In lender/Lender.sol:L247, the lend function don't validate the signatures and orders, the swivelAddr should validate this parameters, this enables to manipulate the order.premium and order.principal and affect the L307 ISwivel(swivelAddr).initiate(orders, amounts, signatures);

[L-08] Missing validation on length when looping two arrays on lend function

Number of orders submitted should be equal to the number of signatures since you got one signature pero order. To solve it add a require to the function;

require(o.lenght == s.length, "Mismatched length order/signature arrays");

Gas report

[G-01] USE immutable

The apwineAddr in redeemer/Redeemer.sol, L33 can be immutable

[G-02] Use type(uint256).max instead of 2**256 - 1

In lender/Lender.sol:

• L84, L93
• L112, L117

[G-03] Cast only in Safe.approve

for (uint256 i; i < len; ) {
-   IERC20 uToken = IERC20(u[i]);
-   if (address(0) != (address(uToken))) {
-       Safe.approve(uToken, a[i], max);
+   if (address(0) != u[i]) {
+       Safe.approve(IERC20(u[i]), a[i], max);
    }
    unchecked {
        i++;
    }
}