SushiToken transfers are broken due to wrong delegates accounting on transfers

[code423n4]

Handle

cmichel

Vulnerability details

When minting / transferring / burning tokens, the SushiToken._beforeTokenTransfer function is called and supposed to correctly shift the voting power due to the increase/decrease in tokens for the from and two accounts.
However, it does not correctly do that, it tries to shift the votes from the from account, instead of the _delegates[from] account.
This can lead to transfers reverting.

POC

Imagine the following transactions on the SushiToken contract.
We'll illustrate the corresponding _moveDelegates calls and written checkpoints for each.

• mint(A, 1000) = transfer(0, A, 1000) => _moveDelegates(0, delegates[A]=0) => no checkpoints are written to anyone because delegatees are still zero
• A delegates to A' => _moveDelegates(0, A') => writeCheckpoint(A', 1000)
• B delegates to B' => no checkpoints are written as B has a zero balance
• transfer(A, B, 1000) => _moveDelegates(A, delegates[B] = B') => underflows when subtracting amount=1000 from A's non-existent checkpoint (defaults to 0 votes)

It should subtract from A's delegatee A''s checkpoint instead.

Impact

Users that delegated votes will be unable to transfer any of their tokens.

Recommended Mitigation Steps

In SushiToken._beforeTokenTransfer, change the _moveDelegates call to be from _delegates[from] instead:

function _beforeTokenTransfer(address from, address to, uint256 amount) internal override { 
    _moveDelegates(_delegates[from], _delegates[to], amount);
    super._beforeTokenTransfer(from, to, amount);
}

This is also how the original code from Compound does it.

[maxsam4]

This is a known issue in Sushi token but was kept unchanged in MISO for "preservation of history :)". That was not necessarily a wise choice lol. I think 1 severity should be fine for this as this was an intentional thing. The delegate feature is not supposed to be used in these tokens. We might create a new token type with this bug fixed.

[ghoul-sol]

We have crazy wallets on the blockchain that will call every possible function available to them and that's why I'm keeping this as is. Even intentional, the issue stands so the warden should get credit for it.