Biconomy Hyphen 2.0 contest - minhquanym's results

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/db8a1fdddd02e8cc209a4c73ffbb3de210e4a81a/contracts/hyphen/LiquidityPool.sol#L319-L322

Vulnerability details

Impact

The getAmountToTransfer function of LiquidityPool updates incentivePool[tokenAddress] by adding some fee to it but the formula is wrong and the value of incentivePool[tokenAddress] will be divided by BASE_DIVISOR (10000000000) each time. After just a few time, the value of incentivePool[tokenAddress] will become zero and that amount of tokenAddress token will be locked in contract.

Proof of concept

Line 319-322

incentivePool[tokenAddress] = (incentivePool[tokenAddress] + (amount * (transferFeePerc - tokenManager.getTokensInfo(tokenAddress).equilibriumFee))) / BASE_DIVISOR;

Let x = incentivePool[tokenAddress], y = amount, z = transferFeePerc and t = tokenManager.getTokensInfo(tokenAddress).equilibriumFee. Then that be written as

x = (x + (y * (z - t))) / BASE_DIVISOR;
x = x / BASE_DIVISOR + (y * (z - t)) / BASE_DIVISOR;

Recommended Mitigation Steps

Fix the bug by change line 319-322 to:

incentivePool[tokenAddress] += (amount * (transferFeePerc - tokenManager.getTokensInfo(tokenAddress).equilibriumFee)) / BASE_DIVISOR;

Lines of code

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

Vulnerability details

Impact

• The depositErc20 function of LiquidityPool assumes that amount of tokenAddress token is transfer to itself and emit an event with value amount + rewardAmount after that.
• However, this may not be true if the tokenAddress is a transfer-on-fee token or a deflationary/rebasing token, causing the received amount to be less than the accounted amount.
• This can lead to loss of funds when executor see that event and call sendFundsToUser.

Proof of concept

• https://github.com/code-423n4/2022-03-biconomy/blob/db8a1fdddd02e8cc209a4c73ffbb3de210e4a81a/contracts/hyphen/LiquidityPool.sol#L170-L172
• Consider the scenario:
  • Assume X is a fee-on-transfer token (e.g WAV3, ZUKI), which take 10% of transfer amount as fee. Alice want to send 10000 X from chain Polygon to Avalanche.
  • Line 170 call transfer function with input amount = 10000. But because X take 10% fee, the actual amount of token X LiquidityPool received is amount * 0.9 = 9000.
  • So on Polygon, LiquidityPool token X balance is increased by 9000. (1)
  • Line 172 a Deposit event is emitted with amount + rewardAmount = 10000 + rewardAmount. Let's just assume rewardAmount = 0 for now, so amount emitted is 10000.
  • And base on documentation, Executor will listen to that Deposit event and call sendFundsToUser with the amount = 10000 from emitted event.
  • On Avalanche, Alice still received 9000 token X, because transfer X on Avalanche also take 10% fee.
  • But LiquidityPool token X balance decreased 10000. (2)
  • Combine (1) and (2), LiquidityPool have lost 1000 token X.

Recommended Mitigation Steps

• Get the actual received amount by calculating the difference of token balance before and after the transfer. For example:

uint256 balanceBefore = IERC20Upgradeable(tokenAddress).balanceOf(address(this));
SafeERC20Upgradeable.safeTransferFrom(IERC20Upgradeable(tokenAddress), sender, address(this), amount);
uint256 receivedAmount = IERC20Upgradeable(tokenAddress).balanceOf(address(this)) - balanceBefore;
emit Deposit(sender, tokenAddress, receiver, toChainId, receivedAmount + rewardAmount, rewardAmount, tag);

1. Not remove executor from executors array when call function removeExecutor

Impact

Function removeExecutor in ExecutorManager.sol did not remove executorAddress from state executors array. Public view function getAllExecutors can return address that is no longer executor

Proof of concept

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/ExecutorManager.sol#L53-L57

Recommended Mitigation Steps

Keep a mapping from executorAddress to index in executors list Given an index, we can remove an element from a array by swapping the element to the end of the array and pop it out of array. Example code

	address[] internal someArray;
	mapping(address => uint) internal indexMapping;

	function removeFromArray(address _address) public {
		require(indexMapping[_address] > 0);
		uint index = indexMapping[_address] - 1;
		someArray[index] = someArray[someArray.length - 1];
                	someArray.pop();
	}

More than that, we can use this indexMapping to replace executorStatus by viewing false in boolean as 0 in uint

2. Token fee can be change without emit event

Impact

Token fee of already added token can be change through function addSupportedToken and this function not emit event FeeChanged

Proof of concept

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/token/TokenManager.sol#L84-L99

Recommended Mitigation Steps

Check if token have already been added. require(!tokensInfo[tokenAddress].supportedToken, “Token already added”)

3. Inconsistent in visibility of setter functions

Impact

In LiquidityPool, all 4 function setTrustedForwarder, setLiquidityProviders, setBaseGas, setExecutorManager require onlyOwner but 2 of them have public visibility and 2 of them have external

Proof of concept

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L107 https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L113 https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L119 https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L127

Recommended Mitigation Steps

If owner is EOA, should change all of them to external to save gas

4. Inconsistent use of BASE_DIVISOR

Impact

LiquidityPool has a const BASE_DIVISOR = 10000000000 which is used for better accuracy. Should use BASE_DIVISOR in getRewardAmount function

Proof of concept

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L184-L185

Recommended Mitigation Steps

Replace 10000000000 with BASE_DIVISOR

5. Function ifEnabled can be simplify.

Impact

Function ifEnabled in WhitelistPeriodManger: https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/WhitelistPeriodManager.sol#L260-L262

function ifEnabled(bool _cond) private view returns (bool) {
    return !areWhiteListRestrictionsEnabled || (areWhiteListRestrictionsEnabled && _cond);
}

It actually just check if areWhiteListRestrictionsEnabled is false or _cond is true.

Proof of concept

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/WhitelistPeriodManager.sol#L260-L262 Let x = areWhiteListRestrictionsEnabled and y = _cond, then we can use truth table to prove that.

​​!x | (x & y) = !x | y

Use this link and type in the above formula to generate truth table. https://trutabgen.com/

Recommended Mitigation Steps

Change the code to

function ifEnabled(bool _cond) private view returns (bool) {
    return (!areWhiteListRestrictionsEnabled || _cond);
}

1. Cache length in the for loop and uncheck index

Impact

At each iteration of the loop, length is read from memory. We can cache the length and save gas per iteration. Solidity 0.8.0 check safe math in every operation. Use uncheck to increase index can save gas.

Proof of concept

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/token/TokenManager.sol#L78

Recommended Mitigation Steps

Optimize

uint256 length = tokenConfig.length;
for (uint256 index = 0; index < length;) {
    depositConfig[toChainId[index]][tokenAddresses[index]].min = tokenConfig[index].min;
    depositConfig[toChainId[index]][tokenAddresses[index]].max = tokenConfig[index].max;
    uncheck { ++index; }
}

2. Cache depositConfig can save gas

Impact

Function getDepositConfig of TokenManger return a struct in memory cost lots of gas. Cache depositConfig after call getDepositConfig can save gas.

Proof of concept

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L157-L158 https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L248-L249 https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L273-L274

Recommended Mitigation Steps

Optimize

DepositConfig config = tokenManager.getDepositConfig(toChainId, tokenAddress);
require(config.min <= amount && config.max >= amount, 
	"Deposit amount not in Cap limit"
);