Yieldy contest - StErMi's results

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/BatchRequests.sol#L14-L27 https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/BatchRequests.sol#L33-L44 https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/BatchRequests.sol#L89-L99

Vulnerability details

Impact

All the functions inside BatchRequests that loops on contracts array and all the contracts that use those functions could revert because of out of gas exception.

Inside BatchRequests addresses are added to the contracts array via addAddress and removed from it via removeAddress

The removeAddress function internally execute delete contracts[i]; but this instruction is not removing an item from the array, but just "resetting" the value in that position to the default value based on the type.

This mean that when you call delete contracts[0] the result is not removing the item from the array but resetting the value of contracts[0] to address(0).

sendWithdrawalRequests and canBatchContracts are executing a loop on the unbound array contracts and this could result in a failed transaction because of out of gas revert. The problem is particular important because there is no way currently to reduce the array or to execute those transactions in smaller batches.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Manual review + hardhat/foundry

Hardhat

Note: increase mocha timeout or set it to infinite

import {
  ethers,
  deployments,
  getNamedAccounts,
  network,
  upgrades,
} from 'hardhat';
import { expect } from 'chai';
import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers';
import { BigNumber, Contract, Signer } from 'ethers';
import ERC20 from '@openzeppelin/contracts/build/contracts/ERC20.json';
import { Staking, BatchRequests, Yieldy } from '../typechain-types';
import * as constants from './constants';
import { tokePoolAbi } from '../src/abis/tokePoolAbi';

