Rewards aren't updated before user's balance change in Gauge's withdrawToken

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L548-L561

Vulnerability details

_updateRewardForAllTokens is called in withdraw() only, while both withdraw() and withdrawToken() are public and can be called directly. Reward update is needed on user's balance change, its absence leads to reward data rewriting and rewards losing impact for a user.

Per discussion with project withdrawToken() will not be exposed in UI, so setting the severity to be medium as user's direct usage of the vulnerable function is required here, while the impact is losing of a part of accumulated rewards.

Proof of Concept

withdraw() is a wrapper for withdrawToken(), which changes user's balance. withdrawToken() can be called directly, and in this case there will be no rewards update:

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L548-L561

    function withdraw(uint amount) public {
        _updateRewardForAllTokens();

        uint tokenId = 0;
        if (amount == balanceOf[msg.sender]) {
            tokenId = tokenIds[msg.sender];
        }
        withdrawToken(amount, tokenId);
    }

    function withdrawToken(uint amount, uint tokenId) public lock {
        totalSupply -= amount;
        balanceOf[msg.sender] -= amount;
        _safeTransfer(stake, msg.sender, amount);

This way if a user call withdrawToken(), the associated rewards can be lost due to incomplete reward balance accounting.

References

A showcase of reward update issue:

belbix/solidly#1

It's replicated live with users losing accumulated rewards:

https://github.com/solidlyexchange/solidly/issues/55

Recommended Mitigation Steps

Consider moving rewards update to withdrawToken(), since it is called by withdraw() anyway:

    function withdraw(uint amount) public {
-       _updateRewardForAllTokens();

        uint tokenId = 0;
        if (amount == balanceOf[msg.sender]) {
            tokenId = tokenIds[msg.sender];
        }
        withdrawToken(amount, tokenId);
    }

    function withdrawToken(uint amount, uint tokenId) public lock {
+    	_updateRewardForAllTokens();

        totalSupply -= amount;
        balanceOf[msg.sender] -= amount;
        _safeTransfer(stake, msg.sender, amount);

Or, if the intent is to keep withdrawToken() as is, consider making it private, so no direct usage be possible:

-    function withdrawToken(uint amount, uint tokenId) public lock {
+    function withdrawToken(uint amount, uint tokenId) private lock {

        totalSupply -= amount;
        balanceOf[msg.sender] -= amount;
        _safeTransfer(stake, msg.sender, amount);

[GalloDaSballo]

I'm extremely conflicted about this finding as it's effectively a copy paste that tries to imply that not calling _updateRewardForAllTokens is dangerous, yet no effort is made in showing why.

That said, _updateRewardForAllTokens seems to be reliant on the supplyCheckpoints which would be changed via _writeSupplyCheckpoint. Leading me to agree that the reward math will be wrong.

I want to advise the warden to write a more detailed POC,which shows impact, as leaving that to imagination is not the way to go