Yieldy contest - 0x29A's results

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/BatchRequests.sol#L16 https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/BatchRequests.sol#L36 https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/BatchRequests.sol#L91

Vulnerability details

Impact

There is possible to get a out of gas issue while iterating the for loop. Please take a look to this link; https://github.com/wissalHaji/solidity-coding-advices/blob/master/best-practices/be-careful-with-loops.md

Proof of Concept

Lets say i want to run the function on BatchRequests.sol#L14 and i got a lot of contracts pending for withdrawal.

Tools Used

Manual revision

Recommended Mitigation Steps

Use this pattern;

    /**
        @notice sendWithdrawalRequests on all addresses in contracts
     */
    function sendWithdrawalRequests(uint256 from, uint256 to) external {
        uint256 contractsLength = contracts.length;
        require(from < contractsLength, "Invalid from");
        require(to <= contractsLength, "Invalid to");
        for (uint256 i = from; i < to; ) {
            if (
                contracts[i] != address(0) &&
                IStaking(contracts[i]).canBatchTransactions()
            ) {
                IStaking(contracts[i]).sendWithdrawalRequests();
            }
            unchecked {
                ++i;
            }
        }
    }

ERC20PermitUpgradeable.sol

[Q] Shadow variable name

Consider rename variable name to name_ on ERC20PermitUpgradeable.sol#L53-L54

diff --git a/src/libraries/ERC20PermitUpgradeable.sol b/src/libraries/ERC20PermitUpgradeable.sol
index 474c25e..c2ec570 100644
--- a/src/libraries/ERC20PermitUpgradeable.sol
+++ b/src/libraries/ERC20PermitUpgradeable.sol
@@ -50,8 +50,8 @@ abstract contract ERC20PermitUpgradeable is
      *
      * It's a good idea to use the same `name` that is defined as the ERC20 token name.
      */
