Yieldy contest - joestakey's results

QA Report

Table of Contents

• summary
• affiliateFee unbounded
• Setters should check the input value
• Comment Missing function parameter
• Events indexing
• Event should be emitted in setters
• Function missing comments
• Function order
• Internal functions should be private
• Public functions can be external
• Unbounded loop

summary

Few vulnerabilities were found. The main concerns are with:

affiliateFee unbounded

PROBLEM

There is no boundary to the affiliateFee set by the owner in setAffiliateFee. If the fee is set to 100%, this would mean users would not be able to claim TOKE rewards from TokeMak as the recipient.amount would be equal to the fee.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

Staking.sol

function setAffiliateFee

TOOLS USED

Manual Analysis

MITIGATION

Add a reasonable upper boundary to setAffiliateFee. Consider also adding a timelock to it to give users time to react.

Setters should check the input value

PROBLEM

Setters and initializers should check the input value - ie make revert if it is the zero address

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

Staking.sol

function setCurvePool

TOOLS USED

Manual Analysis

MITIGATION

Add a non-zero address check to setCurvePool.

Comment Missing function parameter

PROBLEM

Some of the function comments are missing function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Staking.sol

@param orderUid

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for this parameter

Events indexing

PROBLEM

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

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Staking.sol

event LogSetEpochDuration(uint256 indexed blockNumber, uint256 duration)
event LogSetWarmUpPeriod(uint256 indexed blockNumber, uint256 period)
event LogSetCoolDownPeriod(uint256 indexed blockNumber, uint256 period)
event LogSetPauseStaking(uint256 indexed blockNumber, bool shouldPause)
event LogSetPauseUnstaking(uint256 indexed blockNumber, bool shouldPause)
event LogSetPauseInstantUnstaking(uint256 indexed blockNumber,bool shouldPause)
event LogSetAffiliateAddress(uint256 indexed blockNumber,address affilateAddress)
event LogSetAffiliateFee(uint256 indexed blockNumber, uint256 fee)
event LogSetCurvePool(address indexed curvePool, int128 to, int128 from)

Yieldy.sol

event LogSupply(uint256 indexed epoch,uint256 timestamp,uint256 totalSupply)
event LogRebase(uint256 indexed epoch, uint256 rebase, uint256 index)

LiquidityReserve.sol

event FeeChanged(uint256 fee)

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

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Staking.sol

function setTimeLeftToRequestWithdrawal

Yieldy.sol

function _setIndex

TOOLS USED

Manual Analysis

MITIGATION

emit an event in all setters

Function missing comments

PROBLEM

Some functions are missing Natspec comments

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Staking.sol

function initialize

TOOLS USED

Manual Analysis

MITIGATION

Add comments to these functions

Function order

PROBLEM

Functions should be ordered following the Soldiity conventions. This means ordering functions based on their visibility.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Several contracts have external functions defined after internal functions:

• Staking.sol

• Yieldy.sol

• LiquidityReserve.sol

TOOLS USED

Manual Analysis

MITIGATION

Place the externalfunctions before public and internal functions.

Internal functions should be private

PROBLEM

Functions modifying storage that should be called by no contract besides the contract itself should be declared as private instead of internal, as having them available can lead to exploits. This is particularly true for internal functions called in admin functions such as setters.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Staking.sol

function _sendAffiliateFee sends the affiliate fee to the fee address. function setToAndFromCurve sets the coin indexes and is called by setCurvePool, which has the onlyOwner modifier.

Yieldy.sol

function _storeRebase is called by rebase, which has the onlyRole(REBASE_ROLE) modifier.

TOOLS USED

Manual Analysis

MITIGATION

Declare these functions as private instead of internal

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:

Staking.sol

function unstakeAllFromTokemak()

TOOLS USED

Manual Analysis

MITIGATION

Declare this function as external instead of public

Unbounded loop

PROBLEM

Functions looping through unbounded storage arrays may cost excessive amount of gas or even revert if the array is too large.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Staking.sol

removeAddress loops through the array of addresses and delete the address to remove when it finds it. The costs of calling this function may vary drastically depending on the location of the address in the array and how large the array is

TOOLS USED

Manual Analysis

MITIGATION

Consider using Enumerable sets.

Gas Report

Table of Contents

• Caching storage variables in memory to save gas
• Comparison operators
• Custom errors
• Default value initialization
• Event emitting of local variable
• Mathematical optimizations
• Modifier instead of duplicate require
• Prefix increments
• Require instead of AND
• Storage instead of memory
• unchecked arithmetic
• Update Solidity version

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:

