Yieldy contest - unforgiven's results

Lines of code

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

Vulnerability details

Impact

Function rebase() in contract Staking calls Yieldy.rebase(profit, ) and Yieldy.rebase(profit, ) would revert if rebasingCredits / updatedTotalSupply was equal to 0. it's possible to transfer some STAKING_TOKEN directly to Stacking contract before or after deployment of Staking and make rebasingCredits / updatedTotalSupply equal to 0, and then most of the functionalities of Staking would not work because they call rebase() which will revert. it's possible to perform this DOS for any token by transferring some tokens STAKING_TOKEN to contract address and then staking 1 wei in contract. or for tokens with low precisions and low price it's even possible to perform when totalSupply() of Yieldy is low.

also attacker can only make rebasingCredits / updatedTotalSupply too low and so rounding error would be significant when rebasingCredits / updatedTotalSupply gets too low and users funds would be lost because of rounding error.

Proof of Concept

This is rebase() code in Yieldy:

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

As you can see if rebasingCredits / updatedTotalSupply == 0 then the code will revert. updatedTotalSupply is equal to _totalSupply + _profit and rebasingCredits is wad in the first. Yieldy.rebase(profit,) is called by Staking.rebase():

    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;
            }
        }
    }

As you can see the value of _profit is set to epoch.distribute which is contractBalance() - IYieldy(YIELDY_TOKEN).totalSupply() and contractBalance() is sum of STAKING_TOKEN and TOKE balance of Staking contract. so if attacker transfers X amount of STAKCING_TOKEN directly to Staking contract then the value of _profit which is going to send to Yieldy.rebase(profit,) would be higher than X. to exploit this attacker call stake(1 wei) after Staking deployment and then transfer STAKING_TOKEN directly to contract. then the value of rebasingCredits in Yieldy would be 2 wad and the value of _profit sent to Yieldy.rebase(profit,) would be bigger than 2 wad and rebasingCredits / updatedTotalSupply would be 0 and from now on all calls to Staking.rebase() would revert and that means functions Stake() and instantUnstakeReserve() and instantUnstakeCurve() wouldnt work anymore.

it's possible to perform this attack for low precision tokens with low price STAKING_TOKEN too. the only thing attacker needs to do is that in early stage of Staking deployment sends more than rebasingCredits of STAKING_TOKEN token directly to Staking contract address. then in rebase() contract send that amount as profit to Yieldy.rebase() and that call would revert which will cause most of the logics of Staking to revert and not work.

and when rebasingCredits / updatedTotalSupply is low, the rounding error would be high enough that the compounding yield won't show itself. attacker can make rebasingCredits / updatedTotalSupply too low but not 0 and from then user's funds would be lost because of rounding error (wrong number of Yieldy token would be mint for user).

Tools Used

VIM

Recommended Mitigation Steps

the default initial value of rebasingCredits should be very high that attacker couldn't perform this attack.

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L419-L445 https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L120-L126

Vulnerability details

Impact

if the token is deflationary then contract will receive less token that requested amount but contract don't check for the real transferred amount. because this is happening in receiving stacking_token in addLiquidity() of LiquidityReserve and stake() of Staking then those logics for minting YIELDY_TOKEN or LP token is wrong. (contract receive less than amount but mint or transfer amount to user). This can cause other users which staked to lose funds.

Proof of Concept

This is the related code where transfer happens (stake() and addLiquidity()):

    function stake(uint256 _amount, address _recipient) public {
        // if override staking, then don't allow stake
        require(!isStakingPaused, "Staking is paused");
        // amount must be non zero
        require(_amount > 0, "Must have valid amount");

        uint256 yieldyTotalSupply = IYieldy(YIELDY_TOKEN).totalSupply();

        // Don't rebase unless tokens are already staked or could get locked out of staking
        if (yieldyTotalSupply > 0) {
            rebase();
        }

        IERC20Upgradeable(STAKING_TOKEN).safeTransferFrom(
            msg.sender,
            address(this),
            _amount
        );

        Claim storage info = warmUpInfo[_recipient];

        // if claim is available then auto claim tokens
        if (_isClaimAvailable(_recipient)) {
            claim(_recipient);
        }

        _depositToTokemak(_amount);

        // skip adding to warmup contract if period is 0
        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);
        }

    function addLiquidity(uint256 _amount) external {
        require(isReserveEnabled, "Not enabled yet");
        uint256 stakingTokenBalance = IERC20Upgradeable(stakingToken).balanceOf(
            address(this)
        );
        uint256 rewardTokenBalance = IERC20Upgradeable(rewardToken).balanceOf(
            address(this)
        );
        uint256 lrFoxSupply = totalSupply();
        uint256 coolDownAmount = IStaking(stakingContract)
            .coolDownInfo(address(this))
            .amount;
        uint256 totalLockedValue = stakingTokenBalance +
            rewardTokenBalance +
            coolDownAmount;

        uint256 amountToMint = (_amount * lrFoxSupply) / totalLockedValue;
        IERC20Upgradeable(stakingToken).safeTransferFrom(
            msg.sender,
            address(this),
            _amount
        );
        _mint(msg.sender, amountToMint);
    }

