Illuminate contest - joestakey's results

QA Report

Table of Contents

• summary
• Immutable addresses lack zero-address check
• Library function not defined
• Setters should check the input value
• Unbounded fee
• Comment Missing function parameter
• Events indexing
• Event should be emitted in setters
• Function order
• Missing inheritance
• Public functions can be external
• Scientific notation
• Timelock for critical parameter changes

summary

Few vulnerabilities were found, the main concerns are with the lack of boundaries on fees amount:

Immutable addresses lack zero-address check

IMPACT

constructors should check the address written in an immutable address variable is not the zero address.

Note: while it has been indicated by the sponsor input validation will be on the front-end side to relieve users from unnecessary gas spendings, this issue here concerns constructor functions, ie when the contract is deployed by the team.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

MarketPlace.sol

redeemer = r
lender = l

Lender.sol

pendleAddr = p
tempusAddr = t

Redeemer.sol

pendleAddr = p
tempusAddr = t

TOOLS USED

Manual Analysis

MITIGATION

Add a zero address check for the immutable variables aforementioned.

Library function not defined

IMPACT

MarketPlace uses the Safe library transferFrom method in its minting functions. But there is no transferFrom method in marketplace/Safe.sol.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

MarketPlace.sol

Safe.transferFrom(ERC20(address(pool.base())), msg.sender, address(pool), uA)
Safe.transferFrom(ERC20(address(pool.fyToken())), msg.sender, address(pool), ptA)
Safe.transferFrom(ERC20(address(pool.base())), msg.sender, address(pool), a)

TOOLS USED

Manual Analysis

MITIGATION

Declare transferFrom in marketplace/Safe.sol

Setters should check the input value

PROBLEM

constructors and setters should check the input value - ie make revert if it is the zero address or zero

Note: while it has been indicated by the sponsor input validation will be on the front-end side to relieve users from unnecessary gas spendings, this issue here concerns constructor functions or setters called by authorized admins, not users, and most likely by interacting with the contract directly (for instance using Etherscan or Hardhat) and not with a front-end.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

MarketPlace.sol

admin = a
pools[u][m] = a

Lender.sol

swivelAddr = s
admin = a
marketPlace = m
swivelAddr = s

Redeemer.sol

lender = l
swivelAddr = s
apwineAddr = a
admin = a
marketPlace = m
lender = l
swivelAddr = s

TOOLS USED

Manual Analysis

MITIGATION

Add non-zero checks - address - to the setters aforementioned.

Unbounded fees

PROBLEM

There is no upper boundary to f in setFee. This means a malicious admin could set feenominator to 100%, resulting in any call to lend essentially draining users from their tokens without giving them any PT token in exchange.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

Lender.sol

function setFee

TOOLS USED

Manual Analysis

MITIGATION

Add a reasonable upper boundary in setFee.

Comment Missing function parameter

PROBLEM

Some of the function comments are missing function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Lender.sol

@param aave

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for this parameter

Commented code

PROBLEM

There are portions of commented code in some files.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NibblVault.sol

// _purchaseReturn = _buySecondaryCurve(_to, _lowerCurveDiff)
// buyoutValuationDeposit = _currentValuation - ((primaryReserveBalance - fictitiousPrimaryReserveBalance) + secondaryReserveBalance)

TOOLS USED

Manual Analysis

MITIGATION

Remove commented code

Events indexing

PROBLEM

Events should use indexed fields to make it easier to filter them in logs.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Lender.sol

event Lend(uint8 principal, address indexed underlying, uint256 indexed maturity, uint256 returned)
event Mint(uint8 principal, address indexed underlying, uint256 indexed maturity, uint256 amount)
event ScheduleWithdrawal(address indexed token, uint256 hold)\

Redeemer.sol

event Redeem(uint8 principal, address indexed underlying, uint256 indexed maturity, uint256 amount)

TOOLS USED

Manual Analysis

MITIGATION

Add indexed fields to these events so that they have the maximum number of indexed fields possible.

Event should be emitted in setters

PROBLEM

Setters should emit an event so that Dapps can detect important changes to storage.

Again, this does not go against the will of the sponsor to alleviate gas costs from users, as these setters are called by admins.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

MarketPlace.sol

function setPrincipal
function setAdmin
function setPool

Lender.sol

