Yieldy contest - Sm4rty's results

LOW / NON-CRITICAL

1. Use SAFEERC20.SAFEAPPROVE INSTEAD OF APPROVE

This is probably an oversight since SafeERC20 was imported and safeTransfer() was used for ERC20 token transfers. Nevertheless, note that approve() will fail for certain token implementations that do not return a boolean value (). Hence it is recommend to use safeApprove().

Instances:

LiquidityReserve.sol:81 Migration.sol:31 Migration.sol:32 Staking.sol:79 Staking.sol:83 Staking.sol:84 Staking.sol:88 Staking.sol:92

src/contracts/LiquidityReserve.sol:81:        IERC20Upgradeable(rewardToken).approve(
src/contracts/Migration.sol:31:        IYieldy(OLD_YIELDY_TOKEN).approve(_oldContract, type(uint256).max);
src/contracts/Migration.sol:32:        IERC20Upgradeable(stakingToken).approve(
src/contracts/Staking.sol:79:            IERC20(TOKE_POOL).approve(CURVE_POOL, type(uint256).max);
src/contracts/Staking.sol:83:        IERC20(STAKING_TOKEN).approve(TOKE_POOL, type(uint256).max);
src/contracts/Staking.sol:84:        IERC20Upgradeable(YIELDY_TOKEN).approve(
src/contracts/Staking.sol:88:        IERC20Upgradeable(YIELDY_TOKEN).approve(
src/contracts/Staking.sol:92:        IERC20Upgradeable(TOKE_TOKEN).approve(COW_RELAYER, type(uint256).max);

Recommended Mitigation Steps

Update to _token.safeApprove(spender, type(uint256).max) in the function.

2. Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

Instances:

Migration.sol:48 Staking.sol:471

src/contracts/Staking.sol:471:                IYieldy(YIELDY_TOKEN).transfer(
src/contracts/Migration.sol:48:        IYieldy(OLD_YIELDY_TOKEN).transferFrom(

Reference:

This similar medium-severity finding from Consensys Diligence Audit of Fei Protocol.

Recommended Mitigation Steps:

Consider using safeTransfer/safeTransferFrom or require() consistently.

3. _safeMint() should be used rather than _mint() wherever possible

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both open OpenZeppelin and solmate have versions of this function so that NFTs aren’t lost if they’re minted to contracts that cannot transfer them back out.

Instances

LiquidityReserve.sol:80 LiquidityReserve.sol:126 Yieldy.sol:240

src/contracts/LiquidityReserve.sol:80:        _mint(address(this), MINIMUM_LIQUIDITY);
src/contracts/LiquidityReserve.sol:126:        _mint(msg.sender, amountToMint);
src/contracts/Yieldy.sol:240:        _mint(_address, _amount);

Recommendations:

Use _safeMint() instead of _mint().

Gas Optimizations Issues:

1. Preincrement costs less gas as compared to Postincrement :

++i costs less gas as compared to i++ for unsigned integer, as per-increment is cheaper(its about 5 gas per iteration cheaper)

i++ increments i and returns initial value of i. Which means

uint i =  1;
i++; // ==1 but i ==2

But ++i returns the actual incremented value:

uint i = 1;
++i; // ==2 and i ==2 , no need for temporary variable here

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

Instances:

Staking.sol:708

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

Reference:

https://www.reddit.com/r/ethdev/comments/tcwspw/i_vs_i_gas_efficiency/

Remediation:

Use Preincrement(++i) instead of Postincrement(i++) in code.

2. !=0 instead of >0 for UINT

0 is less efficient than != 0 for unsigned integers (with proof) != 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 at 10k 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

Instances

src/contracts/Yieldy.sol:83 src/contracts/Yieldy.sol:96 src/contracts/Staking.sol:118 src/contracts/Staking.sol:410 src/contracts/Staking.sol:572 src/contracts/Staking.sol:604

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");
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");

Reference:

https://twitter.com/gzeon/status/1485428085885640706

Remediation:

I suggest changing > 0 with != 0. Also, please enable the Optimizer.

3. Variables: No need to explicitly initialize variables with default values

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.

We can use uint number; instead of uint number = 0;### Instance Includes: src/contracts/Staking.sol:636 src/contracts/Staking.sol:637

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

Recommendation:

I suggest removing explicit initializations for default values.

4. Inline a modifier that is only used once

As onlyStakingContract() s only used once in Staking.sol contract (in function function instantUnstake(uint256 _amount, address _recipient) it should get inlined to save gas:

Instances:

modifier link

    modifier onlyStakingContract() {
        require(msg.sender == stakingContract, "Not staking contract");
        _;
    }

Function Link

function instantUnstake(uint256 _amount, address _recipient)
        external
        onlyStakingContract

5. Custom Errors instead of Revert Strings to save Gas

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

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).

Instances Includes:

LiquidityReserve.sol: #L25 #61 #L62 #L94 #L105 #L163 #L192 #L215

Yieldy.sol: #58 #59 #83 #96 #187 #190 #210 #249 #257 #279 #286

Staking.sol: #118 #143 #408 #410 #572 #586 #604 #644 #676

src/contracts/LiquidityReserve.sol:25:        require(msg.sender == stakingContract, "Not staking contract");
src/contracts/LiquidityReserve.sol:61:        require(!isReserveEnabled, "Already enabled");
src/contracts/LiquidityReserve.sol:62:        require(_stakingContract != address(0), "Invalid address");
src/contracts/LiquidityReserve.sol:94:        require(_fee <= BASIS_POINTS, "Out of range");
src/contracts/LiquidityReserve.sol:105:        require(isReserveEnabled, "Not enabled yet");
src/contracts/LiquidityReserve.sol:163:        require(_amount <= balanceOf(msg.sender), "Not enough lr tokens");
src/contracts/LiquidityReserve.sol:192:        require(isReserveEnabled, "Not enabled yet");
src/contracts/LiquidityReserve.sol:215:        require(isReserveEnabled, "Not enabled yet");
src/contracts/Yieldy.sol:58:        require(stakingContract == address(0), "Already Initialized");
src/contracts/Yieldy.sol:59:        require(_stakingContract != address(0), "Invalid address");
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");
src/contracts/Yieldy.sol:187:        require(_to != address(0), "Invalid address");
src/contracts/Yieldy.sol:190:        require(creditAmount <= creditBalances[msg.sender], "Not enough funds");
src/contracts/Yieldy.sol:210:        require(_allowances[_from][msg.sender] >= _value, "Allowance too low");
src/contracts/Yieldy.sol:249:        require(_address != address(0), "Mint to the zero address");
src/contracts/Yieldy.sol:257:        require(_totalSupply < MAX_SUPPLY, "Max supply");
src/contracts/Yieldy.sol:279:        require(_address != address(0), "Burn from the zero address");
src/contracts/Yieldy.sol:286:        require(currentCredits >= creditAmount, "Not enough balance");
src/contracts/Staking.sol:118:        require(_recipient.amount > 0, "Must enter valid amount");
src/contracts/Staking.sol:143:        require(_claimAddress != address(0), "Invalid address");
src/contracts/Staking.sol:408:        require(!isStakingPaused, "Staking is paused");
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:586:        require(reserveBalance >= _amount, "Not enough funds in reserve");
src/contracts/Staking.sol:604:        require(_amount > 0, "Invalid amount");
src/contracts/Staking.sol:644:            require(from == 1 || to == 1, "Invalid Curve Pool");
src/contracts/Staking.sol:676:        require(!isUnstakingPaused, "Unstaking is paused");

References:

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

Remediation:

I suggest replacing revert strings with custom errors.

<hr />