Thread by @Quantstamp

We are ready to join the ranks of top sushi critics @wyahaw and @ThisIsDavidGelb.

Here is our Security Review for @SushiSwap by top chef @NomiChef 🍣 🐟 🐡 🛡

A thread 👇🏽

What we found during our sushi tasting 🐠
- 2 medium risk issues
- 3 low risk issues
- 5 informational

Finding #1 from MasterChef.sol 👩‍🍳
Severity: Medium

add() does not prevent the same LP token from being added more than once.

If a token is mistakenly added more than once, it would reset the rewards variables associated with the token (e.g., accSushiPerShare).

Recommendation for finding #1 👨‍⚕️

This could be prevented by creating a mapping from addresses to booleans, such that LP tokens get mapped to true once they've been added. The function could then have a require-statement preventing the same LP token from being added twice.

Finding #2 from MasterChef.sol 🍱
Severity: Medium

migrate() is dependent on a currently unspecified migrator contract.

Migrator can be set to any contract, potentially allowing the theft of funds, particularly if the owner's private key is compromised...

The migrator can be set at any time (and multiple times) by the owner. In the worst case, this could be set to a contract that transfers all underlying LP tokens to an arbitrary address.

Note: since the owner is a timelock contract, users would have 2 days to mass exit the pool before the change could occur.

Finding #3 from MasterChef.sol 🍣
Severity: Low

devAddr receives 9% of every SUSHI distribution instead of 10%.

According to the provided docs, "...10% of every SUSHI distribution is set aside for the development & future iterations, including security audit." However…

...according to L195-196, devaddr is receiving 1/11 = 9% of the total sushi distribution.

Finding #4 from SushiToken.sol 🎣
Severity: Low

_moveDelegates() may not behave correctly after token transfers.

Recommendation for finding #4 🌾

It is not clear if this functionality is as intended. If so, no changes are needed, but user documentation should exist describing the scenario above.

If the scenario above is undesirable, _moveDelegates() should be invoked in _transfer() as well. Note however that with this approach, votes can be more easily "bought" by acquiring SUSHI tokens on exchanges.

Finding #5 🍤
Severity: Low

massUpdatePools() may run out-of-gas if too many tokens are added.

There were also 5 informational findings 🍚

See the scope of the Security Review + read the full report here:
github.com/quantstamp/sus…

Security Reviews are different from full audits.

Read our last Security Review Yearn @iearnfinance 💙
quantstamp.com/blog/yearn-fin…