Velodrome Finance contest - hansfriese's results

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L526 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L844-L868 https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L1084-L1099

Vulnerability details

Impact

withdraw() and merge() functions of VotingEscrow won't work when an approved user(not owner) calls.

Proof of Concept

withdraw() and merge() functions call _burn() function inside and _burn() function calls _removeTokenFrom() using msg.sender. But _removeTokenFrom() requires _from must be owner of token, so it will be reverted when msg.sender is not owner, but approved user.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

You need to use owner instead of msg.sender at #L526.

_removeTokenFrom(owner, _tokenId);

Summary

I've found some low risk issues including wrong comments and I recommend to use same PRECISION for fee denominator. And I see many external functions don't have dev comments and natspecs and it would be good to add for good understandings.

Low Risk Issues

1. Wrong comments contracts\Minter.sol#L30 0.05% => 5%

contracts\Minter.sol#L41 0.03% => 3%

contracts\Minter.sol#L85 2% => 99%

contracts\Pair.sol#L17 You wrote "not immutable" for stable

contracts\VeloGovernor.sol#L21 0.02% => 0.2%

2. It would be good to add one more require() for _fantomSender. contracts\redeem\RedemptionReceiver.sol#L44 If _fantomSender is zero address, then it will be considered as non-initialized even after successful deposit. You need to insert below code at #L45.

require(_fantomSender != address(0), "ZERO ADDRESS");

3. rewards array of Gauge.sol would have duplicate tokens inside. contracts\Gauge.sol#L104-L119

Impact You add 3 tokens without checking isReward[_token] == false, and I think it's possible _token = _token0 or _token1. In this case, rewards array will have duplicate tokens and you can add at most 15 tokens(original limit = 16) unless you remove the duplicate token using swapOutRewardToken() function manually. And when you call addRewardToken() of Bribe.sol at #L106, #L113, #L116, Bribe contract will filter tokens properly so the length of rewards array of Gauge.sol and Bribe.sol will be different.

Recommended Mitigation Steps You need to check isReward is false before you add _token0, _token1.

Non-critical Issues

1. Event should use indexed fields if there are three or more fields contracts\RewardsDistributor.sol#L23

2. Needless uint casting contracts\Voter.sol#L118 contracts\Voter.sol#L168 contracts\Voter.sol#L169

3. require()/revert() statements should have descriptive reason strings There are more than 30 cases and I will show one. contracts\Bribe.sol#L24

1. Needless condition because you checked same condition already contracts\Voter.sol#L111

2. Double declaration for same value contracts\Voter.sol#L140-L141 _totalWeight and _usedWeight save same value and you can declare and use only one of them.

3. Non-strict inequalities are cheaper than strict ones contracts\Router.sol#L41 contracts\factories\PairFactory.sol#L90

4. Check require() at the beginning of function contracts\VotingEscrow.sol#L772 contracts\VotingEscrow.sol#L785 contracts\VotingEscrow.sol#L819 contracts\VotingEscrow.sol#L834 => to #L832 before unlock_time declaration contracts\VotingEscrow.sol#L835 => to #L833 before unlock_time declaration

5. Declare memory variable instead of calling routes[i] several times contracts\Router.sol#L91-L94

6. Change storage to memory if possible contracts\Voter.sol#L99

7. Don't use storage value if possible contracts\Bribe.sol#L87 => "msg.sender" instead of "address(gauge)" contracts\Gauge.sol#L104 => "_bribe" instead of "bribe" contracts\Gauge.sol#L105 => "__ve" instead of "_ve" contracts\Gauge.sol#L106 => "_bribe" instead of "bribe" contracts\Gauge.sol#L111 => "_isForPair" instead of "isForPair" contracts\Gauge.sol#L112 => "_stake" instead of "stake" contracts\Gauge.sol#L113 => "_bribe" instead of "bribe" contracts\Gauge.sol#L116 => "_bribe" instead of "bribe" contracts\Velo.sol#L76 => "msg.sender" instead of "redemptionReceiver" contracts\VotingEscrow.sol#L246 => "owner" instead of "idToOwner[_tokenId]"

8. Use != 0 instead of > 0 for uint variables There are more than 30 cases and I will show one. contracts\Bribe.sol#L42

9. Use ++i instead of i++, i+=1 There are more than 30 cases and I will show one. contracts\Gauge.sol#L179

10. No need to initialize variables with default values There are more than 30 cases and I will show one. contracts\Gauge.sol#L481

11. An array’s length should be cached to save gas in for-loops contracts\Gauge.sol#L353 contracts\Minter.sol#L57 contracts\Pair.sol#L257 contracts\RewardsDistributor.sol#L301 contracts\Router.sol#L90 contracts\Router.sol#L316 contracts\Voter.sol#L76 contracts\Voter.sol#L266 contracts\Voter.sol#L304 contracts\Voter.sol#L310 contracts\Voter.sol#L346 contracts\VotingEscrow.sol#L1146 contracts\VotingEscrow.sol#L1193 contracts\VotingEscrow.sol#L1225 contracts\VotingEscrow.sol#L1249 contracts\VotingEscrow.sol#L1295 contracts\VotingEscrow.sol#L1320