describe('BatchRequests', function () {
  let accounts: SignerWithAddress[];
  let batchRequests: BatchRequests;
  let stakingToken: Contract;
  let rewardToken: Contract;
  let rewardToken2: Contract;
  let rewardToken3: Contract;
  let staking: Staking;
  let staking2: Staking;
  let staking3: Staking;
  let tokePool: Contract;

  // skips EVM time equal to epoch duration
  async function mineToNextEpoch() {
    const epochLength = (await staking.epoch()).duration.toNumber();
    await network.provider.send('evm_increaseTime', [epochLength + 10]);
    await network.provider.send('hardhat_mine');
  }

  beforeEach(async () => {
    const { admin } = await getNamedAccounts();

    await network.provider.request({
      method: 'hardhat_reset',
      params: [
        {
          forking: {
            jsonRpcUrl: process.env.MAINNET_URL,
            blockNumber: constants.BLOCK_NUMBER,
          },
        },
      ],
    });

    await deployments.fixture();
    accounts = await ethers.getSigners();

    const currentBlock = await ethers.provider.getBlockNumber();
    const currentTime = (await ethers.provider.getBlock(currentBlock))
      .timestamp;
    const firstEpochEndTime = currentTime + constants.EPOCH_DURATION;

    stakingToken = new ethers.Contract(
      constants.STAKING_TOKEN,
      ERC20.abi,
      accounts[0]
    );

    const rewardTokenDeployment = await ethers.getContractFactory('Yieldy');
    rewardToken = (await upgrades.deployProxy(rewardTokenDeployment, [
      'USDC Yieldy',
      'USDCy',
      18,
    ])) as Yieldy;
    await rewardToken.deployed();

    rewardToken2 = (await upgrades.deployProxy(rewardTokenDeployment, [
      'Fox Yieldy',
      'FOXy',
      18,
    ])) as Yieldy;
    await rewardToken2.deployed();

    rewardToken3 = (await upgrades.deployProxy(rewardTokenDeployment, [
      'ALX Yieldy',
      'ALXy',
      18,
    ])) as Yieldy;
    await rewardToken3.deployed();

    tokePool = new ethers.Contract(
      constants.TOKE_ADDRESS,
      tokePoolAbi,
      accounts[0]
    );

    const stakingDeployment = await ethers.getContractFactory('Staking');
    staking = (await upgrades.deployProxy(stakingDeployment, [
      stakingToken.address,
      rewardToken.address,
      constants.TOKE_TOKEN,
      constants.TOKE_ADDRESS,
      constants.TOKE_MANAGER,
      constants.TOKE_REWARD,
      stakingToken.address,
      ethers.constants.AddressZero,
      ethers.constants.AddressZero,
      constants.EPOCH_DURATION,
      firstEpochEndTime,
    ])) as Staking;

    staking2 = (await upgrades.deployProxy(stakingDeployment, [
      stakingToken.address,
      rewardToken2.address,
      constants.TOKE_TOKEN,
      constants.TOKE_ADDRESS,
      constants.TOKE_MANAGER,
      constants.TOKE_REWARD,
      stakingToken.address,
      ethers.constants.AddressZero,
      ethers.constants.AddressZero,
      constants.EPOCH_DURATION,
      firstEpochEndTime,
    ])) as Staking;

    staking3 = (await upgrades.deployProxy(stakingDeployment, [
      stakingToken.address,
      rewardToken3.address,
      constants.TOKE_TOKEN,
      constants.TOKE_ADDRESS,
      constants.TOKE_MANAGER,
      constants.TOKE_REWARD,
      stakingToken.address,
      ethers.constants.AddressZero,
      ethers.constants.AddressZero,
      constants.EPOCH_DURATION,
      firstEpochEndTime,
    ])) as Staking;

    const batchDeployment = await ethers.getContractFactory('BatchRequests');
    batchRequests = await batchDeployment.deploy();

    await network.provider.request({
      method: 'hardhat_impersonateAccount',
      params: [constants.STAKING_TOKEN_WHALE],
    });

    // Transfer to admin account for constants.STAKING_TOKEN to be easily transferred to other accounts
    const transferAmount = BigNumber.from('9000000000000000');
    const whaleSigner = await ethers.getSigner(constants.STAKING_TOKEN_WHALE);
    const stakingTokenWhale = stakingToken.connect(whaleSigner);
    await stakingTokenWhale.transfer(admin, transferAmount);
    const stakingTokenBalance = await stakingToken.balanceOf(admin);
    expect(BigNumber.from(stakingTokenBalance).toNumber()).gte(
      transferAmount.toNumber()
    );
    await rewardToken.initializeStakingContract(staking.address); // initialize reward contract
    await rewardToken2.initializeStakingContract(staking2.address);
    await rewardToken3.initializeStakingContract(staking3.address);
  });

  it('Should call sendWithdrawalRequests on multiple contracts', async () => {
    await batchRequests.addAddress(staking.address);
    const addressCountToAdd = 1000000;

    console.log(
      `Adding ${addressCountToAdd} addresses to batchRequests contract`
    );
    for (let i = 0; i < addressCountToAdd; i++) {
      await batchRequests.addAddress(staking.address);
    }

    console.log(`Executing canBatchContracts`);
    await batchRequests.canBatchContracts();
  });
});

Recommended Mitigation Steps

Implement removeAddress in a way to remove the element from the batch to prevent sendWithdrawalRequests and canBatchContracts to possible revert because of out of gas exception.

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/Yieldy.sol#L205-L222 https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/Yieldy.sol#L278-L293

Vulnerability details

Impact

Premise: the action of burning token happens when an amount of tokens is transferred to the address(0). As a consequence:

• those amount of token are burned and cannot be recovered
• the balance of address(0) should not be updated
• the balance of account should be updated, removing the amount burned
• the totalSupply of the token should be updated, removing the amount burned

The current implementation of transferFrom is an external function that can be called by anyone. The function is only checking that the spender (msg.sender)has enough allowance from the _from account to transfer _value of tokens. Because there is no check on the input parameters _from and _to the function is allowing the msg.sender to transfer _value amount of tokens to the address(0).

This behavior (see premise) is in practice allowing the msg.sender to burn _from _value amount of tokens.

This introduces two problems:

