vulnerability

[Cliffordwh]

i Would suggest removing!

   // Migrate lp token to another lp contract. Can be called by anyone. We trust that migrator contract is good.
    function migrate(uint256 _pid) public {
        require(address(migrator) != address(0), "migrate: no migrator");
        PoolInfo storage pool = poolInfo[_pid];
        IBEP20 lpToken = pool.lpToken;
        uint256 bal = lpToken.balanceOf(address(this));
        lpToken.safeApprove(address(migrator), bal);
        IBEP20 newLpToken = migrator.migrate(lpToken);
        require(bal == newLpToken.balanceOf(address(this)), "migrate: bad");
        pool.lpToken = newLpToken;
    }

[SiNONiMiTY]

Bumping this issue up, this migrator function is highlighted on Binance Docs itself as "malicious". PancakeSwap is gaining traction in DeFi scene, seeing this particular function that may serve as a backdoor in extracting assets might scare away potential investors and/or adopters that are savvy enough to check the contract.

See the link below, Item 4: https://www.binance.org/en/blog/how-to-identify-malicious-contract-on-binance-smart-chain/

Snippet https://github.com/pancakeswap/pancake-farm/blob/a61313bf107c7f82e1a0f5736d815041fbf8cdff/contracts/MasterChef.sol#L170

FYI @pancake-swap @fio666 @pancake-cat

[BaptisteGarcin]

That's kind of scary

[cyberena]

Ok so if I think I understand this correctly, it is in FACT already removed from there smart contract code seen here: https://bscscan.com/address/0x0e09fabb73bd3ade0a17ecc321fd13a19e81ce82#code Can anyone confirm or correct my understanding please

[josemtm]

Ok so if I think I understand this correctly, it is in FACT already removed from there smart contract code seen here: https://bscscan.com/address/0x0e09fabb73bd3ade0a17ecc321fd13a19e81ce82#code Can anyone confirm or correct my understanding please

https://bscscan.com/address/0x73feaa1ee314f8c655e354234017be2193c9e24e#code

i think that this is the contract that you are looking for the code is in fact there i would not use pancakeswap with that backdoor there, exist alternatives to migration if fact this is worse than good for the security

[cyberena]

Thanks! Can I ask how did you properly locate the right contract? I went to CMC and searched pancake and copied the contract address from there but apparently its wrong. How did you locate the proper one?

Appreciate the help!

[josemtm]

Thanks! Can I ask how did you properly locate the right contract? I went to CMC and searched pancake and copied the contract address from there but apparently its wrong. How did you locate the proper one?

Appreciate the help!

because its two diferent contracts the one you pick is the cake token contract the one i copy is the main staking contract that has the vulnerability

[cyberena]

Thanks @josemtm for your reply! How do you find that contract ID in first place?

[SiNONiMiTY]

@cyberena

It is stated on the README.md (MasterChef Contract)

[cyberena]

interesting... thank you very much.

On Fri, Mar 5, 2021 at 6:24 AM SiNONiMiTY [email protected] wrote:

@cyberena https://github.com/cyberena

It is stated on the README.md (MasterChef Contract) [image: image] https://user-images.githubusercontent.com/9588282/110128088-5e793180-7e01-11eb-9424-9f2ff8e5c529.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pancakeswap/pancake-farm/issues/12#issuecomment-791450282, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAANN77Y5PPY2VOLB3RGA6TTCDSQDANCNFSM4XPI25XQ .

-- Best regards, Philip

[iamarcel]

There's an important difference between the migrate function on the Binance blog post and this one: the dangerous version of migrate gives the migrator contract infinite spending approval.

Exactly that modification is what e.g., HoneySwap used for their rug pull. This is what happened in that case:

1. Deploy the contract (this is before announcing HoneySwap to the public)
2. Set the migrator to the attacking contract
3. Call migrate for all pairs. Pools are empty so nothing is transferred but infinite approval is given as a side effect.
4. Unset the migrator (to hide the above)
5. Wait
6. The attack: just use the attack contract to transfer the funds (as it has infinite approval from step 3)

That rug pull used a previously granted spending approval which is impossible here: the migrate of PancakeSwap approves a specific balance and immediately uses that full balance, so the migrator contract cannot spend any more of PancakeSwap's LP tokens afterwards.

Also note that migrate is required for a very specific feature: the ability to migrate towards a new version of PancakeSwap.

(Note: I'm a software engineer but not seasoned in smart contracts.)

[ffrappo]

There's an important difference between the migrate function on the Binance blog post and this one: the dangerous version of migrate gives the migrator contract infinite spending approval.

Thanks for taking the time to dive into some detail. Could you elaborate a little on where the actual difference is? The migrate function per-se looks identical.

[tb0b]

Has there been any progress on this issue? Do PancakeSwap still insist it is an essential function?