ankurdubey521
commented
2 years ago Duplicate of #140
Closed code423n4 closed 2 years ago
ankurdubey521
commented
2 years ago Duplicate of #140
pauliax
commented
2 years ago Similar to #140
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.