Staking.sol

scope: initialize()

• CURVE_POOL is read twice, but can be cached using the function argument _curvePool

CURVE_POOL != address(0)
IERC20(TOKE_POOL).approve(CURVE_POOL, type(uint256).max)

• TOKE_POOL is read twice, but can be cached using the function argument _tokePool

IERC20(TOKE_POOL).approve(CURVE_POOL, type(uint256).max)
IERC20(STAKING_TOKEN).approve(TOKE_POOL, type(uint256).max)

• YIELDY_TOKEN is read twice, but can be cached using the function argument _yieldyToken

IERC20Upgradeable(YIELDY_TOKEN)
IERC20Upgradeable(YIELDY_TOKEN)

• LIQUIDITY_RESERVE is read twice, but can be cached using the function argument _liquidityReserve

LIQUIDITY_RESERVE
LIQUIDITY_RESERVE

scope: _sendAffiliateFee()

• affiliateFee is read twice

affiliateFee != 0
_amount * affiliateFee

• FEE_ADDRESS is read twice

FEE_ADDRESS != address(0)
IERC20Upgradeable(TOKE_TOKEN).safeTransfer(FEE_ADDRESS, feeAmount)

scope: transferToke()

• TOKE_TOKEN is read twice

IERC20Upgradeable(TOKE_TOKEN)
IERC20Upgradeable(TOKE_TOKEN)

scope: _requestWithdrawalFromTokemak()

• TOKE_POOL is read once, but can be cached using the local variable tokePoolContract

ITokePool(TOKE_POOL))

scope: stake()

• TOKE_POOL is read three times - at different locations depending on conditions.

YIELDY_TOKEN
IYieldy(YIELDY_TOKEN)
IYieldy(YIELDY_TOKEN)
IYieldy(YIELDY_TOKEN)

scope: claim()

• TOKE_POOL is read twice.

IYieldy(YIELDY_TOKEN)
IYieldy(YIELDY_TOKEN)

scope: _retrieveBalanceFromUser()

• YIELDY_TOKEN is read twice or up to five times depending on conditions.

IERC20Upgradeable(YIELDY_TOKEN)
IYieldy(YIELDY_TOKEN)
IYieldy(YIELDY_TOKEN)
IYieldy(YIELDY_TOKEN)
IERC20Upgradeable(YIELDY_TOKEN)

scope: instantUnstakeReserve()

• LIQUIDITY_RESERVE is read twice

LIQUIDITY_RESERVE
ILiquidityReserve(LIQUIDITY_RESERVE)

scope: setToAndFromCurve()

• CURVE_POOL is read three times

CURVE_POOL != address(0)
ICurvePool(CURVE_POOL).coins(0)
ICurvePool(CURVE_POOL).coins(1)

scope: rebase()

• YIELDY_TOKEN is read three times

IYieldy(YIELDY_TOKEN).rebase(epoch.distribute, epoch.number)
Iuint256 staked = IYieldy(YIELDY_TOKEN).totalSupply()

LiquidityReserve.sol

scope: initialize()

• decimal is read once, but can be cached using the function argument _decimal

WAD = 10**decimal

• WAD is read twice

rebasingCreditsPerToken = WAD
_setIndex(WAD)

scope: rebase()

• _totalSupply is read once, but can be cached using the local variable currentTotalSupply

require(_totalSupply > 0, "Can't rebase if not circulating")

scope: transfer()

• creditBalances[msg.sender] is read twice

require(creditAmount <= creditBalances[msg.sender], "Not enough funds")
creditBalances[msg.sender] - creditAmount

scope: transferFrom()

• _allowances[_from][msg.sender] is read twice

require(_allowances[_from][msg.sender] >= _value, "Allowance too low")
_allowances[_from][msg.sender] - _value

scope: _burn()

• creditBalances[_address] can be cached using the local variable currentCredits

creditBalances[_address] - creditAmount

LiquidityReserve.sol

scope: enableLiquidityReserve()

• stakingToken is read twice

IERC20Upgradeable(stakingToken)
IERC20Upgradeable(stakingToken)

• MINIMUM_LIQUIDITY is read three times

stakingTokenBalance >= MINIMUM_LIQUIDITY
MINIMUM_LIQUIDITY
MINIMUM_LIQUIDITY

• stakingContract is read once, but can be substituted with the function argument _stakingContract

stakingContract

scope: addLiquidity()

• stakingToken is read twice

IERC20Upgradeable(stakingToken)
IERC20Upgradeable(stakingToken)

scope: removeLiquidity()

