neogranicen - MasterchefV2 exhibits inherent incompatibility issues with LB pools.

[sherlock-admin2]

neogranicen

Medium

MasterchefV2 exhibits inherent incompatibility issues with LB pools.

Summary

Although the protocol is supposed to let both V2 and LB pools be added to the masterchefV2 and earn rewards, adding the latter wont be possible since the masterchef assumes the tokens are going to be Erc20s when those of the LB are ERC1155

Vulnerability Detail

The following bolded words in the readme, of this audit repo should be sufficient proof that LB pools are to be added to MasterChefV2 because they demonstrate clearly and explictly that there will be bribes and voting for LB pairs, and the only purpose for those 2 things is to get a pool added and give it weight so that its stakers earn rewards.

Voter: Voting contract for voting on Uniswap V2 and LB pairs. Voters need a MLUM staking position.

BribeRewarders: Bribe pools (V2 / LB) for getting votes

Now the problem is that the Lp tokens of LB pools, which is a mechanism introduced by Trader Joe which the protocol is a fork of, are ERC1155.

Erc1155 tokens need to be passed an additional ID parameter, and if called using Erc20safe operations or using the Erc20 interface like is done in the MasterChefV2 the calls will fail.

As it can be seen here before, the addition of a new pool balanceOf is called on the lp token as a sanity check and it will always fail here on with LB tokens since the function doesn`t exist.

MasterchefV2.sol#L381

token.balanceOf(address(this)); // sanity check  

Impact

At the time of publishing this, out of the top 5 liquidity pools in terms of volume on IoTa 4 are of the type LB and only one is of the type V2:

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/MasterchefV2.sol#L381

Tool used

Manual Review

Recommendation

Consider making a low-level call to both balanceOf(address) and balanceOf(address,id) and then storing the type of token based on which call succeeds.

Since liquidity carry an equal liquidity value irrespective of the bin(id) it is contained in as is mentioned in section 3.5 of the Liquidity Book whitepaper, you can store liquidity deposited from all ids of a specific LB token togather.

You can then add seperate branches for transferfrom and trasnfer based on wether the token is of type LB or V2

[0xSmartContract]

No loss of funds and low impact

[neogranicen]

Escalate This Breaks core contract functionality, since it won't be possible to add pools of type lb which contain the majority of liquidity, and the whole functionality of this contract is rewarding pools, making this a medium as specified by Sherlock not a low.

[sherlock-admin3]

Escalate This Breaks core contract functionality, since it won't be possible to add pools of type lb which contain the majority of liquidity, and the whole functionality of this contract is rewarding pools, making this a medium as specified by Sherlock not a low.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

[0xSmartContract]

It's an interesting issue, and would be valid if the pool in question was indeed a Trader Joe (Your project will only run on the IotaEVM chain and not Avalanche. ) LB pool or an ERC1155.

However consider the following info:

The function add()'s signature uses IERC20 The contest README states the protocol only works with ERC20 The pools were out-of-scope, i.e. we do not know what the LB pool looks like, and we can't possibly know because the protocol team's main repo was private. Then the pool being a fork of Trader Joe's is an assumption, and not to mention non-ERC20s are also out-of-scope to begin with

[neogranicen]

It's an interesting issue, and would be valid if the pool in question was indeed a Trader Joe (Your project will only run on the IotaEVM chain and not Avalanche. ) LB pool or an ERC1155.

However consider the following info:

The function add()'s signature uses IERC20 The contest README states the protocol only works with ERC20 The pools were out-of-scope, i.e. we do not know what the LB pool looks like, and we can't possibly know because the protocol team's main repo was private. Then the pool being a fork of Trader Joe's is an assumption, and not to mention non-ERC20s are also out-of-scope to begin with

The thing is the criteria stated for the issue to be valid is already met the protocol forked trader joe avalanche lb pools and created LB pools on IotaEVM that are ERC1155 and hold the majority of magic sea Iota EVM liquidity.

I would like to provide any information necessary to prove for you this and you can look at source code for some of the lb pools like this and see that they are erc1155.

as an example take the very top and one of the most important in iota which is the pool between iota and usdc which can be found here: https://app.magicsea.finance/liquidity/manual/:8822/add/v21/0xa86d3169d5cccdc224637adad34f4f1be174000c/20?showTop=true

If you check the token associated with the contract you can see that it is an ERC1155 from the iota explorer: https://explorer.evm.iota.org/token/0xa86d3169d5cccdC224637aDAd34F4F1Be174000C

This image illustrates where

Now this applies to the rest of the pool that holds the majority of liquidity and you can check for yourself.

[0xSmartContract]

@WangSecurity can i get your opinion?

The pools were out-of-scope, i.e. we do not know what the LB pool looks like, and we can't possibly know because the protocol team's main repo was private. Then the pool being a fork of Trader Joe's is an assumption, and not to mention non-ERC20s are also out-of-scope to begin with

[neogranicen]

@0xSmartContract @WangSecurity Hey guys I found this additional info that will be helpful in the discussion: https://docs.magicsea.finance/protocol/liquidity-pools

The concept of LB pools can only work if the pool token is an ERC1155 because of the nature of these pools and the concept of bins.

[neogranicen]

@0xSmartContract @WangSecurity Guys, I think I have found conclusive proof this time for what LB pools really look like if you look at the example I gave you above of the usdc.e and iota which has $100K on liquidity: https://explorer.evm.iota.org/token/0xa86d3169d5cccdC224637aDAd34F4F1Be174000C

You can click here to see the contract itself:

When you go to the page you click on the contract code and you see it's small which gives us a hint this contract maybe a proxy:

If we go to the internal transaction we see that this contract has been constantly using delegate a LbPair contract and this combined with the above is sufficient to conclude that this contract is a proxy.

If we go to the implementation you find the source code and that it is a fork of trader joe v2 and an ERC1155 because if we look at the contract we see it inherits LB tooke:

we check the Lb token source code and comments and we see its an ERC1155:

[dahof1]

Correct, LB Pools are ERC1155 Tokens. In production we will not add this token to the masterChef.

So what we do is (like Trader Joe/Merchant Moe): We create an Hook Contract for the LB Token Pair. The Hook itself is an ERC20 Token which will get added to the MasterChef. The hook contract rewards then any user holding active liquidity.

In this way we can reward both: LB pairs and V2 pairs

The LB Rewarder is out of scope but if you interested in, here is the audit report: https://paladinsec.co/assets/audits/20240331_Paladin_TraderJoe_Core2.2_Final_Report.pdf

@neogranicen @WangSecurity @0xSmartContract