Yieldy contest - sashik_eth's results

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/LiquidityReserve.sol#L92-L98

Vulnerability details

Impact

instantUnstake() allows user to unstake their stakingToken for a fee paid to the liquidity providers. This fee could be changed up to 100% any moment by admin.

Malicious admin could frontrun users instantUnstake() transaction and set fee to any value (using setFee()) and get all users unstaking asset.

It's even could lead to a situation when non-malicious admin accidentally frontrun unstaking user by increasing fee to a new rate, which user wasn't expected.

    /**
        @notice sets Fee (in basis points eg. 100 bps = 1%) for instant unstaking
        @param _fee uint - fee in basis points
     */
    function setFee(uint256 _fee) external onlyOwner {
        // check range before setting fee
        require(_fee <= BASIS_POINTS, "Out of range");
        fee = _fee;

        emit FeeChanged(_fee);
    }

Recommended Mitigation Steps

Consider introducing an upper limit for fees so users can know the maximum fess available in protocol and adding timelock to change fee size. This way, frontrunnig will be impossible, and users will know which fee they agree to.

G01 - Using != 0 instead of > 0 in require statement with uint

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled:

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

G02 - unchecked block can be used for gas efficiency of the expression that can't overflow/underflow

Check comments

src/contracts/Staking.sol:716   epoch.distribute = balance - staked; // Could be unchecked due to check on L713
src/contracts/Yieldy.sol:95 rebasingCreditsPerToken = rebasingCredits / updatedTotalSupply; // Could be unchecked since updatedTotalSupply >= _totalSupply (L89) and _totalSupply > 0 (L83)
src/contracts/Yieldy.sol:192    creditBalances[msg.sender] = creditBalances[msg.sender] - creditAmount; //  Could be unchecked due to check on L190
src/contracts/Yieldy.sol:212    uint256 newValue = _allowances[_from][msg.sender] - _value; //  Could be unchecked due to check on L210
src/contracts/Yieldy.sol:288    creditBalances[_address] = creditBalances[_address] - creditAmount; //  Could be unchecked due to check on L286

G03 - Cache storage variables in memory

For the storage variables that will be accessed multiple times, cache and read from the stack can save gas from each read if variable accessed more than once:

src/contracts/LiquidityReserve.sol:75   IERC20Upgradeable(stakingToken).safeTransferFrom( // stakingToken 2 SLOAD
src/contracts/LiquidityReserve.sol:121  IERC20Upgradeable(stakingToken).safeTransferFrom( // stakingToken 2 SLOAD
src/contracts/Yieldy.sol:83 require(_totalSupply > 0, "Can't rebase if not circulating"); // _totalSupply 2 SLOAD 
src/contracts/Yieldy.sol:129    emit LogSupply(_epoch, block.timestamp, _totalSupply); // _totalSupply 2 SLOAD