Yieldy contest - berndartmueller's results

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L50-L59 https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L33-L44

Vulnerability details

Impact

Removing Yieldy contract addresses from the contracts array with BatchRequests.removeAddress replaces the contract address with a zero-address (due to how delete works).

Each function that loops over the contracts array or accesses an array item by index, should zero-address check the value before calling any external contract functions. If this zero-address check is missing, an external call to this zero-address will revert.

Proof of Concept

BatchRequests.canBatchContractByIndex

function canBatchContractByIndex(uint256 _index)
    external
    view
    returns (address, bool)
{
    return (
        contracts[_index],
        IStaking(contracts[_index]).canBatchTransactions() // @audit-info `contracts` with zero-address elements (due to deletion) will revert - add zero-address check and return false
    );
}

BatchRequests.canBatchContracts

function canBatchContracts() external view returns (Batch[] memory) {
    uint256 contractsLength = contracts.length;
    Batch[] memory batch = new Batch[](contractsLength);
    for (uint256 i; i < contractsLength; ) {
        bool canBatch = IStaking(contracts[i]).canBatchTransactions(); // @audit-info `contracts` with zero-address elements (due to deletion) will revert - add zero-address check
        batch[i] = Batch(contracts[i], canBatch);
        unchecked {
            ++i;
        }
    }
    return batch;
}

Tools Used

Manual review

Recommended mitigation steps

Add zero-address checks to both mentioned functions.

Lines of code

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

Vulnerability details

Impact

Rebasing allows the protocol to "distribute" profit/rewards to Yieldy (and Foxy) token holders by increasing the supply of tokens and increasing the balance of each token holder relative to the token balance (creditBalances).

The order of rebasing and unstaking matters, as the token balance of an account is calculated based on the current balance (technically it's not the token balance, it's credit balances creditBalances).

Unstaking (reducing the account balance) before a rebase will make a user loose out on the distributed (rebased) rewards.

Hence, it's always advised to rebase before any unstaking event or whenever the creditBalances of an account are reduced.

Impact: Migrating funds from the old Foxy contract to the new Yieldy contracts while having an outstanding rebase (profit was not distributed yet) will lead to a loss of potential rewards from the old staking contract.

Proof of Concept

Migration.sol#L54

IStaking(OLD_CONTRACT).instantUnstake(false); // @audit-info the boolean function parameter determines if a rebase should occur. In this case, no rebase will take place

Tools Used

Manual review

Recommended mitigation steps

Use true as a function parameter for instantUnstake to always rebase and distribute potential rewards.

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L14-L27

Vulnerability details

Impact

The function BatchRequests.sendWithdrawalRequests allows calling the sendWithdrawalRequests function on all of the Yieldy contracts at once. However, due to the unbounded for loop, if many Yieldy contracts are added to contracts, this function can potentially DoS due to reaching the block gas limit.

Proof of Concept

BatchRequests.sendWithdrawalRequests

function sendWithdrawalRequests() external {
    uint256 contractsLength = contracts.length;
    for (uint256 i; i < contractsLength; ) {
        if (
            contracts[i] != address(0) &&
            IStaking(contracts[i]).canBatchTransactions()
        ) {
            IStaking(contracts[i]).sendWithdrawalRequests();
        }
        unchecked {
            ++i;
        }
    }
}

Tools Used

Manual review

Recommended mitigation steps

Add offset and limit function parameters to implement a "paginated" for loop.

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Migration.sol#L43-L57

Vulnerability details

Impact

Instant unstaking usually requires paying fees to the LiquidityReserve liquidity providers. Hence, a user migrating from an old Yieldy contract to a new contract (via Migration.moveFundsToUpgradedContract) receives fewer staking tokens after paying fees and therefore userWalletBalance is more than the available staking tokens in the migration contract.

Usually, fees will be set to 0 for the migration period. But if fees are kept, migrating will revert due to insufficient staking tokens received from the old contract.

As this issue is very easy to fix and it also allows to keep migrators to pay fees, this finding is submitted as medium severity.

Proof of Concept

Migration.moveFundsToUpgradedContract

function moveFundsToUpgradedContract() external {
    uint256 userWalletBalance = IYieldy(OLD_YIELDY_TOKEN).balanceOf(
        msg.sender
    );

    IYieldy(OLD_YIELDY_TOKEN).transferFrom(
        msg.sender,
        address(this),
        userWalletBalance
    );

    IStaking(OLD_CONTRACT).instantUnstake(false); // @audit-info receives less staking tokens than `userWalletBalance` due to fees being payed

    IStaking(NEW_CONTRACT).stake(userWalletBalance, msg.sender); // @audit-info reverts due to insufficient staking tokens
}

Tools Used

Manual review

Recommended mitigation steps

Use the actual staking token balance IERC20Upgradeable(stakingToken).balanceOf(address(this)) right after unstaking from the old contract:

function moveFundsToUpgradedContract() external {
    uint256 userWalletBalance = IYieldy(OLD_YIELDY_TOKEN).balanceOf(
        msg.sender
    );

    IYieldy(OLD_YIELDY_TOKEN).transferFrom(
        msg.sender,
        address(this),
        userWalletBalance
    );

    IStaking(OLD_CONTRACT).instantUnstake(false);

    IStaking(NEW_CONTRACT).stake(IERC20Upgradeable(stakingToken).balanceOf(address(this)), msg.sender);
}