Frontrunning of setPerTokenWalletCap edge case

[code423n4]

Lines of code

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

Vulnerability details

Impact

The setPerTokenWalletCap() function in WhitelistPeriodManager.sol contains a comment stating:

Special care must be taken when calling this function
There are no checks for _perTokenWalletCap (since it's onlyOwner), but it's essential that it should be >= max lp provided by an lp.
Checking this on chain will probably require implementing a bbst, which needs more bandwidth
Call the view function getMaxCommunityLpPositon() separately before changing this value

Even if the manual step of calling the getMaxCommunityLpPositon() function is properly performed, it is possible for a user to add liquidity to increase the maxLp value in between when the getMaxCommunityLpPositon() function is called and when the setPerTokenWalletCap() function is called. Because this process is manual, this doesn't need to be bot frontrunning in the same block as when the setPerTokenWalletCap() function is called, but can be cause by poor timing of an innocent unknowing user adding liquidity to the protocol. If this condition occurs, the liquidity provider will have provided more liquidity than the perTokenWalletCap limit, breaking the assumptions for this variable and leading to some denial of service conditions.

This edge situation can impact the setTotalCap() function and the "perTokenTotalCap[_token]" state variable as well, but the "perTokenWalletCap[_token]" value would have to be reduced before the "perTokenTotalCap[_token]" value is reduced. The impact to setTotalCap() follows the same execution path but adds the additional step of calling the setTotalCap() function at the end of the process.

Proof of Concept

1. Owner calls getMaxCommunityLpPositon(_token) function to identify maxLp value to confirm new perTokenWalletCap value is below maxLp value
2. An innocent user adds liquidity to their position without the knowledge that the owner is going to reduce the "perTokenWalletCap[_token]" value soon
3. Owner calls setPerTokenWalletCap() function to reduce "perTokenWalletCap[_token]" value
4. The innocent user has more liquidity than the new "perTokenWalletCap[_token]" value. This means that the user can be in a situation where if they remove x amount of liquidity and attempt to add x liquidity back to their position, the innocent user will be unable to do so. Other functions that rely on the assumption that the largest user deposit is below the "perTokenWalletCap[_token]" value may break due to incorrect assumptions

This edge situation can impact the setTotalCap() function and the "perTokenTotalCap[_token]" state variable as well, but the "perTokenWalletCap[_token]" value would have to be reduced before the "perTokenTotalCap[_token]" value is reduced. The impact to setTotalCap() follows the same execution path but adds the additional step of calling the setTotalCap() function at the end of the process.

Tools Used

Manual analysis

Recommended Mitigation Steps

A programmatic solution is the only way to avoid these edge case scenarios, though it will increase gas consumption. To convert the manual calling of getMaxCommunityLpPositon(_token) to a programmatic solution, add the following require statement next to the existing require statement of the setPerTokenWalletCap() function: require(_perTokenWalletCap <= getMaxCommunityLpPositon(_token), "ERR_PWC_GT_MCLP");

[pauliax]

The concern is valid but I do not think that there is any profit for the attacker, and the impact for the regular users is minimal because this value can be updated anytime again by the owner, so I am hesitating if this should be of medium severity or lower, but because the warden provided a nice and comprehensive description, I will leave this in favor of warden.