Illuminate contest - sashik_eth's results

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/marketplace/ERC5095.sol#L108-L119

Vulnerability details

Impact

Function redeem() should check if msg.sender has enough allowance to redeem user's token in case if msg.sender is not a holder, but instead of comparing with principalAmount it compares with underlyingAmount which will always have zero value in time of comparing since named return variables initializes with the default value (0 in case of uint256). It allows malicious actor to redeem any amount of any user's token.

    /// @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){
        if (block.timestamp < maturity) {
            revert Maturity(maturity);
        }
        if (holder == msg.sender) {
            return IRedeemer(redeemer).authRedeem(underlying, maturity, msg.sender, receiver, principalAmount);
        }
        else {
            require(_allowance[holder][msg.sender] >= underlyingAmount, 'not enough approvals');
            return IRedeemer(redeemer).authRedeem(underlying, maturity, holder, receiver, principalAmount);     
        }
    }

Recommended Mitigation Steps

require statement should compare allowance with principalAmount value:

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

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/marketplace/ERC5095.sol#L92-L103 https://github.com/code-423n4/2022-06-illuminate/blob/main/marketplace/ERC5095.sol#L108-L119

Vulnerability details

Impact

Functions withdraw() and redeem() does not decrease allowance value. This allows a malicious actor to withdraw/redeem any amount of user tokens in case of allowance has any value greater than 0, by calling function withdraw/redeem multiple times.

    /// @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){
        if (block.timestamp < maturity) {
            revert Maturity(maturity);
        }
        if (holder == msg.sender) {
            return IRedeemer(redeemer).authRedeem(underlying, maturity, msg.sender, receiver, underlyingAmount);
        }
        else {
            require(_allowance[holder][msg.sender] >= underlyingAmount, 'not enough approvals');
            return IRedeemer(redeemer).authRedeem(underlying, maturity, holder, receiver, underlyingAmount);     
        }
    }

    /// @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){
        if (block.timestamp < maturity) {
            revert Maturity(maturity);
        }
        if (holder == msg.sender) {
            return IRedeemer(redeemer).authRedeem(underlying, maturity, msg.sender, receiver, principalAmount);
        }
        else {
            require(_allowance[holder][msg.sender] >= underlyingAmount, 'not enough approvals');
            return IRedeemer(redeemer).authRedeem(underlying, maturity, holder, receiver, principalAmount);     
        }
    }

Recommended Mitigation Steps

Call of _decreaseAllowance() function should be added after require:

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

L01 - Lack of event emitting after sensitive actions

Contracts do not emit relevant events after setting sensitive variables. Consider emitting events after sensitive changes take place, to facilitate tracking and notify off-chain clients following the contract’s activity in following functions:

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L129 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L137 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L145 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L156 https://github.com/code-423n4/2022-06-illuminate/blob/main/marketplace/MarketPlace.sol#L98 https://github.com/code-423n4/2022-06-illuminate/blob/main/marketplace/MarketPlace.sol#L109 https://github.com/code-423n4/2022-06-illuminate/blob/main/marketplace/MarketPlace.sol#L119 https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L62 https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L70 https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L81 https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L92

L02 - Missing input validation on arrays length

Function Lender.lend() does not perform input validation of arrays length match. A mismatch could lead to an exception or undefined behavior.

L03 - Wrong interface

IRedeemer declares authRedeem() function that returns uint256 value:

    function authRedeem(address underlying, uint256 maturity, address from, address to, uint256 amount) external returns (uint256);

while actual implementation of authRedeem() function in Redeemercontract returns bool value:

    function authRedeem(
        address u,
        uint256 m,
        address f,
        address t,
        uint256 a
    ) public authorized(IMarketPlace(marketPlace).markets(u, m, 0)) returns (bool) {
		...

It lead to wrong returns from withdraw() and redeem() functions since they returns bool value from authRedeem() as uint256 value of token amount.

    /// @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){
        if (block.timestamp < maturity) {
            revert Maturity(maturity);
        }
        if (holder == msg.sender) {
            return IRedeemer(redeemer).authRedeem(underlying, maturity, msg.sender, receiver, underlyingAmount);
        }
        else {
            require(_allowance[holder][msg.sender] >= underlyingAmount, 'not enough approvals');
            return IRedeemer(redeemer).authRedeem(underlying, maturity, holder, receiver, underlyingAmount);     
        }
    }

    /// @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){
        if (block.timestamp < maturity) {
            revert Maturity(maturity);
        }
        if (holder == msg.sender) {
            return IRedeemer(redeemer).authRedeem(underlying, maturity, msg.sender, receiver, principalAmount);
        }
        else {
            require(_allowance[holder][msg.sender] >= underlyingAmount, 'not enough approvals');
            return IRedeemer(redeemer).authRedeem(underlying, maturity, holder, receiver, principalAmount);     
        }
    }

