Biconomy Hyphen 2.0 contest - hyh's results

Lines of code

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

Vulnerability details

Impact

Remainder nft.unpaidRewards are lost and cannot be retrieved after LiquidityFarming.withdraw. I.e. it is not possible to extractRewards for unpaidRewards later if withdraw being called when balance wasn't sufficient to fulfil the full withdraw of the rewards due, which is reasonably inconvenient to estimate for a user in advance

Proof of Concept

LiquidityFarming._sendRewardsForNft is being called by user facing withdraw and extractRewards functions.

There, when the balance isn't sufficient for full rewards transfer to a user, the remainder is stored in unpaidRewards variable:

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

If this happens during extractRewards call, it is possible to run reward gathering again later, when there is enough balance to retrieve unpaidRewards:

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

However, when reward token balance happened to be not sufficient during LiquidityFarming.withdraw, the unpaidRewards will be lost for the user as nftInfo structure required for reward gathering logic is unconditionally deleted:

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

Recommended Mitigation Steps

A way to enable the residual reward retrieval after the withdraw can be introduced to extractRewards -> _sendRewardsForNft logic.

For example, instead of deletion of the nftInfo structure, a flag can be set indicating that stake is no longer active, but remainder rewards are to be retrieved. When extractRewards is being called with this flag on, only nft.unpaidRewards retrieval is attempted

Lines of code

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

Vulnerability details

Impact

On a combination of high enough token value and low enough decimals there can be not enough precision to store reward amount, which can be permanently hid from a user as a result. I.e. on such a combination there will effectively be no rewards for some users as the reward amount calculations will yield zeros due to not having enough decimals to store the value.

For example, if the amount is 0.01 WBTC, accTokenPerShare is 1e-7 (1e-7 of WBTC is rewarded per 1 staked WBTC; this can be the case at the start, when not much rewards accrued yet), (amount * pool.accTokenPerShare) / ACC_TOKEN_PRECISION is zero.

This is loss of value case, but the impact value and probability is low enough, so setting the severity to medium.

Proof of Concept

Some ERC20 have low decimals:

https://github.com/d-xo/weird-erc20#low-decimals

And the least significant digit value can be high enough, for example WBTC have 8 decimals, so at the moment 1 is $0.0004:

https://etherscan.io/token/0x2260fac5e5542a773aa44fbcfedf7c193bc2c599

pool.accTokenPerShare is the accrued amount to be distributed as a reward divided by total amount staked with ACC_TOKEN_PRECISION precision:

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

Staked amount has token decimals as shares multiplier is removed:

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

In the current implementation reward per base token and base token amount are multiplied, which can yield zero after percentage base division:

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

Recommended Mitigation Steps

Consider using the shares without BASE_DIVISOR removal. Also, it can be useful to introduce the minimum amount to send, so that amounts below that be stored without the attempt to send.

Lines of code

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

Vulnerability details

Impact

Reward and base token decimals can differ, while this difference isn't accounted for in the reward amount calculations, which will lead to either missing rewards or sending the whole rewards balance to the first eligible user.

For example:

If reward is native, while base is USDC, then rewards are basically zero as 1 USDC is 1e-12 native token, so the USDC amount used as the base of reward calculation makes negligible the whole reward amount.

If reward is USDC, and base is native, the first reward amount due will most probably use the whole available balance.

Both cases will lead to losses of the rewards for the most of the LP users

Proof of Concept

LP shares saved in the LpTokenMetadata structure have the decimals of {18 + base token's decimals}:

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

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

When shares are used in LiquidityFarming, their decimals are reduced to base token's, for example:

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

However, this base token amount is then used to calculate reward amounts without decimals adjustment. In other words, rewardTokens[baseToken] is treated as if it is guaranteed to have the same decimals as baseToken:

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

This isn't enforced in any way, the reward token for a given base token can be arbitrary:

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

If reward and base token's decimals differ, the reward accounting can become grossly incorrect proportionally to the decimals difference

Recommended Mitigation Steps

Most common approach here is to add the token decimals variables to the contract and the decimals difference multiplier to the rewards calculations