Biconomy Hyphen 2.0 contest - PPrieditis's results

Lines of code

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

Vulnerability details

Impact

TokenManager can add and remove support for tokens. If token support is withdrawn during Cross Chain Transfer or if there is still liquidity in Liquidity Pool then funds become locked.

Proof of Concept

Below is a scenario when funds will get locked during Cross Chain Transfer:

1. User starts Cross Chain Transfer and Deposit Transaction is completed however Transfer Transaction is not yet completed.
2. TokenManager removes support for the token.
3. Executor can't invoke Transfer Transaction because token isn't supported anymore.

Below is a scenario when funds will get locked during Cross Chain Transfer:

1. User adds liquidity.
2. TokenManager removes support for the token.
3. User can't remove anymore liquidity.

Tools Used

Recommended Mitigation Steps

1. Remove modifier tokenChecks from LiquidityPool.sendFundsToUser()
2. Remove "require(_isSupportedToken(_tokenAddress), "ERR__TOKEN_NOT_SUPPORTED");" from LiquidityProviders.removeLiquidity()

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 https://github.com/OpenZeppelin/openzeppelin-contracts/issues/1488#issuecomment-550295609

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 - https://github.com/code-423n4/2021-12-sublime-findings/issues/95 2 - https://github.com/code-423n4/2021-06-realitycards-findings/issues/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); }

Title: Loops aren't caching length property.

Impact

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack. Caching the array length in the stack saves around 3 gas per iteration. Issue from a different contest: https://github.com/code-423n4/2022-01-timeswap-findings/issues/151

Recommended Mitigation Steps

Code example: for (uint256 i = 0; i < executorArray.length; ++i) {...} should be changed to: uint arrayLength = executorArray.length; for (uint256 i; i < arrayLength; ++i){...} https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/ExecutorManager.sol#L31 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/ExecutorManager.sol#L47

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol#L78 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/WhitelistPeriodManager.sol#L180 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/WhitelistPeriodManager.sol#L228 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/LPToken.sol#L77 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol#L78

---

Title: No need to initialize variables with default values

Impact

Explicitly initializing it with its default value is an anti-pattern and wastes gas. The same issue from a different contest: https://github.com/code-423n4/2022-01-yield-findings/issues/56

Recommended Mitigation Steps

Use default values for: https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityFarming.sol#L232-L233 uint256 index; for (index = 0; index < nftsStakedLength; ++index) {

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/WhitelistPeriodManager.sol#L180 for (uint256 i = 0; i < _addresses.length; ++i) {

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/WhitelistPeriodManager.sol#L228 for (uint256 i = 0; i < _tokens.length; ++i) {

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/LPToken.sol#L77 for (uint256 i = 0; i < nftIds.length; ++i) {

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol#L78 for (uint256 index = 0; index < tokenConfig.length; ++index) {

---

Title: Public functions to external (for specific functions)

Impact

Functions which are not used internally by contract should be set as external to save gas and improve code quality. The difference is because in public functions, Solidity immediately copies array arguments to memory, while external functions can read directly from calldata. Memory allocation is expensive, whereas reading from calldata is cheap.

Recommended Mitigation Steps

Below is a list of places where the change is necessary: 1 - LiquidityPool::setTrustedForwarder() https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L107 2 - LiquidityPool::setLiquidityProviders() https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L113

---

Title: "> 0" is less efficient than "!= 0" for unsigned integers

Impact

It is cheaper to use != 0 than > 0 and > than <= for uint256.

Recommended Mitigation Steps

Change !=0 with >0 and <= with >.

Similar optimizations for other contests: 1 - https://github.com/code-423n4/2022-01-trader-joe-findings/issues/240 2 - https://github.com/code-423n4/2022-01-elasticswap-findings/issues/161

It is necessary to change: https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L162 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityFarming.sol#L208 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityFarming.sol#L307 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityFarming.sol#L371 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L162 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L253 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L256 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L288 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L292 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L376 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L385 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L406 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/WhitelistPeriodManager.sol#L187-L188 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/WhitelistPeriodManager.sol#L203 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol#L49-L50

---

Title: LiquidityPool.getRewardAmount() refactoring

Impact

We can make code cleaner. We can save 3 gas by replacing ">=" with "<". Remove unnecessary write/read for variable rewardAmount what would save MSTORE, SSTORE (6 gas).

Recommended Mitigation Steps

Change code from: if (amount >= liquidityDifference) { rewardAmount = incentivePool[tokenAddress]; } else { // Multiply by 10000000000 to avoid 0 reward amount for small amount and liquidity difference rewardAmount = (amount * incentivePool[tokenAddress] * 10000000000) / liquidityDifference; rewardAmount = rewardAmount / 10000000000; } To: if (liquidityDifference < amount) { // Multiply by BASE_DIVISOR to avoid 0 reward amount for small amount and liquidity difference rewardAmount = (amount * incentivePool[tokenAddress] * BASE_DIVISOR) / liquidityDifference / BASE_DIVISOR; } else { rewardAmount = incentivePool[tokenAddress]; }

---

Title: LiquidityPool.getAmountToTransfer() refactoring for totalGasUsed

Impact

Variable baseGas is unnecessary and gas amount should be already in included in transferOverhead. The benefit of baseGas is that we can change it in a event of opcode gas cost change however in a such event it is very likely that it would be necessary to also change transferOverhead. If we would remove baseGas then we would avoid SLOAD and in this case it is 2100 gas because baseGas is not in touched_storage_slots (cold access).

Recommended Mitigation Steps

Remove all baseGas references and change code: https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L330-L332 From: uint256 totalGasUsed = initialGas - gasleft(); totalGasUsed = totalGasUsed + tokenManager.getTokensInfo(tokenAddress).transferOverhead; totalGasUsed = totalGasUsed + baseGas;

To: uint256 totalGasUsed = initialGas - gasleft() + tokenManager.getTokensInfo(tokenAddress).transferOverhead;

---

Title: LiquidityProviders.getTokenPriceInLPShares() reading twice from storage the same variable

Impact

Reading from storage is much more expensive than reading from memory. In this specific case storage (warm access) costs 100 gas and can be replaced with reading from memory so we would save 97 gas.

Recommended Mitigation Steps

Change code: https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L183 From: return totalSharesMinted[_baseToken] / totalReserve[_baseToken]; To: return supply / totalReserve[_baseToken];