-    function __ERC20Permit_init(string memory name) internal onlyInitializing {
-        __EIP712_init_unchained(name, "1");
+    function __ERC20Permit_init(string memory name_) internal onlyInitializing {
+        __EIP712_init_unchained(name_, "1");
     }
 
     function __ERC20Permit_init_unchained(string memory)

BatchRequests.sol

[L] Missing address(0) validation

Validate that variable _address is no address(0) on BatchRequests.sol#L82

LiquidityReserve.sol

[Q] Fee _fee can be equal to BASIS_POINTS (100% fee)

LiquidityReserve.sol#L94 The owner can set a 100% fee, perhaps a lower threashold is more trustable

[Q] Could happen a divition by 0? if totalLockedValue is 0

On LiquidityReserve.sol#L149-L152 if stakingTokenBalance = 0, rewardTokenBalance = 0 and coolDownAmount = 0, then totalLockedValue is 0 and you would end with a divition by 0 error. Recommendation, add if to check value and return 0

        uint256 totalLockedValue = stakingTokenBalance +
            rewardTokenBalance +
            coolDownAmount;
        if (totalLockedValue == 0) {
            return 0;
        }
        uint256 convertedAmount = (_amount * totalLockedValue) / lrFoxSupply;

Similar issue on LiquidityReserve.sol#L116-L120 but here you should use a require to get a readable error;

        uint256 totalLockedValue = stakingTokenBalance +
            rewardTokenBalance +
            coolDownAmount;
        require(totalLockedValue != 0, "TLV is 0");
        uint256 amountToMint = (_amount * lrFoxSupply) / totalLockedValue;

[Q] Could happen a divition by 0? if totalSupply() is 0

LiquidityReserve.sol#L149-L152 Recommendation add a if to check to lrFoxSupply

        if (lrFoxSupply == 0) {
            return 0;
        }
        uint256 totalLockedValue = stakingTokenBalance +
            rewardTokenBalance +
            coolDownAmount;
        uint256 convertedAmount = (_amount * totalLockedValue) / lrFoxSupply;

LiquidityReserveStorage.sol

[Q] Number consistency

You ar using 10000 for BASIS_POINTS, but then you use 10**3 for MINIMUM_LIQUIDITY on LiquidityReserveStorage.sol#L9. Change 10**3 for 1000

Migration.sol

[L] Use safeTransferFrom instead of trasnferFrom to transfer ERC20 tokens

On Migration.sol#L48 use safeTrasnferFrom and change IYieldy(OLD_YIELDY_TOKEN) to IERC20Upgradeable(OLD_YIELDY_TOKEN)

Staking.sol

[L] Use .safeTransfer to transfer ERC20 token instead of .transfer

On Staking.sol#L471 change; IYieldy(YIELDY_TOKEN).transfer( to IERC20Upgradeable.safeTransfer(

[Q] Misspell on comments

A comment seems to got garbage charaters at the end on Staking.sol#L675

    // prevent unstaking if override due to vulnerabilities asdf

Should be;

    // prevent unstaking if override due to vulnerabilities

[L] Missing address(0) validation for _curvePool variable

Add address(0) check to Staking.sol#L157

[Q] Add threashold to _timestamp

To avoid a excesive lock up variable _timestamp should have determine max. Also _timestamp cant be equal to 0 Staking.sol#L246

[Q] Use convention for function signature

The convention is to use (address _user, uint256 _amount) as signature and you are using (uint256 _amount, address _user) on function _retrieveBalanceFromUser

[Q] Function refactor to look cleaner

Refactor to function setToAndFromCurve to make it a bit cleaner

diff --git a/src/contracts/Staking.sol b/src/contracts/Staking.sol
index 33d12c6..f83d866 100644
--- a/src/contracts/Staking.sol
+++ b/src/contracts/Staking.sol
@@ -630,24 +630,21 @@ contract Staking is OwnableUpgradeable, StakingStorage {
         @notice sets to and from coin indexes for curve exchange
      */
     function setToAndFromCurve() internal {
-        if (CURVE_POOL != address(0)) {
-            address address0 = ICurvePool(CURVE_POOL).coins(0);
-            address address1 = ICurvePool(CURVE_POOL).coins(1);
-            int128 from = 0;
-            int128 to = 0;
-
-            if (TOKE_POOL == address0 && STAKING_TOKEN == address1) {
-                to = 1;
-            } else if (TOKE_POOL == address1 && STAKING_TOKEN == address0) {
-                from = 1;
-            }
-            require(from == 1 || to == 1, "Invalid Curve Pool");
-
-            curvePoolFrom = from;
-            curvePoolTo = to;
-
-            emit LogSetCurvePool(CURVE_POOL, curvePoolTo, curvePoolFrom);
+        if (CURVE_POOL == address(0)) {
+            return;
         }
+        (address address0, address address1) = (ICurvePool(CURVE_POOL).coins(0), ICurvePool(CURVE_POOL).coins(1));
+        if (TOKE_POOL == address0 && STAKING_TOKEN == address1) {
+            curvePoolFrom = 0;
+            curvePoolTo = 1;
+        } else if (TOKE_POOL == address1 && STAKING_TOKEN == address0) {
+            curvePoolFrom = 1;
+            curvePoolTo = 0;
+        } else {
+            revert("Invalid Curve Pool");
+        }
+
+        emit LogSetCurvePool(CURVE_POOL, curvePoolTo, curvePoolFrom);
     }
 
     /**

YieldyStorage.sol

[Q] Use type().max instead of ~uint256(0);

On YieldyStorage.sol#L16-L18 consider replacing;

    uint256 internal constant MAX_UINT256 = ~uint256(0);

    uint256 internal constant MAX_SUPPLY = ~uint128(0); // (2^128) - 1

To;

    uint256 internal constant MAX_UINT256 = type(uint256).max;

    uint256 internal constant MAX_SUPPLY = type(uint128).max; // (2^128) - 1

BatchRequests.sol

[G] Cache contracts array

diff --git a/src/contracts/BatchRequests.sol b/src/contracts/BatchRequests.sol
index b608c1c..d5cedd9 100644
--- a/src/contracts/BatchRequests.sol
+++ b/src/contracts/BatchRequests.sol
@@ -12,13 +12,14 @@ contract BatchRequests is Ownable {
         @notice sendWithdrawalRequests on all addresses in contracts
      */
     function sendWithdrawalRequests() external {
-        uint256 contractsLength = contracts.length;
+        address[] memory _contracts = contracts;
+        uint256 contractsLength = _contracts.length;
         for (uint256 i; i < contractsLength; ) {
             if (
-                contracts[i] != address(0) &&
-                IStaking(contracts[i]).canBatchTransactions()
+                _contracts[i] != address(0) &&
+                IStaking(_contracts[i]).canBatchTransactions()
             ) {
-                IStaking(contracts[i]).sendWithdrawalRequests();
+                IStaking(_contracts[i]).sendWithdrawalRequests();
             }
             unchecked {
                 ++i;
@@ -31,11 +32,12 @@ contract BatchRequests is Ownable {
         @return (address, bool)[]
      */
     function canBatchContracts() external view returns (Batch[] memory) {
-        uint256 contractsLength = contracts.length;
+        address[] memory _contracts = contracts;
+        uint256 contractsLength = _contracts.length;
         Batch[] memory batch = new Batch[](contractsLength);
         for (uint256 i; i < contractsLength; ) {
-            bool canBatch = IStaking(contracts[i]).canBatchTransactions();
-            batch[i] = Batch(contracts[i], canBatch);
+            bool canBatch = IStaking(_contracts[i]).canBatchTransactions();
+            batch[i] = Batch(_contracts[i], canBatch);
             unchecked {
                 ++i;
             }
@@ -87,9 +89,10 @@ contract BatchRequests is Ownable {
         @param _address - address to remove
      */
     function removeAddress(address _address) external onlyOwner {
-        uint256 contractsLength = contracts.length;
+        address[] memory _contracts = contracts;
+        uint256 contractsLength = _contracts.length;
         for (uint256 i; i < contractsLength; ) {
-            if (contracts[i] == _address) {
+            if (_contracts[i] == _address) {
                 delete contracts[i];
             }
             unchecked {

LiquidityReserve.sol

[G] Unnecesary require check

The balance check on LiquidityReserve.sol#L162-L163 its nont necessary, _burn function will throguht a revert if msg.sender doesnt got enouth balance.

Migration.sol

[G] Remove interface IYieldy

Remove this line Migration.sol#L9 and replace IYieldy for IERC20Upgradeable

diff --git a/src/contracts/Migration.sol b/src/contracts/Migration.sol
index 868ac9b..8477724 100644
--- a/src/contracts/Migration.sol
+++ b/src/contracts/Migration.sol
@@ -6,8 +6,6 @@ import "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeab
 
 import "../interfaces/IStakingV1.sol";
 import "../interfaces/IStaking.sol";
-import "../interfaces/IYieldy.sol";
-
 contract Migration {
     using SafeERC20Upgradeable for IERC20Upgradeable;
 
@@ -28,7 +26,7 @@ contract Migration {
         OLD_YIELDY_TOKEN = IStakingV1(_oldContract).REWARD_TOKEN();
         address stakingToken = IStaking(_newContract).STAKING_TOKEN();
 
-        IYieldy(OLD_YIELDY_TOKEN).approve(_oldContract, type(uint256).max);
+        IERC20Upgradeable(OLD_YIELDY_TOKEN).approve(_oldContract, type(uint256).max);
         IERC20Upgradeable(stakingToken).approve(
             _newContract,
             type(uint256).max
@@ -41,11 +39,11 @@ contract Migration {
         Note: user needs to approve reward token spend before calling this
      */
     function moveFundsToUpgradedContract() external {
-        uint256 userWalletBalance = IYieldy(OLD_YIELDY_TOKEN).balanceOf(
+        uint256 userWalletBalance = IERC20Upgradeable(OLD_YIELDY_TOKEN).balanceOf(
             msg.sender
         );
 
-        IYieldy(OLD_YIELDY_TOKEN).transferFrom(
+        IERC20Upgradeable(OLD_YIELDY_TOKEN).transferFrom(
             msg.sender,
             address(this),
             userWalletBalance

StakingStorage.sol

[G] COW_SETTLEMENT and COW_RELAYER seem to be constants and should be declared as constanst on StakingStorage.sol#L18-L19

COW_SETTLEMENT and COW_RELAYER are fixed addresses; Staking.sol#L73-L74

Suggestion;

diff --git a/src/contracts/Staking.sol b/src/contracts/Staking.sol
index 33d12c6..a44187c 100644
--- a/src/contracts/Staking.sol
+++ b/src/contracts/Staking.sol
@@ -70,8 +70,6 @@ contract Staking is OwnableUpgradeable, StakingStorage {
         LIQUIDITY_RESERVE = _liquidityReserve;
         FEE_ADDRESS = _feeAddress;
         CURVE_POOL = _curvePool;
-        COW_SETTLEMENT = 0x9008D19f58AAbD9eD0D60971565AA8510560ab41;
-        COW_RELAYER = 0xC92E8bdf79f0507f65a392b0ab4667716BFE0110;
 
         timeLeftToRequestWithdrawal = 12 hours;
 
diff --git a/src/contracts/StakingStorage.sol b/src/contracts/StakingStorage.sol
index 21e8566..5d0b49d 100644
--- a/src/contracts/StakingStorage.sol
+++ b/src/contracts/StakingStorage.sol
@@ -15,9 +15,9 @@ contract StakingStorage {
     address public FEE_ADDRESS; // can be address(0)
     address public CURVE_POOL; // can be address(0)
 
-    address public COW_SETTLEMENT;
-    address public COW_RELAYER;
+    address constant public COW_SETTLEMENT = 0x9008D19f58AAbD9eD0D60971565AA8510560ab41;
+    address constant public COW_RELAYER = 0xC92E8bdf79f0507f65a392b0ab4667716BFE0110;

Staking.sol

[G] Use unchecked on safe math operations;

On Staking.sol#L706-L717 you could use unchecked to perform safe math operations

diff --git a/src/contracts/Staking.sol b/src/contracts/Staking.sol
index 33d12c6..fae5683 100644
--- a/src/contracts/Staking.sol
+++ b/src/contracts/Staking.sol
@@ -703,9 +703,11 @@ contract Staking is OwnableUpgradeable, StakingStorage {
         if (epoch.endTime <= block.timestamp) {
             IYieldy(YIELDY_TOKEN).rebase(epoch.distribute, epoch.number);
 
-            epoch.endTime = epoch.endTime + epoch.duration;
             epoch.timestamp = block.timestamp;
-            epoch.number++;
+            unchecked {
+                epoch.endTime = epoch.endTime + epoch.duration;
+                epoch.number++;
+            }
 
             uint256 balance = contractBalance();
             uint256 staked = IYieldy(YIELDY_TOKEN).totalSupply();
@@ -713,7 +715,9 @@ contract Staking is OwnableUpgradeable, StakingStorage {
             if (balance <= staked) {
                 epoch.distribute = 0;
             } else {
-                epoch.distribute = balance - staked;
+                unchecked {
+                    epoch.distribute = balance - staked;
+                }
             }
         }
     }

Use require(val != 0, "Message") instead of require(val > 0, "Message")

Using > 0 cost more gas than != 0 when used on a uint on a require statement.

This change saves 6 gas per instance

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