Yieldy contest - TomJ's results

[L-01] Use safeTransfer/safeTransferFrom

Some implementations of transfer and transferFrom could return false on failure instead of reverting. It is best practice to add a require() statement that checks the return values or use OpenZeppelin's SafeERC20 wrapper functions. Reference: https://consensys.net/diligence/audits/2020/09/aave-protocol-v2/#unhandled-return-values-of-transfer-and-transferfrom

Issue found at https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/Migration.sol#L48-L52 https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/Staking.sol#L471-L473

[N-01] Event is Missing Indexed Fields

Each event should have 3 indexed fields if there are 3 or more fields.

Issue found at

./LiquidityReserve.sol:21:    event FeeChanged(uint256 fee);
./Yieldy.sol:15-19:    event LogSupply(uint256 indexed epoch, uint256 timestamp, uint256 totalSupply);
./Yieldy.sol:21:    event LogRebase(uint256 indexed epoch, uint256 rebase, uint256 index);
./Staking.sol:21:    event LogSetEpochDuration(uint256 indexed blockNumber, uint256 duration);
./Staking.sol:22:    event LogSetWarmUpPeriod(uint256 indexed blockNumber, uint256 period);
./Staking.sol:23:    event LogSetCoolDownPeriod(uint256 indexed blockNumber, uint256 period);
./Staking.sol:24:    event LogSetPauseStaking(uint256 indexed blockNumber, bool shouldPause);
./Staking.sol:25:    event LogSetPauseUnstaking(uint256 indexed blockNumber, bool shouldPause);
./Staking.sol:26-29:    event LogSetPauseInstantUnstaking(uint256 indexed blockNumber, bool shouldPause);
./Staking.sol:30-33:    event LogSetAffiliateAddress(uint256 indexed blockNumber, address affilateAddress);
./Staking.sol:34:    event LogSetAffiliateFee(uint256 indexed blockNumber, uint256 fee);
./Staking.sol:36:    event LogSetCurvePool(address indexed curvePool, int128 to, int128 from);

[N-02] Incomplete NatSpec

It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces. However there were places where this NatSpec was incomplete.

Issue found at

1. no @param for _index https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/BatchRequests.sol#L47-L50
2. no @param for _index https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/BatchRequests.sol#L62-L65

[N-03] Best Practice of Long Function Layout

Some contract is not following best practice of long function layout. This will reduce readability of code and harder to audit.

