Yieldy contest - BowTiedWardens's results

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L467 https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L406-L450

Vulnerability details

Issue: griefing can happen if coolDownPeriod > 0 due to the fact that you can stake for someone else. Whenever a stake happens, the expiry variable increases with coolDownPeriod. This can be done either by watching the mempool and frontrun a stake when someone tries to claim or someone can just stake multiple times increasing the expiry to a very big number.

Consequences: a bad actor can stake a very small amount to increase someone's expiry, effectively griefing claim()

Affected Code

File: Staking.sol
406:     function stake(uint256 _amount, address _recipient) public {
...
439:             warmUpInfo[_recipient] = Claim({ 
440:                 amount: info.amount + _amount,
441:                 credits: info.credits +
442:                     IYieldy(YIELDY_TOKEN).creditsForTokenBalance(_amount),
443:                 expiry: epoch.number + warmUpPeriod
444:             });
...
450:     }

File: Staking.sol
465:     function claim(address _recipient) public {
466:         Claim memory info = warmUpInfo[_recipient];
467:         if (_isClaimAvailable(_recipient)) { // @audit-info [HIGH] 
468:             delete warmUpInfo[_recipient];
469: 
470:             if (info.credits > 0) {
471:                 IYieldy(YIELDY_TOKEN).transfer(
472:                     _recipient,
473:                     IYieldy(YIELDY_TOKEN).tokenBalanceForCredits(info.credits)
474:                 );
475:             }
476:         }
477:     }

Mitigations

Consider removing stake for another wallet or introduce a mechanism of staking just by whitelisted wallets

File: Staking.sol
443:                 expiry: epoch.number + warmUpPeriod

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L37

Vulnerability details

Issue: canBatchContracts will revert due to the fact that contracts[i] can contain address(0) as an address which will revert the whole call.

Affected Code

File: BatchRequests.sol
33:     function canBatchContracts() external view returns (Batch[] memory) {
34:         uint256 contractsLength = contracts.length;
35:         Batch[] memory batch = new Batch[](contractsLength);
36:         for (uint256 i; i < contractsLength; ) {
37:             bool canBatch = IStaking(contracts[i]).canBatchTransactions();
38:             batch[i] = Batch(contracts[i], canBatch);
39:             unchecked {
40:                 ++i;
41:             }
42:         }
43:         return batch;
44:     }

This, combined with this delete mechanism that just replaces elements in the array with 0, makes the situation even more likely to happen:

File: BatchRequests.sol
89:     function removeAddress(address _address) external onlyOwner {
90:         uint256 contractsLength = contracts.length;
91:         for (uint256 i; i < contractsLength; ) {
92:             if (contracts[i] == _address) {
- 93:                 delete contracts[i]; // @audit-info [LOW] Instead of doing an element deletion, it should be replaced with the last element, then have the last element popped 
+ 93:                 contracts[i] = contracts[contractsLength - 1];
+ 93:                 contracts.pop();
94:             }
95:             unchecked {
96:                 ++i;
97:             }
98:         }
99:     }

Mitigations

Consider adding an address(0) check to skip the contracts values that are empty

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L406

Vulnerability details

Issue: there is a huge arb opportunity for people who deposit 1 block before the rebase()

Consequences: then they can call instantUnstakeReserve or instantUnstakeCurve to unstake the staked amount, in this way the profit that needs to be distributed on the next rebase increases, he also messes up the rewards for the other holders as the instantUnstakeReserve does not burn the YIELD_TOKEN. Even if there is a fee on the instantUnstakeReserve, there is still a chance for profit.

Affected Code

File: Staking.sol
406:     function stake(uint256 _amount, address _recipient) public { // @audit-info [HIGH] 
407:         // if override staking, then don't allow stake
408:         require(!isStakingPaused, "Staking is paused");
409:         // amount must be non zero
410:         require(_amount > 0, "Must have valid amount");
411: 
412:         uint256 yieldyTotalSupply = IYieldy(YIELDY_TOKEN).totalSupply();
413: 
414:         // Don't rebase unless tokens are already staked or could get locked out of staking
415:         if (yieldyTotalSupply > 0) {
416:             rebase();
417:         }
418: 
419:         IERC20Upgradeable(STAKING_TOKEN).safeTransferFrom(
420:             msg.sender,
421:             address(this),
422:             _amount
423:         );
424: 
425:         Claim storage info = warmUpInfo[_recipient];
426: 
427:         // if claim is available then auto claim tokens
428:         if (_isClaimAvailable(_recipient)) {
429:             claim(_recipient);
430:         }
431: 
432:         _depositToTokemak(_amount);
433: 
434:         // skip adding to warmup contract if period is 0
435:         if (warmUpPeriod == 0) {
436:             IYieldy(YIELDY_TOKEN).mint(_recipient, _amount);
437:         } else {
438:             // create a claim and mint tokens so a user can claim them once warm up has passed
439:             warmUpInfo[_recipient] = Claim({
440:                 amount: info.amount + _amount,
441:                 credits: info.credits +
442:                     IYieldy(YIELDY_TOKEN).creditsForTokenBalance(_amount),
443:                 expiry: epoch.number + warmUpPeriod
444:             });
445: 
446:             IYieldy(YIELDY_TOKEN).mint(address(this), _amount);
447:         }
448: 
449:         sendWithdrawalRequests();
450:     }