This lead's to missinformation of external caller which will receive value 1 in case of succesul redeem because true return value from authRedeem() would be interpreted as 1.

Recommendation: Update interface IRedeemer to return bool value in authRedeem() function. Also update withdraw() and redeem() so they return actual token amounts instead of interpreted as uint256 bool values.

N01 - Open TODO

lender/Cast.sol:9   require(n <= type(uint128).max, ''); // TODO err msgs 
lender/Element.sol:15   // TODO audit structure / names / order-of-members etc...
lender/Element.sol:8    // TODO are these established element names? kind? not type? etc... 

N02 - Typos

lender/Lender.sol:83 // max is the maximum integer value for a 256 unsighed integer lender/Lender.sol:650 // send the remaing amount to the given yield pool marketplace/ERC5095.sol:16 /// @dev address and interface for an external custody contract (necessary for some project's backwards compatability) marketplace/MarketPlace.sol:11 /// @notice This contract is in charge of managing the avaialable principals for each loan market. marketplace/MarketPlace.sol:50 /// @notice intializes the MarketPlace contract redeemer/Redeemer.sol:125 // Burn the prinicipal token from Illuminate
redeemer/Redeemer.sol:189 // Redeems prinicipal tokens from yield redeemer/Redeemer.sol:237 /// @param d Sense contract that splits the loan's prinicpal and yield

G01 - unchecked block can be used for gas efficiency of the expression that can't overflow/underflow

Since function Lender.calculateFee() always return value less or equal to its input parameter then next lines could be unchecked:

lender/Lender.sol:219   uint256 returned = yield(u, y, a - calculateFee(a), address(this)); 
lender/Lender.sol:229   uint256 returned = yield(u, y, a - calculateFee(a), msg.sender); 
lender/Lender.sol:281   uint256 amountLent = amount - fee; 
lender/Lender.sol:355   amount: a - calculateFee(a),  
lender/Lender.sol:411   returned = IPendle(pendleAddr).swapExactTokensForTokens(a - fee, r, path, address(this), d)[0]; 
lender/Lender.sol:517   lent = a - fee;  
lender/Lender.sol:575   lent = a - fee;  
lender/Lender.sol:624   uint256 returned = token.deposit(a - fee, address(this));  
lender/Lender.sol:681   return feenominator > 0 ? a / feenominator : 0; // could be unchecked due to statement logic

Line with adding block.timestamp value to constant value could be unchecked since their overflow would appear only in the extremely far future:

lender/Lender.sol:688   uint256 when = block.timestamp + HOLD; 

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

G02 - Variable could be immutable

apwineAddr never changes so it could be set to immutable.

redeemer/Redeemer.sol:33    address public apwineAddr; 

https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L33

G03 - Caching storage variables

Since reading from memory is much cheaper than reading from storage, state variables that are called more than 1 SLOAD inside the function should be cached.

redeemer/Redeemer.sol:180   Safe.transferFrom(IERC20(principal), lender, address(this), amount); // lender 2 SLOAD

https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L180

redeemer/Redeemer.sol:224   Safe.transferFrom(token, lender, address(this), amount);  // lender 2 SLOAD

https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L224

redeemer/Redeemer.sol:259   Safe.transferFrom(token, lender, address(this), amount);  

https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L259

G04 - Using prefix increment

Prefix increment is cheaper than postfix increment. Consider using ++i in next lines:

lender/Lender.sol:96    i++; 
lender/Lender.sol:120   i++; 
lender/Lender.sol:289   i++;