• stakingToken is read twice

IERC20Upgradeable(stakingToken)
IERC20Upgradeable(stakingToken)

scope: unstakeAllRewardTokens()

• stakingContract is read twice

IERC20Upgradeable(stakingToken)
IERC20Upgradeable(stakingToken)

BatchRequests.sol

scope: canBatchContractByIndex()

• contracts[_index] is read twice

contracts[_index]
IStaking(contracts[_index]).canBatchTransactions()

TOOLS USED

Manual Analysis

MITIGATION

cache these storage variables in memory

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:

Staking.sol

requestedWithdrawals.minCycle <= currentCycleIndex
_amount <= walletBalance
if (_amount >= warmUpBalance)
require(reserveBalance >= _amount, "Not enough funds in reserve")
if (epoch.endTime <= block.timestamp)
if (balance <= staked)

Yieldy.sol

require(creditAmount <= creditBalances[msg.sender], "Not enough funds")
require(_allowances[_from][msg.sender] >= _value, "Allowance too low")
require(currentCredits >= creditAmount, "Not enough balance")

LiquidityReserve.sol

require(stakingTokenBalance >= MINIMUM_LIQUIDITY,"Not enough staking tokens")
require(_fee <= BASIS_POINTS, "Out of range")
require(_amount <= balanceOf(msg.sender), "Not enough lr tokens")
require(IERC20Upgradeable(stakingToken).balanceOf(address(this)) >= amountToWithdraw,"Not enough funds")

TOOLS USED

Manual Analysis

MITIGATION

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

example:

-_fee <= BASIS_POINTS;
+_fee < BASIS_POINTS + 1;

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

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:

Staking.sol