Mitigations

Burn the YIELD_TOKEN amount in the instantUnstakeReserve

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L153-L160 https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L632-L651

Vulnerability details

• Staking.sol#setCurvePool

File: Staking.sol
153:     /**
154:         @notice sets the curve pool address
155:         @param _curvePool uint
156:      */
157:     function setCurvePool(address _curvePool) external onlyOwner {
158:         CURVE_POOL = _curvePool;
159:         setToAndFromCurve();
160:     }

• Staking.sol#setToAndFromCurve

File: Staking.sol
632:     function setToAndFromCurve() internal {
633:         if (CURVE_POOL != address(0)) {
634:             address address0 = ICurvePool(CURVE_POOL).coins(0);
635:             address address1 = ICurvePool(CURVE_POOL).coins(1);
636:             int128 from = 0;
637:             int128 to = 0;
638: 
639:             if (TOKE_POOL == address0 && STAKING_TOKEN == address1) {
640:                 to = 1;
641:             } else if (TOKE_POOL == address1 && STAKING_TOKEN == address0) {
642:                 from = 1;
643:             }
644:             require(from == 1 || to == 1, "Invalid Curve Pool");
645: 
646:             curvePoolFrom = from;
647:             curvePoolTo = to;
648: 
649:             emit LogSetCurvePool(CURVE_POOL, curvePoolTo, curvePoolFrom);
650:         }
651:     }

Consider doing just like in Staking.sol#initialize:

File: Staking.sol
78:         if (CURVE_POOL != address(0)) {
79:             IERC20(TOKE_POOL).approve(CURVE_POOL, type(uint256).max);
80:             setToAndFromCurve();
81:         }

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Yieldy.sol#L110-L114 https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Yieldy.sol#L97-L100

Vulnerability details

_storeRebase()'s signature is as such:

• Yieldy.sol#_storeRebase()

