QA Report

[code423n4]

Title: Critical changes should use two-step procedure

Impact

Changing critical addresses in contracts should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one (i.e. claims ownership). This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

Recommended Mitigation Steps

OZ still has open GitHub issue on how precisely two-step process should look like however probably the best solution would be to use code what is similar in comment OpenZeppelin/openzeppelin-contracts#1488 (comment)

Below is a list of places what should use two-step process:
1 - hyphen/ExecutorManager.sol is using OZ Ownable. Ownable transferOwnership is a one-step process and should be changed to a two-step process.
2 - security/Pausable.sol _pauser variable.
3 - hyphen/token/LPToken.sol uses OZ OwnableUpgradeable.sol
4 - hyphen/LiquidityFarming.sol uses OZ OwnableUpgradeable.sol
5 - hyphen/LiquidityPool.sol OZ OwnableUpgradeable.sol
6 - hyphen/LiquidityProviders.sol OZ OwnableUpgradeable.sol
7 - hyphen/WhitelistPeriodManager.sol OZ OwnableUpgradeable.sol

Examples of similar issues from other contests:
1 - code-423n4/2021-12-sublime-findings#95
2 - code-423n4/2021-06-realitycards-findings#105

---

Title: Events should be declared in an interface and not in a contract

Impact

Interfaces are basically limited to what the Contract ABI can represent, and the conversion between the ABI and an interface should be possible without any information loss. https://docs.soliditylang.org/en/v0.8.12/contracts.html?highlight=interface#interfaces
It is necessary to declare event inside interface otherwise it is not available externally via interface. For example web3 javascript/nodejs code will use interface and not contract to listen for events.

Recommended Mitigation Steps

Move events from contracts to interfaces. Example in Uniswap with event in interface: https://github.com/Uniswap/v3-core/blob/main/contracts/interfaces/IUniswapV3Factory.sol

---

Title: Wrong contract element layout order

Impact

Contract should use proper layout order to improve readability.

Recommended Mitigation Steps

Inside each contract, library or interface, use the following order:

1. Type declarations
2. State variables
3. Events
4. Modifiers
5. Functions
   https://docs.soliditylang.org/en/v0.8.12/style-guide.html?highlight=interface#order-of-layout

Order should be changed for:
https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol#L13-L30
https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L34-L46

---

Title: Remove or fully implement OZ Pausable in TokenManager.sol

Impact

TokenManager gives impression that it is using OZ Pausable however functionality actually is not used. It is not clear if it was forgotten to implement pause() and unpause() or OZ Pausable should be removed. There is only one Pausable modifier used "whenNotPaused" however it is not possible to "pause". Also it does not make a lot of sense to use whenNotPaused for function changeFee() because there is also modifier onlyOwner.

Recommended Mitigation Steps

Remove or fully implement OZ Pausable for:
https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol

---

Title: Missing event emission

Impact

FeeChanged event should be emitted when protocol fee has been changed. TokenManager.sol can change protocol fee via function changeFee() and addSupportedToken() however addSupportedToken() does not emit an event. It is possible to call addSupportedToken() after changeFee() and change would not be detected by off chain event listeners.

Recommended Mitigation Steps

Emit FeeChanged in function addSupportedToken().

---

Title: Scientific notation for big numbers in LiquidityPool.sol

Impact

In order to improve code readability it is better to use scientific notation for big numbers.

Recommended Mitigation Steps

In place of 10000000000 use 1e10
https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L20

---

Title: Hardcoded value in place of constant

Impact

LiquidityPool.sol has a constant BASE_DIVISOR however there is a place in code where there is hardcoded value in place of a constant BASE_DIVISOR.

Recommended Mitigation Steps

Change hardcoded value with constant BASE_DIVISOR:
https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L184-L185

---

Title: LiquidityPool.getAmountToTransfer() should be simplified

Impact

Function getAmountToTransfer() has some unnecessary steps and code can be simplified. Change would also decrease gas costs.

Recommended Mitigation Steps