function setAdmin
function setFee
function setMarketPlace
function setSwivel

Redeemer.sol

function setAdmin
function setMarketPlace
function setLender
function setSwivel

TOOLS USED

Manual Analysis

MITIGATION

emit an event in all setters

Function order

PROBLEM

Functions should be ordered following the Soldiity conventions.

SEVERITY

Non-Critical

PROOF OF CONCEPT

MarketPlace.sol

In MarketPlace.sol and Lender.sol, several external functions are declared after public and even internal functions.

TOOLS USED

Manual Analysis

MITIGATION

Place the externalfunctions before public and internal functions.

Missing inheritance

PROBLEM

Decoupling an interface from its implementation can lead to code drift and incomplete or incorrect interfaces/implementations

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Redeemer.sol

contract Redeemer is not inheriting from IRedeemer.

TOOLS USED

Manual Analysis

MITIGATION

Redeemer should inherit from IRedeemer ( which is by the way defined in marketplace/Interfaces, but not in redeemer/Interfaces - add IRedeemer to redeemer/Interfaces for consistency)

Public functions can be external

PROBLEM

It is good practice to mark functions as external instead of public if they are not called by the contract where they are defined.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Lender.sol

Note: the lend functions listed below have the same name but are separate functions with different parameters.

function mint
function lend
function lend
function lend
function lend
function lend
function lend
function lend
function lend

Redeemer.sol

Note: the redeem functions listed below have the same name but are separate functions with different parameters.

function redeem
function redeem
function redeem
function redeem
function authRedeem

TOOLS USED

Manual Analysis

MITIGATION

Declare these functions as external instead of public

Scientific notation

PROBLEM

For readability, it is best to use scientific notation (e.g 10e5) rather than decimal literals(100000) or exponentiation(10**5). Underscores are used throughout the contracts and do improve readability too, so this is more of a suggestion.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Lender.sol

feenominator = 1000

TOOLS USED

Manual Analysis

Timelock for critical parameter changes

PROBLEM

It is good practice to add timelock to critical parameters changes, such as admin changes, to give users time to react.

SEVERITY

Non-Critical

PROOF OF CONCEPT

All the admin setters across all three contracts are missing a timelock.

TOOLS USED

Manual Analysis

MITIGATION

Consider adding a timelock to admin setters.

Gas Report

Table of Contents

• Caching storage variables in memory to save gas
• Calldata instead of memory for RO function parameters
• Comparison operators
• Constructor parameters should be avoided when possible
• Custom errors
• Default value initialization
• Immutable variables save storage
• Mathematical optimizations
• Prefix increments
• unchecked arithmetic
• unnecessary computation

Caching storage variables in memory to save gas

IMPACT

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

PROOF OF CONCEPT

Instances include:

Lender.sol

scope: calculateFee()

• feeNominator is read twice if feeNominator > 0. It may be worth caching it as it is quite likely to be the case.

return feenominator > 0 ? a / feenominator : 0

Redeemer.sol

scope: redeem(uint8 p,address u,uint256 m,address o)

• lender is read twice.

uint256 amount = IERC20(principal).balanceOf(lender)
Safe.transferFrom(IERC20(u), lender, address(this), amount)

scope: redeem(uint8 p,address u,uint256 m)

• lender is read twice.

uint256 amount = IERC20(principal).balanceOf(lender)
Safe.transferFrom(IERC20(u), lender, address(this), amount)

scope: redeem(uint8 p,address u,uint256 m, bytes32 i)

• lender is read twice.

uint256 amount = token.balanceOf(lender)
Safe.transferFrom(token, lender, address(this), amount)

scope: redeem(uint8 p,address u,uint256 m, address d,address o)

• lender is read twice.

uint256 amount = token.balanceOf(lender)
Safe.transferFrom(token, lender, address(this), amount)

TOOLS USED

Manual Analysis

MITIGATION

cache these storage variables in memory

Calldata instead of memory for RO function parameters

PROBLEM

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Try to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.

PROOF OF CONCEPT

Instances include:

Lender.sol

uint256[] memory a

TOOLS USED

Manual Analysis

MITIGATION

Replace memory with calldata

Comparison Operators

IMPACT

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and ==.

Using strict comparison operators hence saves gas, approximately 3 gas in require and if statements

PROOF OF CONCEPT

Instances include:

Lender.sol