As you can see contract transfers amount of STAKE_TOKEN and assumes it is going to receive that amount and then mint the same amount of YIELDY_TOKEN or LP token. So user receive more funds which belongs to other users. protocol logics are not suitable for deflationary tokens and funds would be lost.

Tools Used

VIM

Recommended Mitigation Steps

check the real amount of tokens that contract receives.

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L100-L127

Vulnerability details

Impact

Function addLiquidity() suppose to do add Liquidity for the staking Token and receive lrToken in exchange. to calculate amount of IrToken codes uses this calculation: amountToMint = (_amount * lrFoxSupply) / totalLockedValue but it's possible for attacker to manipulate totalLockedValue (by sending tokens directly to LiquidityReserve address) and make totalLockedValue/lrFoxSupply very high in early stage of contract deployment so because of rounding error in calculation of amountToMint the users would receive very lower IrToken and users funds would be lost and attacker can steal them. Attacker can perform this attack by sending tokens before even LiquidityReserve deployed because the contract address would be predictable and attacker can perform front-run or sandwich attack too.

also it's possible to perform this attack for STAKING_TOKEN with low precision and low price even if LiquidityReserve had some balances.

Proof of Concept

This is addLiquidity() code in LiquidityReserve:

    function addLiquidity(uint256 _amount) external {
        require(isReserveEnabled, "Not enabled yet");
        uint256 stakingTokenBalance = IERC20Upgradeable(stakingToken).balanceOf(
            address(this)
        );
        uint256 rewardTokenBalance = IERC20Upgradeable(rewardToken).balanceOf(
            address(this)
        );
        uint256 lrFoxSupply = totalSupply();
        uint256 coolDownAmount = IStaking(stakingContract)
            .coolDownInfo(address(this))
            .amount;
        uint256 totalLockedValue = stakingTokenBalance +
            rewardTokenBalance +
            coolDownAmount;

        uint256 amountToMint = (_amount * lrFoxSupply) / totalLockedValue;
        IERC20Upgradeable(stakingToken).safeTransferFrom(
            msg.sender,
            address(this),
            _amount
        );
        _mint(msg.sender, amountToMint);
    }

As you can see code uses this calculation: amountToMint = (_amount * lrFoxSupply) / totalLockedValue; to find the amount of IrToken that is going to mint for user. but attacker can send stakingToken or rewardToken directly to LiquidityReserve address when the there is no liqudity in the contract and make totalLockedValue very high. then attacker call addLiquidity() and mint some IrToken for himself and from then anyone tries to call addLiquidity() because of rounding error is going to lose some funds (receives less IrToken than he is supposed to)

Tools Used

VIM

Recommended Mitigation Steps

add more precision when calculating IrToken so this attack wouldn't be feasible to perform.

Lines of code

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

Vulnerability details

Impact

users funds are in Staking contract address and owner shouldn't be able to instantly withdraw users funds. if owner make mistake or the private key (or DAO) get hacked then user funds can be lost immediately.

Proof of Concept

This is transferToke() code in Staking:

    /**
        @notice transfer TOKE from staking contract to address
        @dev used so DAO can get TOKE and manually trade to return FOX to the staking contract
        @param _claimAddress address to send TOKE rewards
     */
    function transferToke(address _claimAddress) external onlyOwner {
        // _claimAddress can't be 0x0
        require(_claimAddress != address(0), "Invalid address");
        uint256 totalTokeAmount = IERC20Upgradeable(TOKE_TOKEN).balanceOf(
            address(this)
        );
        IERC20Upgradeable(TOKE_TOKEN).safeTransfer(
            _claimAddress,
            totalTokeAmount
        );
    }

As you can see there is no time-lock or other mechanism to prevent owner from withdrawing contract TOKE balance instantly. those tokens belongs to users and this can create risk for users funds and user funds would be lost by simple mistakes or attacks.

Tools Used

VIM

Recommended Mitigation Steps

add some mechanism to prevent instant withdrawal of funds.