1. The burn implementation has the onlyRole(MINTER_BURNER_ROLE) modifier. This mean that only a specific role is allowed to burn or mint tokens. Because transferFrom is callable by anyone, it means that it allow anyone to execute the same actions that should be executable only by a specific role. This is an authorization problem because it is allowing "normal" users to burn tokens.
2. The burn implementation will burn user tokens. When tokens are burned, the user creditBalances is updated (removing credits) but also rebasingCredits and _totalSupply global state variables are updated. transferFrom functions is only updating the _from and _to user's creditBalances because it is only transferring tokens from an account to another. This mean that if someone will use transferFrom to burn tokens, those rebasingCredits and _totalSupply global state variables will not be updated even if those tokens have been burned. Every external contract that rely on those global variables value will not act correctly.

Note: even if they are two separate issues (authorization problem and behavior problem) I have created only one finding because they are strictly correlated and about the same function.

Proof of Concept

import { ethers, getNamedAccounts, upgrades } from 'hardhat';
import { expect } from 'chai';
import { Yieldy } from '../typechain-types/Yieldy';
import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers';
import { BigNumber, Contract, ContractFactory, Signer } from 'ethers';
import { MockProvider } from 'ethereum-waffle';
import {
  keccak256,
  toUtf8Bytes,
  defaultAbiCoder,
  solidityPack,
  hexlify,
} from 'ethers/lib/utils';
import { ecsign } from 'ethereumjs-util';

const PERMIT_TYPEHASH = keccak256(
  toUtf8Bytes(
    'Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)'
  )
);

function getDomainSeparator(name: string, tokenAddress: string) {
  return keccak256(
    defaultAbiCoder.encode(
      ['bytes32', 'bytes32', 'bytes32', 'uint256', 'address'],
      [
        keccak256(
          toUtf8Bytes(
            'EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)'
          )
        ),
        keccak256(toUtf8Bytes(name)),
        keccak256(toUtf8Bytes('1')),
        1,
        tokenAddress,
      ]
    )
  );
}

export async function getApprovalDigest(
  token: Contract,
  approve: {
    owner: string;
    spender: string;
    value: BigNumber;
  },
  nonce: BigNumber,
  deadline: BigNumber
): Promise<string> {
  const name = await token.name();
  const DOMAIN_SEPARATOR = getDomainSeparator(name, token.address);
  return keccak256(
    solidityPack(
      ['bytes1', 'bytes1', 'bytes32', 'bytes32'],
      [
        '0x19',
        '0x01',
        DOMAIN_SEPARATOR,
        keccak256(
          defaultAbiCoder.encode(
            ['bytes32', 'address', 'address', 'uint256', 'uint256', 'uint256'],
            [
              PERMIT_TYPEHASH,
              approve.owner,
              approve.spender,
              approve.value,
              nonce,
              deadline,
            ]
          )
        ),
      ]
    )
  );
}

