Yieldy contest - shung's results

Lines of code

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

Vulnerability details

Impact

When warmUpPeriod is greater than 0, anyone can deny (i.e.: grief) any user from claiming their YIELDY_TOKENs from Staking.sol.

Proof of Concept

1. Admin sets the warmUpPeriod to 2.
2. A user deposits a decent amount STAKING_TOKENs, with the expectation that within 2 epochs they will be able to claim YIELDY_TOKENs and utilize them in DeFi.
3. After the rebase (i.e.: epoch.number++), a griefer stakes 1 wei of STAKING_TOKEN to the user.
4. This bumps up the warm up expiry.
5. Griefer repeats this every epoch, ensuring the user’s warm up period never expires.

Tools Used

Vim

Recommended Mitigation Steps

Do not allow callers to stake for others.

Lines of code

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

Vulnerability details

Impact

In Staking.sol, trying to set CURVE_POOL with the setCurvePool() function will not give the CURVE_POOL the approval to spend Staking.sol’s TOKE_POOL tokens. This will render instantUnstakeCurve() function unusable as the CURVE_POOL cannot spend TOKE_POOL tokens of Staking.sol.

Proof of Concept

1. Admin deploys Staking.sol by inputting zero address for _curvePool as there are no relevant curve pools on launch.
2. After a Curve pool for the staking token and the corresponding Toke token becomes popular, admin decides to define the Curve pool in Staking.sol.
3. Admin calls setCurvePool() with the Curve pool address, with the intention that now instant unstaking through the Curve pool will be possible.
4. Users try instant unstaking using instantUnstakeCurve() function, however all the transactions revert. This is because when executing CURVE_POOL.exchange(), the Curve pool lacks the approval to spend the Staking.sol’s TOKE_POOL tokens.
5. Admin realizes there is no way to make the instantUnstakeCurve() function ever operational.

Tools Used

VIM

Recommended Mitigation Steps

In setCurvePool(), first revoke the approval of the previous Curve pool, then approve the new Curve pool to spend TOKE_POOL tokens.

diff --git a/src/contracts/Staking.sol b/src/contracts/Staking.sol
index 33d12c6..9990230 100644
--- a/src/contracts/Staking.sol
+++ b/src/contracts/Staking.sol
@@ -155,8 +155,15 @@ contract Staking is OwnableUpgradeable, StakingStorage {
         @param _curvePool uint
      */
     function setCurvePool(address _curvePool) external onlyOwner {
+        address previousCurvePool = CURVE_POOL;
+        if (previousCurvePool != address(0)) {
+            IERC20(TOKE_POOL).approve(previousCurvePool, 0);
+        }
         CURVE_POOL = _curvePool;
-        setToAndFromCurve();
+        if (_curvePool != address(0)) {
+            IERC20(TOKE_POOL).approve(_curvePool, type(uint256).max);
+            setToAndFromCurve();
+        }
     }
 
     /**

Lines of code

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

Vulnerability details

Impact

In Yieldy.sol, rebase() function passes the wrong variable to _storeRebase(). This causes rebases array to store defective Rebase elements, and it also causes LogRebase event to output wrong values. This would cause indexers and user interfaces provide invalid information to users and admins, leading to user and management issues. Other critical consequences can occur if other contracts decide to use these stored values.

Proof of Concept

There can be multiple ways this can cause issues. One of them is an external contract utilizing the faulty storage values.

When rebasing, rebase() function passes updatedTotalSupply instead of currentTotalSupply to _storeRebase() function. _storeRebase(), using the updatedTotalSupply, calculates rebasePercent incorrectly, which gets stored as a property of the rebases element. If an external contract depends on rebases[index].rebase (rebasePercent), it will have issues.

Tools Used

Vim

Recommended Mitigation Steps

Simply pass the correct variable.

diff --git a/src/contracts/Yieldy.sol b/src/contracts/Yieldy.sol
index e459d74..882e5cb 100644
--- a/src/contracts/Yieldy.sol
+++ b/src/contracts/Yieldy.sol
@@ -97,7 +97,7 @@ contract Yieldy is

             _totalSupply = updatedTotalSupply;

-            _storeRebase(updatedTotalSupply, _profit, _epoch);
+            _storeRebase(currentTotalSupply, _profit, _epoch);
         }
     }

Lines of code

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

Vulnerability details

Impact

An invalid affiliateFee amount set by owner can result in all accumulated protocol revenue to be sent to the affiliate (FEE_ADDRESS).

Proof of Concept

1. Protocol generates 9990 TOKE tokens in revenue and accumulates it in Staking.sol.
2. Owner sets affiliateFee from 0 to 1_000_000 (10,000%) instead of 1000 (10%) by mistake.
3. Owner claims the rewards from the next cycle.
4. Let’s say, 10 TOKE tokens are received as reward.
5. When sending the affiliate’s share, it calculates the share as (10 * 1_000_000 / 10_000) == 1000, which is all the protocol revenue in the contract.

Tools Used

VIM

Recommended Mitigation Steps

Check against a reasonable upper bound (e.g.: 10% of BASIS_POINTS) when setting affiliateFee through the setAffiliateFee() function.