Change code: https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L316-L326
From:
if (transferFeePerc > tokenManager.getTokensInfo(tokenAddress).equilibriumFee) {
// Here add some fee to incentive pool also
lpFee = (amount * tokenManager.getTokensInfo(tokenAddress).equilibriumFee) / BASE_DIVISOR;
incentivePool[tokenAddress] =
(incentivePool[tokenAddress] +
(amount * (transferFeePerc - tokenManager.getTokensInfo(tokenAddress).equilibriumFee))) /
BASE_DIVISOR;
} else {
lpFee = (amount * transferFeePerc) / BASE_DIVISOR;
}
uint256 transferFeeAmount = (amount * transferFeePerc) / BASE_DIVISOR;

To:
uint256 transferFeeAmount = (amount * transferFeePerc) / BASE_DIVISOR;
if (transferFeePerc > tokenManager.getTokensInfo(tokenAddress).equilibriumFee) {
// Excess fee is added to incentivePool
lpFee = (amount * tokenManager.getTokensInfo(tokenAddress).equilibriumFee) / BASE_DIVISOR;
incentivePool[tokenAddress] = incentivePool[tokenAddress] + transferFeeAmount - lpFee;
} else {
lpFee = transferFeeAmount;
}

---

Title: LiquidityPool.withdrawErc20GasFee() and LiquidityPool.withdrawNativeGasFee() should be simplified

Impact

Refactoring has minimal positive effect on gas costs however main benefit is to have smaller code base. Also it would be better to use NATIVE constant than address(this) in event because NATIVE constant address already is used to describe native token however LiquidityPool address doesn't yet has such meaning.

Recommended Mitigation Steps

Refactor code: https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L372-L392
From:
function withdrawErc20GasFee(address tokenAddress) external onlyExecutor whenNotPaused nonReentrant {
require(tokenAddress != NATIVE, "Can't withdraw native token fee");
// uint256 gasFeeAccumulated = gasFeeAccumulatedByToken[tokenAddress];
uint256 _gasFeeAccumulated = gasFeeAccumulated[tokenAddress][_msgSender()];
require(_gasFeeAccumulated != 0, "Gas Fee earned is 0");
gasFeeAccumulatedByToken[tokenAddress] = gasFeeAccumulatedByToken[tokenAddress] - _gasFeeAccumulated;
gasFeeAccumulated[tokenAddress][_msgSender()] = 0;
SafeERC20Upgradeable.safeTransfer(IERC20Upgradeable(tokenAddress), _msgSender(), _gasFeeAccumulated);
emit GasFeeWithdraw(tokenAddress, _msgSender(), _gasFeeAccumulated);
}

function withdrawNativeGasFee() external onlyExecutor whenNotPaused nonReentrant {
uint256 _gasFeeAccumulated = gasFeeAccumulated[NATIVE][_msgSender()];
require(_gasFeeAccumulated != 0, "Gas Fee earned is 0");
gasFeeAccumulatedByToken[NATIVE] = gasFeeAccumulatedByToken[NATIVE] - _gasFeeAccumulated;
gasFeeAccumulated[NATIVE][_msgSender()] = 0;
(bool success, ) = payable(_msgSender()).call{value: _gasFeeAccumulated}("");
require(success, "Native Transfer Failed");

emit GasFeeWithdraw(address(this), _msgSender(), _gasFeeAccumulated);

}

To:
function withdrawGasFee(address tokenAddress) external onlyExecutor whenNotPaused nonReentrant {
uint256 _gasFeeAccumulated = gasFeeAccumulated[tokenAddress][_msgSender()];
require(_gasFeeAccumulated > 0, "Gas Fee earned is 0");
gasFeeAccumulatedByToken[tokenAddress] = gasFeeAccumulatedByToken[tokenAddress] - _gasFeeAccumulated;
gasFeeAccumulated[tokenAddress][_msgSender()] = 0;
if(tokenAddress == NATIVE){
(bool success, ) = payable(_msgSender()).call{value: _gasFeeAccumulated}("");
require(success, "Native Transfer Failed");
} else {
SafeERC20Upgradeable.safeTransfer(IERC20Upgradeable(tokenAddress), _msgSender(), _gasFeeAccumulated);
}
emit GasFeeWithdraw(tokenAddress, _msgSender(), _gasFeeAccumulated);
}