Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reward per second can overflow in liquidity farming #36

Closed
code423n4 opened this issue Mar 12, 2022 · 1 comment
Closed

Reward per second can overflow in liquidity farming #36

code423n4 opened this issue Mar 12, 2022 · 1 comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate This issue or pull request already exists sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

The owner of the contract can lock user principal in liquidity farming

Proof of Concept

The setRewardPerSecond function gives the owner the power to change the rewardPerSecond for the stacking without any maximum amount. The owner could setRewardPerSecond to a very high number for any base token.

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

If the stacker then wants to withdraw their liquidity NFT they are forced to call the updatePool function. This function calls getUpdatedAccTokenPerShare which computes an accumulator variable by adding the different reward amounts multiplied by the time in seconds. If rewardPerSecond is high enough the accumulator can be made close to max uint which will make the arithmetic will revert and the user's principal will be locked.

The math to compute the accumulator is unchecked so it won't revert but if the accumulator is made high enough it will revert when multiplied by the precision constant.
https://github.com/code-423n4/2022-03-biconomy/blob/db8a1fdddd02e8cc209a4c73ffbb3de210e4a81a/contracts/hyphen/LiquidityFarming.sol#L274

For example, if rewardPerSecond is set to 2**250 and a few seconds later to 0 the accumulator will be in that order of magnitude, and accumulator * ACC_TOKEN_PRECISION will revert the withdraw transaction.

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

I consider this a medium issue given it's a critical attack but it's only performable by the contract's governance.

Recommended Mitigation Steps

Add a maximum amount to reward per second to protect from overflows.

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Mar 12, 2022
code423n4 added a commit that referenced this issue Mar 12, 2022
@ankurdubey521 ankurdubey521 added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Mar 30, 2022
@pauliax
Copy link
Collaborator

pauliax commented May 3, 2022

#80 (comment)

@pauliax pauliax closed this as completed May 3, 2022
@pauliax pauliax added the duplicate This issue or pull request already exists label May 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate This issue or pull request already exists sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

3 participants