Biconomy Hyphen 2.0 contest - throttle's results

Lines of code

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

Vulnerability details

Impact

Owners have full control over the protocol

Proof of Concept

Owners have full control over:

• executors who perform token transfers on behalf of the destination chain
• reclaiming / withdrawing any tokens (including reward tokens) held by farming contract
• total upgradeability
• instant parameters change (no timelock)
• 1 step owner change (gold standard is 2-step owner change)

Tools Used

Manual review

Recommended Mitigation Steps

Make executors decentralized. Add TimeLock for parameter changes.

QA findings

---------------------------------------------------------------------------

1 - Executor arrays can contain obsolete executors

eExecutors can contain invalid data. Executor is never removed from array

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

Tools Used

Manual review

Recommended Mitigation Steps

Not sure what the intent is. Executor array si never used within smart contracts. If it's not needed than delte it and save gas and readability. If it is needed then fix it.

---------------------------------------------------------------------------

2 - Excluded user who added liquidity and then was unexcluded can block the withdrawals

Excluded user who added liquidity didn't account for totalLiquidty increase. Later, if he is removed from excluded list and tries to remove liqudity, totalLiquidty will be subtracted which can lead to DoS for other user who want to remove liquidity

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

Not sure if this is Medium or Low because excluding is in full custody of admins and the intention for that mechanism is not clear.

Tools Used

Manual review

Recommended Mitigation Steps

Not sure, depends on intent

---------------------------------------------------------------------------

3 - Validate input variables bounds

fee variables bounds are not checked. this can lead to expensive mistake

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

Tools Used

Manual review

Recommended Mitigation Steps

Validate bounds

---------------------------------------------------------------------------

Gas Savings

---------------------------------------------------------------------------

1 - Don't call constants form other contracts. Import them.

Calling contract in order to get constant value is unnecessarily expensive.

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

Tools Used

Manual review

Recommended Mitigation Steps

Import this constant or instantiate it.

---------------------------------------------------------------------------

2 - Cache state variable

Cache rewardRateLog[_baseToken][i] to save 1 SLOAD.

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

Tools Used

Manual review

Recommended Mitigation Steps

Consider state variable

---------------------------------------------------------------------------

3 - Loop iteration can be unchecked

Decrement can be unchecked because i is always > 0 when decrement happens.

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

Tools Used

Manual review

Recommended Mitigation Steps

Add unchecked { --i };

---------------------------------------------------------------------------

4 - Unused external call

Call to token metadata yields return variables which are not used later.

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

Tools Used

Manual review

Recommended Mitigation Steps

Remove external call to token metadata

---------------------------------------------------------------------------

5 - Reuse computed variables

getTokenPriceInLPShares() is called twice and yields the same result. Reuse the result to save gas

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

Tools Used

Manual review

Recommended Mitigation Steps

Reuse the result form previous call to save gas

---------------------------------------------------------------------------

6 - Function ifEnabled() can be rewritten

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/WhitelistPeriodManager.sol#L260-L262

Unnecessary SLOAD in logical expression. Also makes code less readable.

Tools Used

Manual review

Recommended Mitigation Steps

Refactor to:

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

---------------------------------------------------------------------------

7 - tokenChecks not necessary

tokenChecks is not necessary because function is disabling support so it doesn't matter what address token it is

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

Tools Used

Manual review

Recommended Mitigation Steps

Remove tokenChecks

---------------------------------------------------------------------------