require (block.timestamp >= when, 'withdrawal still on hold')

TOOLS USED

Manual Analysis

MITIGATION

Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable

example:

-block.timestamp >= when
+block.timestamp > when - 1;

However, if 1 is negligible compared to the value of the variable, we can omit the increment.

-block.timestamp >= when
+block.timestamp > when;

Constructor parameters should be avoided when possible

IMPACT

Constructor parameters are expensive. The contract deployment will be cheaper in gas if they are hard coded instead of using constructor parameters.

PROOF OF CONCEPT

Instances include:

MarketPlace.sol

redeemer = r
lender = l

Lender.sol

swivelAddr = s
pendleAddr = p
tempusAddr = t
feenominator = 1000, this one is not a construction parameter, this initial value can be hardcoded.

Redeemer.sol

lender = l
swivelAddr = s
pendleAddr = p
tempusAddr = t
apwineAddr = a

TOOLS USED

Manual Analysis

MITIGATION

Hardcode storage variables with their initial value instead of writing it during contract deployment with constructor parameters.

Custom Errors

IMPACT

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here.

It not only saves gas upon deployment - ~5500 gas saved per custom error instead of a require statement, but it is also cheaper in a function call, 22 gas saved per require statement replaced with a custom error.

Custom errors are defined using the error statement

PROOF OF CONCEPT

Instances include:

Lender.sol

require (when != 0, 'no withdrawal scheduled')
require (block.timestamp >= when, 'withdrawal still on hold')

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance, in Lender.sol:

Replace

require (when != 0, 'no withdrawal scheduled')

with

if (when == 0) {
	revert NoWithdrawalScheduled();
}

and define the custom error in the contract

error NoWithdrawalScheduled();

Default value initialization

IMPACT

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes 3 gas per variable initialized.

PROOF OF CONCEPT

Instances include:

Lender.sol

uint i = 0

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values.

Immutable variables save storage

PROBLEM

If a variable is set in the constructor and never modified afterwards, marking it as immutable can save a storage operation - 20,000 gas.

PROOF OF CONCEPT

Instances include:

Redeemer.sol

address public apwineAddr is only set in the constructor but can never be modified.

TOOLS USED

Manual Analysis

MITIGATION

Mark it as immutable.

Mathematical optimizations

PROBLEM

X += Y costs 3 more gas than X = X + Y.

PROOF OF CONCEPT

Instances include:

Lender.sol

totalFee += fee
lent += amountLent
returned += amountLent * order.premium / order.principal
fees[u] += totalFee
fees[u] += calculateFee(a)
fees[u] += fee
fees[u] += fee
fees[u] += fee
fees[u] += fee
fees[u] += fee

TOOLS USED

Manual Analysis

MITIGATION

use X = X + Y instead of X += Y (same with -)

Prefix increments

IMPACT

Prefix increments are cheaper than postfix increments: it returns the incremented variable instead of returning a temporary variable storing the initial value of the variable. It saves 5 gas per iteration

PROOF OF CONCEPT

Instances include:

Lender.sol

i++
i++
i++

TOOLS USED

Manual Analysis

MITIGATION

change variable++ to ++variable.

Unchecked arithmetic

IMPACT

The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas

PROOF OF CONCEPT

Instances include:

Lender.sol

a - calculateFee(a), calculateFee(a) <= a, so it cannot underflow
a - calculateFee(a), calculateFee(a) <= a, so it cannot underflow
uint256 amountLent = amount - fee, fee = calculateFee(amount) <= amount, so it cannot underflow
a - fee, fee = calculateFee(a) <= a, so it cannot underflow
a - fee, fee = calculateFee(a) <= a, so it cannot underflow
a - fee, fee = calculateFee(a) <= a, so it cannot underflow
a - fee, fee = calculateFee(a) <= a, so it cannot underflow

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block

Unnecessary computation

IMPACT

Gas can be saved by avoiding using local variables when they are not necessary.

PROOF OF CONCEPT

There are two instances:

Lender.sol

uint256 max = 2**256 - 1, is only used here so it can be inlined
uint256 max = 2**256 - 1, is only used here so it can be inlined

TOOLS USED

Manual Analysis

MITIGATION

• inline max with its value to avoid the use of a local variable
• Additional gas can be saved by using type(uint256).max instead of 2**256 - 1