describe('Yieldy', function () {
  let accounts: SignerWithAddress[];
  let Yieldy: ContractFactory;
  let yieldy: Yieldy;

  beforeEach(async () => {
    // initialize Yieldy using a contract we control fully in place of the staking
    // contract allows for more localize testing
    accounts = await ethers.getSigners();
    const { stakingContractMock } = await getNamedAccounts();
    Yieldy = await ethers.getContractFactory('Yieldy');
    yieldy = (await upgrades.deployProxy(Yieldy, [
      'Fox Yieldy',
      'FOXy',
      18,
    ])) as Yieldy;
    await yieldy.deployed();

    await yieldy.initializeStakingContract(stakingContractMock);
  });

  describe('accounts can "burn" tokens via transferFrom', () => {
    it('can only be called by accounts with MINTER_BURNER_ROLE', async () => {
      const rebasingCreditsPerToken = await yieldy.rebasingCreditsPerToken();

      // Grant account0 the minter/burner role
      const minterRole = await yieldy.MINTER_BURNER_ROLE();
      yieldy.grantRole(minterRole, accounts[0].address);

      // mint to account1 100 tokens
      const mintAmount = ethers.utils.parseUnits('100', 18);
      yieldy.mint(accounts[1].address, mintAmount);

      let balance = await yieldy.balanceOf(accounts[1].address);
      let creditBalance = await yieldy.creditBalances(accounts[1].address);
      let totalSupply = await yieldy.totalSupply();
      let rebasingCredits = await yieldy.rebasingCredits();

      // Check account1 balance
      expect(balance).to.be.eq(mintAmount);

      // Check account1 creditBalances
      expect(creditBalance).to.be.eq(mintAmount.mul(rebasingCreditsPerToken));

      // Check totalSupply
      expect(totalSupply).to.be.eq(mintAmount);

      // Check rebasingCredits
      expect(rebasingCredits).to.be.eq(mintAmount.mul(rebasingCreditsPerToken));

      // account 1 is not able to call BURN
      const burnTx = yieldy
        .connect(accounts[1] as Signer)
        .burn(accounts[1].address, mintAmount);

      await expect(burnTx).to.be.revertedWith(
        `AccessControl: account ${accounts[1].address.toLowerCase()} is missing role ${minterRole}`
      );

      // PROVE THAT ACCOUNT1 can "burn" tokens via transferFrom

      // approve itself to transfer mintAmount via transferFrom
      await yieldy
        .connect(accounts[1] as Signer)
        .approve(accounts[1].address, mintAmount);

      // account 1 is able to "burn" tokens via transferFrom(account1, address(0), amount)
      // usually (see OpenZeppelin transferFrom implementation) this operation is prevented when `_to` is zeroaddress
      // because is like burning tokens
      await yieldy
        .connect(accounts[1] as Signer)
        .transferFrom(
          accounts[1].address,
          ethers.constants.AddressZero,
          mintAmount
        );

      // After transferFrom to 0 address the account1 will not have any more token
      // BUT the totalSupply remain the same
      // and also the rebasingCredits (total) remain the same
      // The problem is that those token have been "burned" because sent to address(0) and cannot
      // be retrieved anymore
      balance = await yieldy.balanceOf(accounts[1].address);
      creditBalance = await yieldy.creditBalances(accounts[1].address);
      totalSupply = await yieldy.totalSupply();
      rebasingCredits = await yieldy.rebasingCredits();

      // check balance, this is correct to be 0
      expect(balance).to.be.eq(0);

      // Check account1 creditBalances, this is correct to be 0
      expect(creditBalance).to.be.eq(0);

      // Check totalSupply, this should be 0 because we have "burned" all the tokens
      // instead it's equal to the amount of `mintAmount` (it has not changed)
      expect(totalSupply).to.be.eq(mintAmount);

      // Check rebasingCredits, this should be 0 because we have "burned" all the tokens
      // instead it's equal to the amount of `mintAmount * rebasingCreditsPerToken` (it has not changed)
      expect(rebasingCredits).to.be.eq(mintAmount.mul(rebasingCreditsPerToken));
    });
  });
});

Tools Used

Manual + hardhat test

Recommended Mitigation Steps

Revert transferFrom if _to == address(0), the protocol is allowing the burn of tokens only via the burn function that can be called only by an account that has the role MINTER_BURNER_ROLE

Natspec documentation partially added or missing at all

Natspec documentation are useful for internal developers that need to work on the project, external developers that need to integrate with the project, auditors that have to review it but also for end users given that Etherscan has officially integrated the support for it directly on their site.

These contracts are missing natspec for function/variables

• LiquidityReserveStorage miss natspec for all variable name, only "normal" comments are present
• YieldyStorage miss natspec for all variable name, only "normal" comments are present for some variables
• StakingStorage miss natspec for all variable name, only "normal" comments are present for some variables
• BatchRequests miss natspec for variable contracts
• Migration miss natspec for OLD_CONTRACT, NEW_CONTRACT, OLD_YIELDY_TOKEN variables and constructor function
• Batch miss natspec for struct variables
• Claim miss natspec for struct variables
• Epoch miss natspec for struct variables
• Rebase miss natspec for struct variables

