Biconomy Hyphen 2.0 contest - catchup's results

Lines of code

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

Vulnerability details

Impact

equilibriumFee and maxFee values are used directly within getTransferFee() function without any checks performed. If these values were entered faulty by mistake at addSupportedToken() or changeFee(), such as too high maxFee, or equilibriumFee > maxFee, then transfer fee calculations will be incorrect. It would be good to check these settings against some default values and against each other before using them in getTransferFee() function.

Proof of Concept

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

Tools Used

manual analysis

Recommended Mitigation Steps

Include checks such as: require(maxFee <= maxAllowedFee, "maxFee higher than allowed"); require(equilibriumFee < maxFee, "equilibriumFee faulty");

C4 finding submitted: (risk = 1 (Low Risk)) updateTokenCap() updates only the transferConfig cap values, but not deposit config values.

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol#L111 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol#L112 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol#L96

Vulnerability details

addSupportedToken() writes the cap limits to transferConfig first and then copies the same values to deposit config by tokensInfo[tokenAddress].tokenConfig = transferConfig[tokenAddress]; However, updateTokenCap() updates only the transferConfig cap values, but not deposit config values.

Impact

There is no way to update tokensInfo[tokenAddress].tokenConfig

Proof of Concept

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol#L111 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol#L112 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol#L96

Tools Used

Manual analysis

Recommended Mitigation Steps

Either update the deposit tokenConfig values the same as transfer config values such as: tokensInfo[tokenAddress].tokenConfig = transferConfig[tokenAddress]; Or include seperate min max parameters in updateTokenCap() for deposit config and use them.

C4 finding submitted: (risk = 1 (Low Risk)) getTokensInfo() returns transfer min-max values

Lines of code

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

Vulnerability details

addSupportedToken() writes the cap limits to transferConfig first and then copies the same values to deposit config by tokensInfo[tokenAddress].tokenConfig = transferConfig[tokenAddress]; However, updateTokenCap() updates only the transferConfig cap values, but not deposit config values.

Impact

getTokensInfo() returns transferConfig[tokenAddress] where it should be tokensInfo[tokenAddress].tokenConfig

Proof of Concept

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

Tools Used

Manual analysis

Recommended Mitigation Steps

Line 121 should be changed as: tokensInfo[tokenAddress].tokenConfig;

C4 finding submitted: (risk = 1 (Low Risk)) maxFee is not checked against a max value while being updated

Lines of code

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

Vulnerability details

changeFee() maxFee should be checked against a maximum value (like 10%) to make sure it is not set too high by mistake.

Impact

maxFee can be set beyond limits which would mess up fee calculations.

Proof of Concept

getTransferFee() function calculations will give unpredicted results. https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L342

Tools Used

Manual analysis

Recommended Mitigation Steps

A check should be added to changeFee() such as: require(_maxFee <= maxAllowedFee, "maxFee higher than allowed");

C4 finding submitted: (risk = 1 (Low Risk)) changeFee() should have tokenChecks(tokenAddress) modifier

Lines of code

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

Vulnerability details

changeFee() is not checking if the tokenAddress is zero and the token is supported.

Impact

Unnecessary execution if the tokenAddress is zero or the token is not supported

Proof of Concept

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

Tools Used

Manual analysis

Recommended Mitigation Steps

changeFee() should include tokenChecks(tokenAddress) modifier

C4 finding submitted: (risk = 0 (Non-critical)) No need to define abicoder V2

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L4 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/LPToken.sol#L3

Vulnerability details

abicoder V2 is explicitly defined at LPToken.sol, LiquiditPool.sol. Starting from solidity version 0.8.0 abi coder V2 is activated by default.

Impact

Unnecessary code

Proof of Concept

https://docs.soliditylang.org/en/v0.8.11/080-breaking-changes.html

Tools Used

Manual analysis

Recommended Mitigation Steps

These pragma's can be removed.

C4 finding submitted: (risk = 0 (Non-critical)) executors and executorStatus can be made private.

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/ExecutorManager.sol#L9 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/ExecutorManager.sol#L10

Vulnerability details

Internal state variables can be private

Impact

Proof of Concept

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/ExecutorManager.sol#L9 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/ExecutorManager.sol#L10

Tools Used

Manual analysis

Recommended Mitigation Steps

Change executors and executorStatus visibility as private.

C4 finding submitted: (risk = 0 (Non-critical)) setBaseGas() takes a parameter of uint128

Lines of code

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

Vulnerability details

setBaseGas() takes a parameter of uint128 but baseGas is actually uint256

Impact

Better coding practice

Proof of Concept

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

Tools Used

Manual analysis

Recommended Mitigation Steps

Change the parameter to uint256.

C4 finding submitted: (risk = 0 (Non-critical)) typo in comment

Lines of code

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

Vulnerability details

There is a typo for the word 'after'

Impact

Proof of Concept

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

Tools Used

Manual analysis

Recommended Mitigation Steps

Fix the typo.