Sushiswap smart-contract bug and quality of audits in community

As we have promised in our earlier article, we want to share with the community a bug.

Dracula Protocol still lacks official audits of its code.

However, today’s post is not about us, but about audits and their role in the DeFi community as a whole. During the development, our team have noticed the Sushiswap governance issue in $SUSHI simultaneously with John Seok that was overlooked by auditors; one may check, that $DRC has this issue fixed. As at that moment, Sushiswap doesn’t have governance implemented; it’s not that relevant. When we’d started developing our protocol and have Sushiswap MasterChef smart-contract investigated as a potential skeleton for our MasterVampire contract, we’ve noticed another bug, this time with more serious ramifications.

We want to describe the vulnerability and a potential backdoor that was left by mistake, and forked multiple times in different projects. The bug itself may lead users to have their rewards partially lost or unjustifiedly multiplied. A potential attack based on this is hard to implement, and it takes special preparations, plus owner rights on the initial stage of the launch. But it gives the ability to mint a significant share of the already circulating supply amount of token for anyone with admin rights. Even under multisig and governance, it’s extremely hard to notice anything malicious in common transactions.

As far as we know, this issue is implemented in every Sushiswap clone. We’ll illustrate the issue on the example of Sushiswap MasterChef contract, as it was audited several times and got the most attention.

MasterChef contract has massUpdatePools functions, that iterates over all pools and calls updatePool function. This function updates given pool and mints a reward for all blocks previously unrewarded. massUpdatePools is referenced and called only in add and set functions. In both of them, calling massUpdatePools is under an optional flag. updatePoolis called in deposit and withdraw.

massUpdatePools is called only if _withUpdate flag is set

Let’s understand why _withUpdate being an optional flag is irresponsible and possibly malicious.

Lost rewards

Imagine that there is a pretty small pool on Sushiswap in terms of not that many stakeholders there. With rare deposits and withdrawals, as a result, updatePool is also called rarely. Then for some reason rewards are reworked, and this pool has it’s share decreased without _withUpdate flag and a call to updatePool. It might be more or less obvious that reward is decreased when set is called, and way less obvious when new pools added with new allocation points.

One doesn’t need to decrease the allocation points of our imaginary small pool manually, but rather add new pools with more allocation points effectively diluting our pool share. Without a prior call to updatePool users of our imaginary pool will experience that their rewards for all blocks between lastRewardBlock and the one in which new pools have been added will be calculated not in accordance with the old reward plan, but with the new one, and with their new allocation points they will receive fewer rewards. This is equivalent for retrospectively burning some of users rewards.

Here are some transactions, calling both add and set, that Sushiswap called without _withUpdate flag. The amount of lost and unfairly gained Sushis by users is undefined.

Unfounded minting

However, it also may work differently: some people may receive much more than they deserved.

Consider a malicious owner of the contract on the initial stage of production, silently adds few pools with 0 allocation points and deposits there some tokens. This doesn’t look suspicious at all, because allocation points seemingly set to 0, and no one incentivized to stake at those pools.

But let’s imagine what will happen if after, say, 10 days from the start bunch of new transactions will come with some new pools and their reward updates. Barely anyone will notice that some pools with prior zero allocation points start to have even 10% of the allocation, as people tend to believe that no one is able to mint more than 100/1000 Sushi per block.

As a result, update transaction executed without _withUpdate flag will lead to those pools (with dev funds staking from block one) receiving total mint of 10% * 10 days = ~10% of total circulating supply at the moment. And ~640,000 Sushi minted in one block.

Audits

Contracts with this issue were reviewed many times.

By Peckshield team auditing Sushiswap. By Quantstump auditing Sushiswap. By Slowmist team auditing Moonswap. By Tomochain and Arcadia Group for Luaswap.

We believe that the severity of this finding is at least on a high level. The fact that it was overlooked by auditors raises some questions about the role of audit in DeFi.

The fix for this is pretty simple, and it’s implemented in Dracula Protocol. One simply needs to hardcode call to updatePool in set function without any “flags”.

Nevertheless, we still lack an official audit for our project and invite anyone to check our code.