Biconomy Hyphen 2.0 contest - pedroais's results

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityProviders.sol#L352

Vulnerability details

Impact

Liquidity provider will be unable to withdraw tokens that were previously supported

Proof of Concept

In liquidityProviders.sol users can provide liquidity only in supported tokens. This is checked when the user provides liquidity and checked again when he withdraws the liquidity.

If a user provides liquidity in a supported token and the token is then removed from the list he won't be able to withdraw the funds.

Supported tokens should only be checked in deposit and not in withdrawal. If the token was never supported the NFT won't be valid so the withdraw function will revert anyways.

I consider this a medium issue : Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Recommended Mitigation Steps

Only check the token is supported when depositing and not in withdrawals.

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.

Lines of code

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

Vulnerability details

Impact

An unbounded loop allows an attacker to lock stacker's principal in liquidity farming

Proof of Concept

In the liquidityFarming contract, deposits can be made for someone without their permission. This should be fine as deposits can only benefit the receiver. The problem is in the withdraw function which loops through all NFTs locked by the user until it finds the desired one.

An attacker could deposit multiple NFTS with dust amounts (just some wei of base token for each nft) to make the withdrawal loop very big and surpass the block gas limit. The receiver could be locked from withdrawing their true principal.

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

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L171 https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L273

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

When a user calls the deposit function the reward amount is calculated and an event is emited with amount+reward as the transfer amount. The function checks amount is smaller than the max amount.

An executor then listens to this event and calls sendFundsToUser with rewards + amount as the amount parameter. This function checks amount+reward is smaller than max amount.

This is a problem because the amount transferred may be in the limit but amount + reward could pass the limit and the executor won't be able to send the transaction. The user will lose the funds. Both checks should be made with the reward or without the reward but the checks should be the same for this not to happen.

Step by step : Max transfer is set to 50 for token A Bob transfers 49 tokens, this will pass since 49<50. The reward is calculated in 2 tokens. The executor then calls sendFundsToUser with 52. This transaction will revert and user will lose their tokens.

This value of amount includes rewards but the previous check didn't include rewards: https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L273

Recommended Mitigation Steps

Both checks should be made over the same amount = amount + rewards

Variables that are set in the initializer and can't change (there is no method to change them ) should be immutable to save gas : https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L26 https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L27

Variables that weigh less than 32 bytes should be grouped together to save storage slots (and gas): https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L29 (Address is 20 bytes and bool is 1 byte, they should be declared next to each other in the struct to save a slot)

Public functions that are never used inside the SAME contract should be external to save gas : https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L169

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L329

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L333

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L83

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L107

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L113

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityProviders.sol#L78

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityProviders.sol#L96

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityProviders.sol#L100

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityProviders.sol#L104

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityProviders.sol#L108

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityProviders.sol#L127

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityProviders.sol#L131

Unnecessary checks :

Checking approval is useless. If the approval isn't made the safeTransfer will revert anyway:

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L200

Same for this one : https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityProviders.sol#L270

If the NFT is already staked then it will be held inside the contract and the transfer will fail so checking this does nothing : https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L211

If the contract doesn't have enough balance the transfer will fail and success will be false which will make the transaction revert. This check does nothing : https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L288

The same for this one if the balance is not enough safeTransfer will revert : https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L292

Instead of setting = false delete could be used wich will free storage and return some gas : https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/token/TokenManager.sol#L101