require(_stakingToken != address(0) && _yieldyToken != address(0) && _tokeToken != address(0) &&_tokePool != address(0) &&_tokeManager != address(0) &&_tokeReward != address(0) &&_liquidityReserve != address(0),"Invalid address")
require(_recipient.amount > 0, "Must enter valid amount")
require(_claimAddress != address(0), "Invalid address"
require(!isStakingPaused, "Staking is paused")
require(_amount > 0, "Must have valid amount")
require(_amount <= walletBalance + warmUpBalance,"Insufficient Balance")
require(_amount > 0, "Invalid amount")
require(!isUnstakingPaused && !isInstantUnstakingPaused,"Unstaking is paused")
require(!isUnstakingPaused && !isInstantUnstakingPaused,"Unstaking is paused")
require(_amount > 0, "Invalid amount")
require(CURVE_POOL != address(0) &&(curvePoolFrom == 1 || curvePoolTo == 1),"Invalid Curve Pool")
require(!isUnstakingPaused && !isInstantUnstakingPaused,"Unstaking is paused")
require(from == 1 || to == 1, "Invalid Curve Pool")
require(!isUnstakingPaused, "Unstaking is paused")

Yieldy.sol

require(stakingContract == address(0), "Already Initialized")
require(_stakingContract != address(0), "Invalid address")
require(_totalSupply > 0, "Can't rebase if not circulating")
require(rebasingCreditsPerToken > 0, "Invalid change in supply")
require(_to != address(0), "Invalid address")
require(creditAmount <= creditBalances[msg.sender], "Not enough funds")
require(_allowances[_from][msg.sender] >= _value, "Allowance too low")
require(_address != address(0), "Mint to the zero address")
require(_totalSupply < MAX_SUPPLY, "Max supply")
require(_address != address(0), "Burn from the zero address")
require(currentCredits >= creditAmount, "Not enough balance")

Migration.sol

require(_oldContract != address(0) && _newContract != address(0), "Invalid address")

LiquidityReserve.sol

require(msg.sender == stakingContract, "Not staking contract")
require(_stakingToken != address(0) && _rewardToken != address(0),"Invalid address")
require(!isReserveEnabled, "Already enabled")
require(_stakingContract != address(0), "Invalid address")
require(stakingTokenBalance >= MINIMUM_LIQUIDITY, "Not enough staking tokens")
require(_fee <= BASIS_POINTS, "Out of range")
require(isReserveEnabled, "Not enabled yet")
require(_amount <= balanceOf(msg.sender), "Not enough lr tokens")
require(IERC20Upgradeable(stakingToken).balanceOf(address(this)) >= amountToWithdraw,"Not enough funds")
require(isReserveEnabled, "Not enabled yet")
require(isReserveEnabled, "Not enabled yet")

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance, in BatchRequests.sol:

Replace

require(isReserveEnabled, "Not enabled yet")

with

if (!isReserveEnabled) {
		revert IsNotEnabled();
}

and define the custom error in the contract

error IsNotEnabled();

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:

Staking.sol

int128 from = 0
int128 to = 0

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values.

Event emitting of local variable

PROBLEM

When emitting an event, using a local variable instead of a storage variable saves gas.

PROOF OF CONCEPT

Instances include:

Staking.sol

emit LogSetCurvePool(CURVE_POOL, curvePoolTo, curvePoolFrom)

TOOLS USED

Manual Analysis

MITIGATION

Emit a local variable or function argument instead of storage variable

Mathematical optimizations

PROBLEM

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

PROOF OF CONCEPT

Instances include:

Staking.sol

requestWithdrawalAmount -= requestedWithdrawals.amount
withdrawalAmount += requestedWithdrawals.amount
withdrawalAmount -= info.amount
amountLeft -= warmUpBalance
requestWithdrawalAmount += _amount

TOOLS USED

Manual Analysis

MITIGATION

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

Modifier instead of duplicate require

PROBLEM

When a require statement is use multiple times, it is cheaper to use a modifier instead.

PROOF OF CONCEPT

Instances include:

Staking.sol

require(_amount > 0, "Invalid amount")
require(_amount > 0, "Invalid amount")
require(!isUnstakingPaused && !isInstantUnstakingPaused,"Unstaking is paused")
require(!isUnstakingPaused && !isInstantUnstakingPaused,"Unstaking is paused")
require(!isUnstakingPaused && !isInstantUnstakingPaused,"Unstaking is paused")\

LiquidityReserve.sol

require(!isReserveEnabled, "Already enabled")
require(isReserveEnabled, "Not enabled yet")
require(isReserveEnabled, "Not enabled yet")
require(isReserveEnabled, "Not enabled yet")

TOOLS USED

Manual Analysis

MITIGATION

Use modifiers for these repeated statements

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:

Staking.sol

epoch.number++

TOOLS USED

Manual Analysis

MITIGATION

change variable++ to ++variable.

Require instead of AND

IMPACT

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas.

PROOF OF CONCEPT

Instances include:

Staking.sol

require(_stakingToken != address(0) && _yieldyToken != address(0) && _tokeToken != address(0) &&_tokePool != address(0) &&_tokeManager != address(0) &&_tokeReward != address(0) &&_liquidityReserve != address(0),"Invalid address")
require(!isUnstakingPaused && !isInstantUnstakingPaused,"Unstaking is paused")
require(!isUnstakingPaused && !isInstantUnstakingPaused,"Unstaking is paused")
require(CURVE_POOL != address(0) &&(curvePoolFrom == 1 || curvePoolTo == 1),"Invalid Curve Pool")
require(!isUnstakingPaused && !isInstantUnstakingPaused,"Unstaking is paused")

Migration.sol

require(_oldContract != address(0) && _newContract != address(0), "Invalid address")

LiquidityReserve.sol

require(_stakingToken != address(0) && _rewardToken != address(0),"Invalid address")

TOOLS USED

Manual Analysis

MITIGATION

Break down the statements in multiple require statements.

-require(!isUnstakingPaused && !isInstantUnstakingPaused,"Unstaking is paused")
+require(!isUnstakingPaused)
+require(!isInstantUnstakingPaused)

You can also improve gas savings by using custom errors

Storage instead of memory

IMPACT

Whenever a storage reference is casted to memory, a copy is made and hence costs more gas when it is read.

PROOF OF CONCEPT

Instances include:

Staking.sol

Claim memory info = warmUpInfo[_recipient]
Claim memory info = coolDownInfo[_recipient]

TOOLS USED

Manual Analysis

MITIGATION

Use storage instead of memory

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:

Staking.sol

epoch.distribute = balance - staked cannot underflow because of the check line 713

Yieldy.sol

creditBalances[msg.sender] - creditAmount cannot underflow because of the check line 190
_allowances[_from][msg.sender] - _value cannot underflow because of the check line 210
creditBalances[_address] - creditAmount cannot underflow because of the check line 286

LiquidityReserve.sol

uint256 amountMinusFee = _amount - ((_amount * fee) / BASIS_POINTS) cannot underflow because fee <= BASIS_POINTS, ie (_amount * fee) / BASIS_POINTS) <= _amount

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block

Update Solidity version

IMPACT

Use a solidity version >0.8.10. This will save gas on external calls as they skip contract existence checks.

PROOF OF CONCEPT

All the contracts in the scope

TOOLS USED

Manual Analysis

MITIGATION

Update the Solidity version in every contract.