Biconomy Hyphen 2.0 contest - Ruhum's results

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L170 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L325

Vulnerability details

Impact

Since the protocol is supposed to be able to bridge all kinds of tokens it's important to keep in mind that some tokens take fees on transfer and some might in the future (e.g. USDT). If a user deposits tokens that either have a fee-on-transfer or are rebasing tokens, the internal accounting of the protocol will be messed up. For example, the protocol will think that it has more tokens than it actually has. That will result in a loss of funds for the users.

Proof of Concept

See the linked code blocks where the user deposits funds. The contract doesn't verify how many tokens it received.

To support these kinds of tokens, the protocol has to verify the actual amount of tokens that were transferred:

uint oldBalance = ERC20(_token).balanceOf(address(liquidityPool));
SafeERC20Upgradeable.safeTransferFrom(IERC20Upgradeable(_token), _msgSender(), address(liquidityPool), _amount);
uint newBalance = ERC20(_token).balanceOf(address(liquidityPool));
uint actualAmount = newBalance - oldBalance;

Tools Used

none

Recommended Mitigation Steps

On incoming deposits, check how many tokens the contract received and use that value for internal accounting.

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityFarming.sol#L180

Vulnerability details

Impact

The LiquidityFarming contract has a function, reclaimTokens(), that allows the owner to withdraw all of the contract's funds. In some contracts, such a function is used as a last resort to save user funds in case of an emergency. But, the LiquidityFarming contract is both pausable and upgradable. It doesn't need to be able to withdraw all the funds. Instead, it poses a new security risk. In case the owner's keys are compromised all the funds can be stolen.

It doesn't seem to serve any purpose or affect the contract's usability. I'd recommend removing it.

Proof of Concept

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityFarming.sol#L180

Tools Used

none

Recommended Mitigation Steps

remove the reclaimTokens() function.

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L149-L173 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L242-L261

Vulnerability details

Impact

The LiquidityPool contract doesn't verify that the user transfers to a different chain. They are able to make a transfer within the same chain. I've tried to come up with a situation where an attacker would be able to drain the pool's funds through the reward system. But, the fees seem to be always larger than the reward amount.

Still, it isn't in anybody's interest to allow transfers within the same chain. It causes losses for the user (fees > reward) and the executor (gas costs).

Proof of Concept

See linked code blocks. There is no check for the chainID. Well, there can't be because the contract doesn't know what chain it's on.

Tools Used

none

Recommended Mitigation Steps

Add a chainID state variable and assign it a value in initalize(). On every deposit verify that toChainID != chainID.

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol#L44-L54 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol#L97-L98 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L352

Vulnerability details

Impact

There are no validity checks verifying that the maxFee of a token is actually larger than the equilibriumFee. If the maxFee is lower, transfers of that token will always fail because of an underflow. The executor will lose their gas on these failed transactions.

Proof of Concept

Here the following line will cause an underflow:

uint256 denominator = equilibriumFee * providedLiquidity + (maxFee - equilibriumFee) * resultingLiquidity;

Tools Used

none

Recommended Mitigation Steps

add a check that maxFee >= equilibriumFee

Two issues with rounding when withdrawing liquidity and fees:

User can't withdraw all of their liquidity if they leave small amount

// LiquidityProvers.test.ts

describe("Fee Calculation and Extraction", async function () {
  this.beforeEach(async () => {
    await token.connect(owner).approve(liquidityProviders.address, await token.balanceOf(owner.address));
    await token.connect(charlie).approve(liquidityProviders.address, await token.balanceOf(charlie.address));
    await liquidityProviders.addTokenLiquidity(token.address, 100);
    await liquidityProviders.connect(charlie).addTokenLiquidity(token.address, 300);
  });

  it.only("Should allow extraction of fee in ERC20", async function () {
    await liquidityProviders.addLpFeeTesting(token.address, 10);
    await liquidityProviders.increaseTokenLiquidity(1, 100);
    await liquidityProviders.addLpFeeTesting(token.address, 10);

    await expect(() => liquidityProviders.connect(charlie).removeLiquidity(2, 300)).to.changeTokenBalances(
      token,
      [liquidityPool, charlie],
      [-313, 313]
    );

    await expect(() => liquidityProviders.removeLiquidity(1, 199)).to.changeTokenBalances(
      token,
      [liquidityPool, owner],
      [-206, 206]
    );

    const result = await lpToken.tokenMetadata(1);
    console.log(result);
    expect(result.suppliedLiquidity.toString()).to.not.equal("0");
    expect(result.shares.toString()).to.not.equal("0");
  });
});

claimFees leaves a small dust amount of fees

// LiquidityProvers.test.ts

describe("Fee Calculation and Extraction", async function () {
  this.beforeEach(async () => {
    await token.connect(owner).approve(liquidityProviders.address, await token.balanceOf(owner.address));
    await token.connect(charlie).approve(liquidityProviders.address, await token.balanceOf(charlie.address));
    await liquidityProviders.addTokenLiquidity(token.address, 100);
    await liquidityProviders.connect(charlie).addTokenLiquidity(token.address, 300);
  });

  it.only("Should allow extraction of fee in ERC20", async function () {
    await liquidityProviders.addLpFeeTesting(token.address, 10);
    await liquidityProviders.increaseTokenLiquidity(1, 100);
    await liquidityProviders.addLpFeeTesting(token.address, 10);

    const charlieRewards = await liquidityProviders.getFeeAccumulatedOnNft(2);
    const aliceRewards = await liquidityProviders.getFeeAccumulatedOnNft(1);

    expect(charlieRewards.add(aliceRewards).toString()).to.equal("20");
  });
});

ExecutorManager doesn't properly remove an executor

Instead of fully removing the executor from the array, the removeExecutor() function only sets the executorStatus map to false. If somebody retrieves all the executors using getAllExecutors() they won't receive a correct representation of the executors.

To properly remove it from the array use the following code:

uint executorsLength = executors.length;
for (uint i; i < executorsLength; i++) {
  if (executors[i] == executorAddress) {
    executors[i] = executors[executorsLength - 1];
    executors.pop();
  }
}