File: Yieldy.sol
104:     /**
105:         @notice emits event with data about rebase
106:         @param _previousCirculating uint
107:         @param _profit uint
108:         @param _epoch uint
109:      */
110:     function _storeRebase(
111:         uint256 _previousCirculating,
112:         uint256 _profit,
113:         uint256 _epoch
114:     ) internal {

However, instead of being called with the expected _previousCirculating value, it's called with the current circulation value:

• Yieldy.sol#rebase()

File: Yieldy.sol
89:             uint256 updatedTotalSupply = currentTotalSupply + _profit;
...
103:             _totalSupply = updatedTotalSupply;
104: 
105:             _storeRebase(updatedTotalSupply, _profit, _epoch); // @audit-info this should be currentTotalSupply otherwise previous = current

As a consequence, the functionality isn't doing what it was created for.

Mitigation

Consider calling _storeRebase() with currentTotalSupply:

File: Yieldy.sol
- 105:             _storeRebase(updatedTotalSupply, _profit, _epoch);
+ 105:             _storeRebase(currentTotalSupply, _profit, _epoch);

Overview

Table of Contents

• 1. Low Risk Issues
  • 1.1. Add constructor initializers
  • 1.2. Unbounded loop on array can lead to DoS
  • 1.3. Prevent accidentally burning tokens
  • 1.4. Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions
  • 1.5. Missing events for critical parameters that change the state
  • 1.6. All initialize() functions are front-runnable in the solution
  • 1.7. Use a 2-step ownership transfer pattern
  • 1.8. Instead of doing an element deletion, it should be replaced with the last element, then have the last element popped in removeAddress()
  • 1.9. external/public functions without access-control that modify the state should be nonReentrant
• 2. Non-Critical Issues
  • 2.1. COW parameters are hardcoded
  • 2.2. Unused constant
  • 2.3. Typos

1. Low Risk Issues

1.1. Add constructor initializers

As per OpenZeppelin’s (OZ) recommendation, “The guidelines are now to make it impossible for anyone to run initialize on an implementation contract, by adding an empty constructor with the initializer modifier. So the implementation contract gets initialized automatically upon deployment.”

Note that this behaviour is also incorporated the OZ Wizard since the UUPS vulnerability discovery: “Additionally, we modified the code generated by the Wizard 19 to include a constructor that automatically initializes the implementation when deployed.”

Furthermore, this thwarts any attempts to frontrun the initialization tx of these contracts:

src/contracts/LiquidityReserve.sol:
  36      function initialize(
  37          string memory _tokenName,
  38          string memory _tokenSymbol,
  39          address _stakingToken,
  40          address _rewardToken
  41:     ) external initializer {

src/contracts/Staking.sol:
  38      function initialize(
  39          address _stakingToken,
  40          address _yieldyToken,
  41          address _tokeToken,
  42          address _tokePool,
  43          address _tokeManager,
  44          address _tokeReward,
  45          address _liquidityReserve,
  46          address _feeAddress,
  47          address _curvePool,
  48          uint256 _epochDuration,
  49          uint256 _firstEpochEndTime
  50:     ) external initializer {

src/contracts/Yieldy.sol:
  29      function initialize(
  30          string memory _tokenName,
  31          string memory _tokenSymbol,
  32          uint8 _decimal
  33:     ) external initializer {

1.2. Unbounded loop on array can lead to DoS

As these arrays can grow quite large (only push operations, no pop), the transaction's gas cost could exceed the block gas limit and make it impossible to call the impacted functions at all.

BatchRequests.sol:16:        for (uint256 i; i < contractsLength; ) {
BatchRequests.sol:36:        for (uint256 i; i < contractsLength; ) {
BatchRequests.sol:82:        contracts.push(_address);
BatchRequests.sol:91:        for (uint256 i; i < contractsLength; ) {

Consider introducing a reasonable upper limit based on block gas limits and adding a method to remove elements in the array.

1.3. Prevent accidentally burning tokens

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Consider adding a check to prevent accidentally burning tokens here:

• LiquidityReserve.sol: instantUnstake()#_recipient

188:     function instantUnstake(uint256 _amount, address _recipient)
189:         external
190:         onlyStakingContract
191:     {
192:         require(isReserveEnabled, "Not enabled yet");
+ 192:         require(_recipient != address(0), "Invalid address");
...
204:         IERC20Upgradeable(stakingToken).safeTransfer(
205:             _recipient, //@audit _recipient can be address(0)
206:             amountMinusFee
207:         );
208:         unstakeAllRewardTokens();
209:     }

Notice that such a check already exists in the solution:

File: Staking.sol
141:     function transferToke(address _claimAddress) external onlyOwner {
142:         // _claimAddress can't be 0x0
143:         require(_claimAddress != address(0), "Invalid address");
144:         uint256 totalTokeAmount = IERC20Upgradeable(TOKE_TOKEN).balanceOf(
145:             address(this)
146:         );
147:         IERC20Upgradeable(TOKE_TOKEN).safeTransfer(
148:             _claimAddress,
149:             totalTokeAmount
150:         );
151:     }

Also notice that other places in the solution already have some kind of checks to prevent accidentally burning tokens. Consider adding the suggested zero-address check

1.4. Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

See this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.

Staking.sol:18:contract Staking is OwnableUpgradeable, StakingStorage {

1.5. Missing events for critical parameters that change the state

• src/contracts/Staking.sol:

  246:     function setTimeLeftToRequestWithdrawal(uint256 _timestamp)
  247          external
  248          onlyOwner
  249      {
  250          timeLeftToRequestWithdrawal = _timestamp;
  251      }

• src/contracts/Yieldy.sol:

  69:     function _setIndex(uint256 _index) internal {
  70          index = creditsForTokenBalance(_index);
  71      }

1.6. All initialize() functions are front-runnable in the solution

Consider adding some access control to them or deploying atomically or using constructor initializer:

LiquidityReserve.sol:36:    function initialize(
Staking.sol:38:    function initialize(
Yieldy.sol:29:    function initialize(

1.7. Use a 2-step ownership transfer pattern

Contracts inheriting from OpenZeppelin's libraries have the default transferOwnership() function (a one-step process). It's possible that the onlyOwner role mistakenly transfers ownership to a wrong address, resulting in a loss of the onlyOwner role. Consider overriding the default transferOwnership() function to first nominate an address as the pendingOwner and implementing an acceptOwnership() function which is called by the pendingOwner to confirm the transfer.

BatchRequests.sol:6:import "@openzeppelin/contracts/access/Ownable.sol";
BatchRequests.sol:8:contract BatchRequests is Ownable {

LiquidityReserve.sol:7:import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
LiquidityReserve.sol:16:    OwnableUpgradeable,

Staking.sol:6:import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
Staking.sol:18:contract Staking is OwnableUpgradeable, StakingStorage {

1.8. Instead of doing an element deletion, it should be replaced with the last element, then have the last element popped in removeAddress()

The contracts array will grow every time a new address is pushed as delete does not remove the element. The delete operation will just replace the previous element with address(0).

File: BatchRequests.sol
89:     function removeAddress(address _address) external onlyOwner {
90:         uint256 contractsLength = contracts.length;
91:         for (uint256 i; i < contractsLength; ) {
92:             if (contracts[i] == _address) {
- 93:                 delete contracts[i]; // @audit-info [LOW] Instead of doing an element deletion, it should be replaced with the last element, then have the last element popped 
+ 93:                 contracts[i] = contracts[contractsLength - 1];
+ 93:                 contracts.pop();
94:             }
95:             unchecked {
96:                 ++i;
97:             }
98:         }
99:     }

1.9. external/public functions without access-control that modify the state should be nonReentrant

As a best practice to reduce the attack surface, every external/public function that modifies the state of the smart contract and that is free to be called by any user should be nonReentrant. This modifier isn't used at all in the solution.

2. Non-Critical Issues

2.1. COW parameters are hardcoded

File: Staking.sol
73:         COW_SETTLEMENT = 0x9008D19f58AAbD9eD0D60971565AA8510560ab41;
74:         COW_RELAYER = 0xC92E8bdf79f0507f65a392b0ab4667716BFE0110;

2.2. Unused constant

As this constant isn't used, it should be deleted for readability reasons:

File: YieldyStorage.sol
16:     uint256 internal constant MAX_UINT256 = ~uint256(0); // @audit-info [INFO] not used

2.3. Typos

• Lot of references to FOX which need to be yieldy

src/contracts/LiquidityReserve.sol:
  112:         uint256 lrFoxSupply = totalSupply();
  120:         uint256 amountToMint = (_amount * lrFoxSupply) / totalLockedValue;
  139:         uint256 lrFoxSupply = totalSupply();
  152:         uint256 convertedAmount = (_amount * totalLockedValue) / lrFoxSupply;

src/contracts/Migration.sol:
  39:         @notice unstake user's FOXy from the old Staking contract and immediately

src/contracts/Staking.sol:
  138:         @dev used so DAO can get TOKE and manually trade to return FOX to the staking contract
  367:         @notice owner function to requestWithdraw all FOX from tokemak in case of an attack on tokemak

• "asdf"

File: Staking.sol
675:         // prevent unstaking if override due to vulnerabilities asdf

Overview

Table of Contents:

• 1. Duplicated external function call
• 2. Wrong use of the memory keyword for a Struct
• 3. Caching storage values in memory
• 4. Avoid emitting a storage variable when a memory value is available
• 5. Unchecking arithmetics operations that can't underflow/overflow
• 6. LiquidityReserveStorage : Tightly pack storage variables
• 7. YieldyStorage : Tightly pack storage variables
• 8. Duplicated conditions should be refactored to a modifier or function to save deployment costs
• 9. A modifier used only once and not being inherited should be inlined to save gas
• 10. Pre-Solidity 0.8.13: > 0 is less efficient than != 0 for unsigned integers (with proof)
• 11. >= is cheaper than > (and <= cheaper than <)
• 12. Splitting require() statements that use && saves gas
• 13. Using private rather than public for constants saves gas
• 14. Amounts should be checked for 0 before calling a transfer
• 15. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)
• 16. Public functions to external
• 17. It costs more gas to initialize variables with their default value than letting the default value be applied
• 18. Upgrade pragma
• 19. Use Custom Errors instead of Revert Strings to save Gas
• 20. Functions guaranteed to revert when called by normal users can be marked payable
• 21. Use 1000 rather than exponentiation 10**3

1. Duplicated external function call

External function calls are expensive. This one seems like a copy-paste error:

• Staking.sol#initialize()

File: Staking.sol
84:         IERC20Upgradeable(YIELDY_TOKEN).approve(
85:             LIQUIDITY_RESERVE,
86:             type(uint256).max
87:         );
- 88:         IERC20Upgradeable(YIELDY_TOKEN).approve(
- 89:             LIQUIDITY_RESERVE,
- 90:             type(uint256).max
- 91:         );

2. Wrong use of the memory keyword for a Struct

When copying a state struct in memory, there are as many SLOADs and MSTOREs as there are slots. When reading the whole struct multiple times is not needed, it's better to actually only read the relevant field(s). When only some of the fields are read several times, these particular values should be cached instead of the whole state struct.

Here, the storage keyword should be used instead of memory:

• Saving 1 STORE and 1 MSTORE: https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L259-L266

File: Staking.sol
259:     function _isClaimAvailable(address _recipient)
260:         internal
261:         view
262:         returns (bool)
263:     {
- 264:         Claim memory info = warmUpInfo[_recipient]; //@audit 2 SLOADs + 2 MSTOREs
+ 264:         Claim storage info = warmUpInfo[_recipient]; //@audit reference-fetching helper (loved by the Optimizer)
+ 264:         uint256 _expiry = info.expiry; //@audit 1 SLOAD, 1 MSTORE
- 265:         return epoch.number >= info.expiry && info.expiry != 0;
+ 265:         return epoch.number >= _expiry && _expiry != 0;
266:     }

• Saving 2 MSTOREs and 2 MLOADs: https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L281-L289

File: Staking.sol
- 281:         RequestedWithdrawalInfo memory requestedWithdrawals = tokePoolContract
+ 281:         RequestedWithdrawalInfo storage requestedWithdrawals = tokePoolContract
282:             .requestedWithdrawals(address(this));
283:         uint256 currentCycleIndex = tokeManager.getCurrentCycleIndex();
284:         return
285:             epoch.number >= info.expiry &&
286:             info.expiry != 0 &&
287:             info.amount != 0 &&
288:             ((requestedWithdrawals.minCycle <= currentCycleIndex && //@audit requestedWithdrawals.minCycle only accessed once
289:                 requestedWithdrawals.amount + withdrawalAmount >=  //@audit requestedWithdrawals.amount only accessed once

• Saving 1 SLOAD and 1 MSTORE: https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L466-L474

File: Staking.sol
465:     function claim(address _recipient) public {
- 466:         Claim memory info = warmUpInfo[_recipient]; //@audit 2 SLOADs + 2 MSTOREs
+ 466:         Claim storage info = warmUpInfo[_recipient]; //@audit reference-fetching helper (loved by the Optimizer)
+ 466:         uint256 _credits = info.expiry; //@audit 1 SLOAD, 1 MSTORE
467:         if (_isClaimAvailable(_recipient)) {
468:             delete warmUpInfo[_recipient];
469: 
- 470:             if (info.credits > 0) {
+ 470:             if (_credits > 0) {
471:                 IYieldy(YIELDY_TOKEN).transfer(
472:                     _recipient,
- 473:                     IYieldy(YIELDY_TOKEN).tokenBalanceForCredits(info.credits)
+ 473:                     IYieldy(YIELDY_TOKEN).tokenBalanceForCredits(_credits)
474:                 );
475:             }
476:         }
477:     }

3. Caching storage values in memory

The code can be optimized by minimizing the number of SLOADs.

SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.

• contracts[i]

File: BatchRequests.sol
18:                 contracts[i] != address(0) &&
19:                 IStaking(contracts[i]).canBatchTransactions()
20:             ) {
21:                 IStaking(contracts[i]).sendWithdrawalRequests();

• contracts[i]

File: BatchRequests.sol
37:             bool canBatch = IStaking(contracts[i]).canBatchTransactions();
38:             batch[i] = Batch(contracts[i], canBatch);

• contracts[_index]

File: BatchRequests.sol
56:             contracts[_index],
57:             IStaking(contracts[_index]).canBatchTransactions()

• stakingToken

File: LiquidityReserve.sol
64:         uint256 stakingTokenBalance = IERC20Upgradeable(stakingToken).balanceOf(
...
75:         IERC20Upgradeable(stakingToken).safeTransferFrom(

• stakingContract

File: LiquidityReserve.sol
72:         stakingContract = _stakingContract;
...
81:         IERC20Upgradeable(rewardToken).approve(
- 82:             stakingContract,
+ 82:             _stakingContract,

• stakingToken

File: LiquidityReserve.sol
106:         uint256 stakingTokenBalance = IERC20Upgradeable(stakingToken).balanceOf(
...
121:         IERC20Upgradeable(stakingToken).safeTransferFrom(

• stakingToken

File: LiquidityReserve.sol
171:             IERC20Upgradeable(stakingToken).balanceOf(address(this)) >=
...
177:         IERC20Upgradeable(stakingToken).safeTransfer(

• affiliateFee and FEE_ADDRESS

File: Staking.sol
129:     function _sendAffiliateFee(uint256 _amount) internal {
130:         if (affiliateFee != 0 && FEE_ADDRESS != address(0)) {
131:             uint256 feeAmount = (_amount * affiliateFee) / BASIS_POINTS;
132:             IERC20Upgradeable(TOKE_TOKEN).safeTransfer(FEE_ADDRESS, feeAmount);
133:         }
134:     }

• TOKE_TOKEN

File: Staking.sol
144:         uint256 totalTokeAmount = IERC20Upgradeable(TOKE_TOKEN).balanceOf(
145:             address(this)
146:         );
147:         IERC20Upgradeable(TOKE_TOKEN).safeTransfer(

• requestWithdrawalAmount

File: Staking.sol
392:             if (requestWithdrawalAmount > 0) {
393:                 _requestWithdrawalFromTokemak(requestWithdrawalAmount);
394:             }

• YIELDY_TOKEN

File: Staking.sol
519:         uint256 walletBalance = IERC20Upgradeable(YIELDY_TOKEN).balanceOf(
...
522:         uint256 warmUpBalance = IYieldy(YIELDY_TOKEN).tokenBalanceForCredits(
...
545:                     IYieldy(YIELDY_TOKEN).creditsForTokenBalance(_amount);
546:                 uint256 remainingAmount = IYieldy(YIELDY_TOKEN)
...
559:             IERC20Upgradeable(YIELDY_TOKEN).safeTransferFrom(

• LIQUIDITY_RESERVE

File: Staking.sol
583:             LIQUIDITY_RESERVE
...
588:         ILiquidityReserve(LIQUIDITY_RESERVE).instantUnstake(

• CURVE_POOL / STAKING_TOKEN / TOKE_POOL

File: Staking.sol
633:         if (CURVE_POOL != address(0)) {
634:             address address0 = ICurvePool(CURVE_POOL).coins(0);
635:             address address1 = ICurvePool(CURVE_POOL).coins(1);
636:             int128 from = 0;
637:             int128 to = 0;
638: 
639:             if (TOKE_POOL == address0 && STAKING_TOKEN == address1) {
640:                 to = 1;
641:             } else if (TOKE_POOL == address1 && STAKING_TOKEN == address0) {
642:                 from = 1;
643:             }
644:             require(from == 1 || to == 1, "Invalid Curve Pool");
645: 
646:             curvePoolFrom = from;
647:             curvePoolTo = to;
648: 
649:             emit LogSetCurvePool(CURVE_POOL, curvePoolTo, curvePoolFrom);
650:         }

• _totalSupply

File: Yieldy.sol
82:         uint256 currentTotalSupply = _totalSupply;
- 83:         require(_totalSupply > 0, "Can't rebase if not circulating");
+ 83:         require(currentTotalSupply > 0, "Can't rebase if not circulating");

• _totalSupply

File: Yieldy.sol
122:                 totalStakedAfter: _totalSupply,
...
129:         emit LogSupply(_epoch, block.timestamp, _totalSupply);

4. Avoid emitting a storage variable when a memory value is available

When they are the same, consider emitting the memory value instead of the storage value:

File: Staking.sol
646:             curvePoolFrom = from;
647:             curvePoolTo = to;
648: 
- 649:             emit LogSetCurvePool(CURVE_POOL, curvePoolTo, curvePoolFrom);
+ 649:             emit LogSetCurvePool(CURVE_POOL, to, from);

5. Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

Consider wrapping with an unchecked block here:

File: Yieldy.sol
210:         require(_allowances[_from][msg.sender] >= _value, "Allowance too low");
211: 
- 212:         uint256 newValue = _allowances[_from][msg.sender] - _value;
+ 212:         unchecked { uint256 newValue = _allowances[_from][msg.sender] - _value; }

File: Yieldy.sol
190:         require(creditAmount <= creditBalances[msg.sender], "Not enough funds");
191: 
- 192:         creditBalances[msg.sender] = creditBalances[msg.sender] - creditAmount;
+ 192:         unchecked { creditBalances[msg.sender] = creditBalances[msg.sender] - creditAmount; }

File: Staking.sol
713:             if (balance <= staked) {
714:                 epoch.distribute = 0;
715:             } else {
- 716:                 epoch.distribute = balance - staked;
+ 716:                 unchecked { epoch.distribute = balance - staked; }
717:             }

File: LiquidityReserve.sol
- 196:         uint256 amountMinusFee = _amount - ((_amount * fee) / BASIS_POINTS);
+ 196:         unchecked { uint256 amountMinusFee = _amount - ((_amount * fee) / BASIS_POINTS); }

6. LiquidityReserveStorage : Tightly pack storage variables

Here, variables can be tightly packed from to save 1 SLOT:

File: LiquidityReserveStorage.sol
04: contract LiquidityReserveStorage {
05:     address public stakingToken; // staking token address
06:     address public rewardToken; // reward token address
07:     address public stakingContract; // staking contract address
+ 8:     bool public isReserveEnabled; // ensures we are fully initialized
08:     uint256 public fee; // fee for instant unstaking
09:     uint256 public constant MINIMUM_LIQUIDITY = 10**3; // lock minimum stakingTokens for initial liquidity
10:     uint256 public constant BASIS_POINTS = 10000; // 100% in basis points
- 11:     bool public isReserveEnabled; // ensures we are fully initialized //@audit can be tightly packed
12: }

7. YieldyStorage : Tightly pack storage variables

Here, variables can be tightly packed from to save 1 SLOT:

File: YieldyStorage.sol
06: contract YieldyStorage {
07:     address public stakingContract;
+ 08:     uint8 internal decimal;
08:     Rebase[] public rebases;
09:     uint256 public index;
10:     bytes32 public constant ADMIN_ROLE = keccak256("ADMIN");
11:     bytes32 public constant MINTER_BURNER_ROLE =
12:         keccak256("MINTER_BURNER_ROLE");
13:     bytes32 public constant REBASE_ROLE = keccak256("REBASE_ROLE");
14: 
15:     uint256 internal WAD;
16:     uint256 internal constant MAX_UINT256 = ~uint256(0);
17: 
18:     uint256 internal constant MAX_SUPPLY = ~uint128(0); // (2^128) - 1
19:     uint256 public rebasingCreditsPerToken; // gonsPerFragment (fragment == 1 token)
20:     uint256 public rebasingCredits; // total credits in system
21:     mapping(address => uint256) public creditBalances; // gonBalances (gon == credit)
22: 
- 23:     uint8 internal decimal; //@audit can be tightly packed
24: }

8. Duplicated conditions should be refactored to a modifier or function to save deployment costs

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

9. A modifier used only once and not being inherited should be inlined to save gas

Affected code:

src/contracts/LiquidityReserve.sol:
   24:     modifier onlyStakingContract() {

  188      function instantUnstake(uint256 _amount, address _recipient)
  189          external
  190:         onlyStakingContract
  191      {

10. Pre-Solidity 0.8.13: > 0 is less efficient than != 0 for unsigned integers (with proof)

Up until Solidity 0.8.13: != 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

Consider changing > 0 with != 0 here:

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");
Yieldy.sol:83:        require(_totalSupply > 0, "Can't rebase if not circulating");
Yieldy.sol:96:            require(rebasingCreditsPerToken > 0, "Invalid change in supply");

Also, please enable the Optimizer.

11. >= is cheaper than > (and <= cheaper than <)

Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas). This also holds true between <= and <.

Consider replacing strict inequalities with non-strict ones to save some gas here:

Staking.sol:324:        uint256 amountToRequest = balance < _amount ? balance : _amount;

12. Splitting require() statements that use && saves gas

If you're using the Optimizer at 200, instead of using the && operator in a single require statement to check multiple conditions, Consider using multiple require statements with 1 condition per require statement:

LiquidityReserve.sol:45:            _stakingToken != address(0) && _rewardToken != address(0),
Migration.sol:21:            _oldContract != address(0) && _newContract != address(0),
Staking.sol:55:            _stakingToken != address(0) &&
Staking.sol:56:                _yieldyToken != address(0) &&
Staking.sol:57:                _tokeToken != address(0) &&
Staking.sol:58:                _tokePool != address(0) &&
Staking.sol:59:                _tokeManager != address(0) &&
Staking.sol:60:                _tokeReward != address(0) &&
Staking.sol:575:            !isUnstakingPaused && !isInstantUnstakingPaused,
Staking.sol:606:            CURVE_POOL != address(0) &&
Staking.sol:612:            !isUnstakingPaused && !isInstantUnstakingPaused,

Please, note that this might not hold true at a higher number of runs for the Optimizer (10k). However, it indeed is true at 200.

13. Using private rather than public for constants saves gas

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table

LiquidityReserveStorage.sol:9:    uint256 public constant MINIMUM_LIQUIDITY = 10**3; // lock minimum stakingTokens for initial liquidity
LiquidityReserveStorage.sol:10:    uint256 public constant BASIS_POINTS = 10000; // 100% in basis points
StakingStorage.sol:39:    uint256 public constant BASIS_POINTS = 10000; // 100% in basis points

14. Amounts should be checked for 0 before calling a transfer

Checking non-zero transfer values can avoid an expensive external call and save gas.

Consider adding a non-zero-value check here:

LiquidityReserve.sol:121:        IERC20Upgradeable(stakingToken).safeTransferFrom(
LiquidityReserve.sol:177:        IERC20Upgradeable(stakingToken).safeTransfer(
LiquidityReserve.sol:198:        IERC20Upgradeable(rewardToken).safeTransferFrom(
LiquidityReserve.sol:204:        IERC20Upgradeable(stakingToken).safeTransfer(

15. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)

Pre-increments and pre-decrements are cheaper.

For a uint256 i variable, the following is true with the Optimizer enabled at 10k:

Increment:

• i += 1 is the most expensive form
• i++ costs 6 gas less than i += 1
• ++i costs 5 gas less than i++ (11 gas less than i += 1)

Decrement:

• i -= 1 is the most expensive form
• i-- costs 11 gas less than i -= 1
• --i costs 5 gas less than i-- (16 gas less than i -= 1)

Note that post-increments (or post-decrements) return the old value before incrementing or decrementing, hence the name post-increment:

uint i = 1;  
uint j = 2;
require(j == i++, "This will be false as i is incremented after the comparison");

However, pre-increments (or pre-decrements) return the new value:

uint i = 1;  
uint j = 2;
require(j == ++i, "This will be true as i is incremented before the comparison");

In the pre-increment case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2.

Affected code:

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

Consider using pre-increments and pre-decrements where they are relevant (meaning: not where post-increments/decrements logic are relevant).

16. Public functions to external

An external call cost is less expensive than one of a public function. The following functions could be set external to save gas and improve code quality (extracted from Slither).

Staking.sol:370:    function unstakeAllFromTokemak() public onlyOwner {

17. It costs more gas to initialize variables with their default value than letting the default value be applied

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Affected code:

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

Consider removing explicit initializations for default values.

18. Upgrade pragma

Using newer compiler versions and the optimizer give gas optimizations. Also, additional safety checks are available for free.

The advantages here are:

• Contract existence checks (>= 0.8.10): external calls skip contract existence checks if the external call has a return value

Consider upgrading here :

BatchRequests.sol:2:pragma solidity 0.8.9;
LiquidityReserve.sol:2:pragma solidity 0.8.9;
LiquidityReserveStorage.sol:2:pragma solidity 0.8.9;
Migration.sol:2:pragma solidity 0.8.9;
Staking.sol:2:pragma solidity 0.8.9;
StakingStorage.sol:2:pragma solidity 0.8.9;
Yieldy.sol:2:pragma solidity 0.8.9;
YieldyStorage.sol:2:pragma solidity 0.8.9;

19. Use Custom Errors instead of Revert Strings to save Gas

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deploy cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Consider replacing all revert strings with custom errors in the solution.

LiquidityReserve.sol:25:        require(msg.sender == stakingContract, "Not staking contract");
LiquidityReserve.sol:44:        require(
LiquidityReserve.sol:61:        require(!isReserveEnabled, "Already enabled");
LiquidityReserve.sol:62:        require(_stakingContract != address(0), "Invalid address");
LiquidityReserve.sol:68:        require(
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:        require(
LiquidityReserve.sol:192:        require(isReserveEnabled, "Not enabled yet");
LiquidityReserve.sol:215:        require(isReserveEnabled, "Not enabled yet");
Migration.sol:20:        require(
Staking.sol:54:        require(
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:        require(
Staking.sol:572:        require(_amount > 0, "Invalid amount");
Staking.sol:574:        require(
Staking.sol:586:        require(reserveBalance >= _amount, "Not enough funds in reserve");
Staking.sol:604:        require(_amount > 0, "Invalid amount");
Staking.sol:605:        require(
Staking.sol:611:        require(
Staking.sol:644:            require(from == 1 || to == 1, "Invalid Curve Pool");
Staking.sol:676:        require(!isUnstakingPaused, "Unstaking is paused");
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");

20. Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

BatchRequests.sol:81:    function addAddress(address _address) external onlyOwner {
BatchRequests.sol:89:    function removeAddress(address _address) external onlyOwner {
LiquidityReserve.sol:92:    function setFee(uint256 _fee) external onlyOwner {
Staking.sol:141:    function transferToke(address _claimAddress) external onlyOwner {
Staking.sol:157:    function setCurvePool(address _curvePool) external onlyOwner {
Staking.sol:167:    function setAffiliateFee(uint256 _affiliateFee) external onlyOwner {
Staking.sol:177:    function setAffiliateAddress(address _affiliateAddress) external onlyOwner {
Staking.sol:187:    function shouldPauseStaking(bool _shouldPause) public onlyOwner {
Staking.sol:197:    function shouldPauseUnstaking(bool _shouldPause) external onlyOwner {
Staking.sol:207:    function shouldPauseInstantUnstaking(bool _shouldPause) external onlyOwner {
Staking.sol:217:    function setEpochDuration(uint256 duration) external onlyOwner {
Staking.sol:226:    function setWarmUpPeriod(uint256 _vestingPeriod) external onlyOwner {
Staking.sol:235:    function setCoolDownPeriod(uint256 _vestingPeriod) external onlyOwner {
Staking.sol:370:    function unstakeAllFromTokemak() public onlyOwner {
Staking.sol:769:    function preSign(bytes calldata orderUid) external onlyOwner {

21. Use 1000 rather than exponentiation 10**3

1000 is readable enough and the cost of the exponentiation operation would be saved here:

+ LiquidityReserveStorage.sol:9:    uint256 public constant MINIMUM_LIQUIDITY = 10**3; // lock minimum stakingTokens for initial liquidity
- LiquidityReserveStorage.sol:9:    uint256 public constant MINIMUM_LIQUIDITY = 1000; // lock minimum stakingTokens for initial liquidity