Following is line from solidity docs (link: https://docs.soliditylang.org/en/v0.8.15/style-guide.html#function-declaration)

For long function declarations, it is recommended to drop each argument onto its own line 
at the same indentation level as the function body. The closing parenthesis and opening bracket 
should be placed on their own line as well at the same indentation level as the function declaration.

Some of the function doesn't have closing parenthesis and opening bracket on their own line. Issue found at https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/Yieldy.sol#L33 https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/Yieldy.sol#L209 https://https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/Staking.sol#L50github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/LiquidityReserve.sol#L41 https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/Staking.sol#L50

[G-01] Should Use Unchecked Block where Over/Underflow is not Possible

Since Solidity 0.8.0, all arithmetic operations revert on overflow and underflow by default. However in places where overflow and underflow is not possible, it is better to use unchecked block to reduce the gas usage. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#checked-or-unchecked-arithmetic

Issue found at

1. Yieldy.sol

Wrap line 192 with unchecked since underflow is not possible due to line 190 check
190:        require(creditAmount <= creditBalances[msg.sender], "Not enough funds");
192:        creditBalances[msg.sender] = creditBalances[msg.sender] - creditAmount;

Wrap line 212 with unchecked since underflow is not possible due to line 210 check
210:        require(_allowances[_from][msg.sender] >= _value, "Allowance too low");
212:        uint256 newValue = _allowances[_from][msg.sender] - _value;

2. Staking.sol

Wrap line 716 with unchecked since underflow is not possible due to line 713 check
713:            if (balance <= staked) {
714:                epoch.distribute = 0;
715:            } else {
716:                epoch.distribute = balance - staked;

[G-02] Minimize the Number of SLOADs by Caching State Variable

SLOADs cost 100 gas where MLOADs/MSTOREs cost only 3 gas. Whenever function reads storage value more than once, it should be cached to save gas.

Issue found at

1. LiquidityReserve.sol

Cache MINIMUM_LIQUIDITY of function enableLiquidityReserve
69:            stakingTokenBalance >= MINIMUM_LIQUIDITY,
78:            MINIMUM_LIQUIDIT
80:        _mint(address(this), MINIMUM_LIQUIDITY);

2. Staking.sol

Cache TOKE_POOL and STAKING_TOKEN of function setToAndFromCurve
639:            if (TOKE_POOL == address0 && STAKING_TOKEN == address1) {
641:            } else if (TOKE_POOL == address1 && STAKING_TOKEN == address0) {

Cache FEE_ADDRESS of function _sendAffiliateFee
130:        if (affiliateFee != 0 && FEE_ADDRESS != address(0)) {
132:            IERC20Upgradeable(TOKE_TOKEN).safeTransfer(FEE_ADDRESS, feeAmount);

Cache warmUpPeriod of function stake
435:        if (warmUpPeriod == 0) {
443:                expiry: epoch.number + warmUpPeriod

Cache withdrawalAmount of function _isClaimWithdrawAvailable
289:                requestedWithdrawals.amount + withdrawalAmount >=
290:                info.amount) || withdrawalAmount >= info.amount);

Cache affiliateFee of function _sendAffiliateFee
130:        if (affiliateFee != 0 && FEE_ADDRESS != address(0)) {
131:            uint256 feeAmount = (_amount * affiliateFee) / BASIS_POINTS;

Cache curvePoolFrom and curvePoolTo of function instantUnstakeCurve
607:                (curvePoolFrom == 1 || curvePoolTo == 1),
619:        return
620:            ICurvePool(CURVE_POOL).exchange(
621:                curvePoolFrom,
622:                curvePoolTo,

3. Yieldy.sol

Cache MAX_SUPPLY of function rebase
91:            if (updatedTotalSupply > MAX_SUPPLY) {
92:                updatedTotalSupply = MAX_SUPPLY;

[G-03] Use Calldata instead of Memory for Read Only Function Parameters

It is cheaper gas to use calldata than memory if the function parameter is read only. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory. More details on following link. link: https://docs.soliditylang.org/en/v0.8.15/types.html#data-location

I recommend changing following memory to calldata

./LiquidityReserve.sol:37:        string memory _tokenName,
./LiquidityReserve.sol:38:        string memory _tokenSymbol,
./Yieldy.sol:30:        string memory _tokenName,
./Yieldy.sol:31:        string memory _tokenSymbol,

[G-04] Change Function Visibility Public to External

If the function is not called internally, it is cheaper to set your function visibility to external instead of public.

Issue found at Staking.sol:370:unstakeAllFromTokemak() Yieldy.sol:138:balanceOf() Yieldy.sol:182:transfer() Yieldy.sol:205:transferFrom() Yieldy.sol:227:decimals()

[G-05] Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas

This is because EVM operates on 32 bytes at a time. So I recommend using uint256 instead of anything smaller. More information about this in the following link. link: https://docs.soliditylang.org/en/v0.8.15/internals/layout_in_storage.html

Issue found at

./Yieldy.sol:32:        uint8 _decimal
./Yieldy.sol:227:    function decimals() public view override returns (uint8) {
./Staking.sol:36:    event LogSetCurvePool(address indexed curvePool, int128 to, int128 from);
./Staking.sol:113:        uint8 _v,
./Staking.sol:636:            int128 from = 0;
./Staking.sol:637:            int128 to = 0;

[G-06] Use require instead of &&

When there are multiple conditions in require statement, break down the require statement into multiple require statements instead of using && can save gas.

Issue found at

./LiquidityReserve.sol:44-47:        require(_stakingToken != address(0) && _rewardToken != address(0), "Invalid address");
./Migration.sol:20-23:        require(_oldContract != address(0) && _newContract != address(0), "Invalid addressh);
./Staking.sol:54-63:        require(_stakingToken != address(0) && _yieldyToken != address(0) && _tokeToken != address(0) && _tokePool != address(0) && _tokeManager != address(0) && _tokeReward != address(0) && _liquidityReserve != address(0), "Invalid address");
./Staking.sol:574-577:        require(!isUnstakingPaused && !isInstantUnstakingPaused, "Unstaking is paused");
./Staking.sol:605-609:        require(CURVE_POOL != address(0) && (curvePoolFrom == 1 || curvePoolTo == 1), "Invalid Curve Pool");
./Staking.sol:611-614:        require(!isUnstakingPaused && !isInstantUnstakingPaused, "Unstaking is paused");

For example these can be changed to

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

[G-07] Duplicate require() Checks Should be a Modifier or a Function

Since below require checks are used more than once, I recommend making these to a modifier or a function.

./LiquidityReserve.sol:105:        require(isReserveEnabled, "Not enabled yet");
./LiquidityReserve.sol:192:        require(isReserveEnabled, "Not enabled yet");
./LiquidityReserve.sol:215:        require(isReserveEnabled, "Not enabled yet");

./Staking.sol:572:        require(_amount > 0, "Invalid amount");
./Staking.sol:604:        require(_amount > 0, "Invalid amount");

./Staking.sol:574-577:        require(!isUnstakingPaused && !isInstantUnstakingPaused, "Unstaking is paused");
./Staking.sol:611-614:        require(!isUnstakingPaused && !isInstantUnstakingPaused, "Unstaking is paused");

[G-08] Unnecessary Default Value Initialization

When variable is not initialized, it will have its default values. For example, 0 for uint, false for bool and address(0) for address. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#scoping-and-declarations

I suggest removing default value initialization for following variables.

./Staking.sol:636:            int128 from = 0;
./Staking.sol:637:            int128 to = 0;

For example these can change to:

• int128 from;

[G-09] ++i Costs Less Gas than i++

It is better to use ++i than i++ when possible since it costs less gas.

Issue found at:

./Staking.sol:708:            epoch.number++;

[G-10] != 0 costs less gass than > 0

!= 0 costs less gas when optimizer is enabled and is used for unsigned integers in "require" statement. I suggest changing > 0 to != 0

Issue found at:

./Yieldy.sol:83:        require(_totalSupply > 0, "Can't rebase if not circulating");
./Yieldy.sol:96:            require(rebasingCreditsPerToken > 0, "Invalid change in supply");
./Staking.sol:118:        require(_recipient.amount > 0, "Must enter valid amount");
./Staking.sol:410:        require(_amount > 0, "Must have valid amount");
./Staking.sol:572:        require(_amount > 0, "Invalid amount");
./Staking.sol:604:        require(_amount > 0, "Invalid amount");

[G-11] Use Custom Errors to Save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings. Details are explained here: https://blog.soliditylang.org/2021/04/21/custom-errors/

I recommend using custom errors.

./LiquidityReserve.sol:25:        require(msg.sender == stakingContract, "Not staking contract");
./LiquidityReserve.sol:44-47:        require(_stakingToken != address(0) && _rewardToken != address(0), "Invalid address");
./LiquidityReserve.sol:61:        require(!isReserveEnabled, "Already enabled");
./LiquidityReserve.sol:62:        require(_stakingContract != address(0), "Invalid address");
./LiquidityReserve.sol:68-71:        require(stakingTokenBalance >= MINIMUM_LIQUIDITY, "Not enough staking tokens");
./LiquidityReserve.sol:94:        require(_fee <= BASIS_POINTS, "Out of range");
./LiquidityReserve.sol:105:        require(isReserveEnabled, "Not enabled yet");
./LiquidityReserve.sol:163:        require(_amount <= balanceOf(msg.sender), "Not enough lr tokens");
./LiquidityReserve.sol:170-174:        require(IERC20Upgradeable(stakingToken).balanceOf(address(this)) >= amountToWithdraw, "Not enough funds");
./LiquidityReserve.sol:192:        require(isReserveEnabled, "Not enabled yet");
./LiquidityReserve.sol:215:        require(isReserveEnabled, "Not enabled yet");
./Migration.sol:20-23:        require(_oldContract != address(0) && _newContract != address(0), "Invalid addressh);
./Yieldy.sol:58:        require(stakingContract == address(0), "Already Initialized");
./Yieldy.sol:59:        require(_stakingContract != address(0), "Invalid address");
./Yieldy.sol:83:        require(_totalSupply > 0, "Can't rebase if not circulating");
./Yieldy.sol:96:            require(rebasingCreditsPerToken > 0, "Invalid change in supply");
./Yieldy.sol:187:        require(_to != address(0), "Invalid address");
./Yieldy.sol:190:        require(creditAmount <= creditBalances[msg.sender], "Not enough funds");
./Yieldy.sol:210:        require(_allowances[_from][msg.sender] >= _value, "Allowance too low");
./Yieldy.sol:249:        require(_address != address(0), "Mint to the zero address");
./Yieldy.sol:257:        require(_totalSupply < MAX_SUPPLY, "Max supply");
./Yieldy.sol:279:        require(_address != address(0), "Burn from the zero address");
./Yieldy.sol:286:        require(currentCredits >= creditAmount, "Not enough balance");
./Staking.sol:54-63:        require(_stakingToken != address(0) && _yieldyToken != address(0) && _tokeToken != address(0) && _tokePool != address(0) && _tokeManager != address(0) && _tokeReward != address(0) && _liquidityReserve != address(0), "Invalid address");
./Staking.sol:118:        require(_recipient.amount > 0, "Must enter valid amount");
./Staking.sol:143:        require(_claimAddress != address(0), "Invalid address");
./Staking.sol:408:        require(!isStakingPaused, "Staking is paused");
./Staking.sol:410:        require(_amount > 0, "Must have valid amount");
./Staking.sol:527-530:        require(_amount <= walletBalance + warmUpBalance, "Insufficient Balance");
./Staking.sol:572:        require(_amount > 0, "Invalid amount");
./Staking.sol:574-577:        require(!isUnstakingPaused && !isInstantUnstakingPaused, "Unstaking is paused");
./Staking.sol:586:        require(reserveBalance >= _amount, "Not enough funds in reserve");
./Staking.sol:604:        require(_amount > 0, "Invalid amount");
./Staking.sol:605-609:        require(CURVE_POOL != address(0) && (curvePoolFrom == 1 || curvePoolTo == 1), "Invalid Curve Pool");
./Staking.sol:611-614:        require(!isUnstakingPaused && !isInstantUnstakingPaused, "Unstaking is paused");
./Staking.sol:644:            require(from == 1 || to == 1, "Invalid Curve Pool");
./Staking.sol:676:        require(!isUnstakingPaused, "Unstaking is paused");