Yieldy contest - WatchPug's results

Lines of code

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

Vulnerability details

if (warmUpPeriod == 0) {
    IYieldy(YIELDY_TOKEN).mint(_recipient, _amount);
} else {
    // create a claim and mint tokens so a user can claim them once warm up has passed
    warmUpInfo[_recipient] = Claim({
        amount: info.amount + _amount,
        credits: info.credits +
            IYieldy(YIELDY_TOKEN).creditsForTokenBalance(_amount),
        expiry: epoch.number + warmUpPeriod
    });

    IYieldy(YIELDY_TOKEN).mint(address(this), _amount);
}

Staking.sol#stake() is a public function and you can specify an arbitrary address as the _recipient.

When warmUpPeriod > 0, with as little as 1 wei of YIELDY_TOKEN, the _recipient's warmUpInfo will be push back til epoch.number + warmUpPeriod.

Recommendation

Consider changing to not allow deposit to another address when warmUpPeriod > 0.

Lines of code

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

Vulnerability details

Every time the rebase() gets called, there will be a surge of price per share for the existing stakeholders.

This enables a well-known attack vector, in which the attacker will deposit a huge amount of underlying tokens and take a large portion of the vault, then trigger the surge, and exit right after.

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

function rebase() public {
    // we know about the issues surrounding block.timestamp, using it here will not cause any problems
    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++;

        uint256 balance = contractBalance();
        uint256 staked = IYieldy(YIELDY_TOKEN).totalSupply();

        if (balance <= staked) {
            epoch.distribute = 0;
        } else {
            epoch.distribute = balance - staked;
        }
    }
}

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

function rebase(uint256 _profit, uint256 _epoch)
    external
    onlyRole(REBASE_ROLE)
{
    uint256 currentTotalSupply = _totalSupply;
    require(_totalSupply > 0, "Can't rebase if not circulating");

    if (_profit == 0) {
        emit LogSupply(_epoch, block.timestamp, currentTotalSupply);
        emit LogRebase(_epoch, 0, getIndex());
    } else {
        uint256 updatedTotalSupply = currentTotalSupply + _profit;

        if (updatedTotalSupply > MAX_SUPPLY) {
            updatedTotalSupply = MAX_SUPPLY;
        }

        rebasingCreditsPerToken = rebasingCredits / updatedTotalSupply;
        require(rebasingCreditsPerToken > 0, "Invalid change in supply");

        _totalSupply = updatedTotalSupply;

        _storeRebase(updatedTotalSupply, _profit, _epoch);
    }
}

PoC

Given:

• Current YIELDY_TOKEN.totalSupply() is 100,000 STAKING_TOKEN;
• Pending distribution epoch.distribute amount is 1,000 STAKING_TOKEN;

1. 1 block before epoch.endTime, the attacker deposit 100,000 STAKING_TOKEN, taking 50% of the pool;
2. 1 block later, the attacker called rebase(), which distributed the 1,000 STAKING_TOKEN to YIELDY_TOKEN and caused a surge of price per share;
3. The attacker withdraws all the shares and received 100,500 STAKING_TOKEN.

As a result, the attacker has stolen half of the pending rewards which belongs to the old users.

Recommendation

Consider switching to a rewardRate-based gradual release model, such as Synthetix's StakingRewards contract.

See: https://github.com/Synthetixio/synthetix/blob/develop/contracts/StakingRewards.sol#L113-L132

Lines of code

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

Vulnerability details

function instantUnstakeReserve(uint256 _amount) external {
    require(_amount > 0, "Invalid amount");
    // prevent unstaking if override due to vulnerabilities
    require(
        !isUnstakingPaused && !isInstantUnstakingPaused,
        "Unstaking is paused"
    );

    rebase();
    _retrieveBalanceFromUser(_amount, msg.sender);

    uint256 reserveBalance = IERC20Upgradeable(STAKING_TOKEN).balanceOf(
        LIQUIDITY_RESERVE
    );

    require(reserveBalance >= _amount, "Not enough funds in reserve");

    ILiquidityReserve(LIQUIDITY_RESERVE).instantUnstake(
        _amount,
        msg.sender
    );
}

In the current implementation of Staking.sol#instantUnstakeReserve(), balanceOf staking token in the LiquidityReserve contract is used as the reserveBalance, which is used to determine whether the amount can instant unstake.

However, there can be some funds actually already available but not yet claimed, which can and should be used for instant unstake.

IStaking.claimWithdraw() will only be called in LiquidityReserve#removeLiquidity() and LiquidityReserve#instantUnstake().

When the balance is low in the LiquidityReserve, and liquidity providers are inactive, there is good chance that IStaking.claimWithdraw() wont be called for a long period of time. In which case, instantUnstakeReserve() can be malfunction.

Recommendation

Consider adding an internal view function getLiquidityReserveAvailableTokenBalance(), and using it to determine whether enough funds in the reserve:

function getLiquidityReserveAvailableTokenBalance() internal view returns (uint256) {
    uint256 stakingTokenBalance = IERC20Upgradeable(STAKING_TOKEN).balanceOf(
        LIQUIDITY_RESERVE
    );
    if (_isClaimWithdrawAvailable(LIQUIDITY_RESERVE)) {
        Claim memory info = coolDownInfo[LIQUIDITY_RESERVE];
        return stakingTokenBalance + info.amount;
    } else {
        return stakingTokenBalance;
    }
}