Consider adding the missing natspec documentation.

Yieldy transferFrom implementation decrease allowance even if allowance is type(uint256).max

The OpenZeppelin current implementation will not decrease the allowance of the spender if the allowance is equal to type(uint256).max If this is the intended behavior, it would be better to document it, given that it behave differently from what a widely accepted implementation (open zeppelin) do.

Consider aligning the behavior of Yieldy to the OpenZeppelin implementation if possible.

Yieldy _mint allow to mint 0 tokens

While it's true that the Staking.stake will revert when the _amount is zero, it would be better to also check to prevent minting 0 token and emit a confusing event at the low level _mint function.

Consider implementing the check in the _mint function.

Staking initialize has duplicated code

The following snippet of code is duplicated inside the initialize function

IERC20Upgradeable(YIELDY_TOKEN).approve(
  LIQUIDITY_RESERVE,
  type(uint256).max
);

Consider removing the duplicated code.

BatchRequests allow adding the same address to the contracts list

contracts array is used by sendWithdrawalRequests and canBatchContracts function to batch call and check which contract are "batchable".

Because the same contract can be added multiple time, this can result in sendWithdrawalRequests to call contract.sendWithdrawalRequests multiple time in the same transaction.

Prevent event "spamming"

Event emission are useful to monitor contract usage done via external tools/services. The contract should prevent to fire an event when not needed. For example, when a setter is called, the event should be emitted only when the value of the variable that will be updated is different compared to the new value passed as a parameter.

The following function could implement the mechanism to prevent emitting an event when the value has not changed:

Staking.sol

• setCurvePool
• setAffiliateFee
• setAffiliateAddress
• shouldPauseStaking
• shouldPauseInstantUnstaking
• setEpochDuration
• setCoolDownPeriod
• setWarmUpPeriod
• setTimeLeftToRequestWithdrawal

LiquidityReserve.sol

• setFee

affiliateFee in Staking contract has not upper bound

Not having an upper bound could allow the owner of the Staking contract to set affiliateFee equal to BASIS_POINTS and claim all the user token when claimFromTokemak is called.

Consider adding a max limit to the affiliateFee value.

Staking initialize function does not check input parameter

Both _epochDuration and _firstEpochEndTime are not validated at the moment. Consider adding some sanity check on those input parameters.

Missing event emission

Event emission are important to monitor contract activity with external tool/services. Consider adding event emission to the following function:

LiquidityReserve.sol

• addLiquidity
• removeLiquidity
• instantUnstake
• unstakeAllRewardTokens

Migration.sol

• moveFundsToUpgradedContract

Staking.sol

• unstakeAllFromTokemak
• sendWithdrawalRequests
• stake
• claim
• claimWithdraw
• instantUnstakeReserve
• instantUnstakeCurve
• unstake
• rebase
• addRewardsForStakers
• preSign

State variable COW_SETTLEMENT and COW_RELAYER declared in StakingStorage could be declared as constant

COW_SETTLEMENT and COW_RELAYER are initialized in Staking.sol initialize function with a constant address.

The initialization of those variable could be moved directly in their definition and transform those variables in constants, optimizing the gas usage for portion of code that use them.

Staking _sendAffiliateFee should avoid sending fee when the amount is zero

feeAmount in _sendAffiliateFee could be zero because of rounding if (_amount * affiliateFee) < BASIS_POINTS.

The function could check feeAmount > 0 and perform IERC20Upgradeable(TOKE_TOKEN).safeTransfer(FEE_ADDRESS, feeAmount) only in that case

Staking transferToke should avoid sending Toke when the amount is zero

If totalTokeAmount is equal to zero, avoid to call safeTransfer and waste gas sending 0 transferring zero tokens.