xTRIBE contest - rayn's results

Lines of code

https://github.com/fei-protocol/flywheel-v2/blob/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/FlywheelCore.sol#L165

Vulnerability details

Impact

Though setFlywheelRewards has requiresAuth, it still has rug risk that a privileged user can move all rewardToken of flywheelRewards to new (malicious) newFlywheelRewards unconditionally.

Proof of Concept

A malicious user or a compromised admin can call setFlywheelRewards in flywheel-v2/src/FlywheelCore.sol, which can transfer all rewardToken to newFlywheelRewards directly.

Tools Used

vim, ethers.js

Recommended Mitigation Steps

Use timelock on setFlywheelRewards function before transfering all rewardToken to new FlywheelRewards address.

Or separate into two functions with different auth:

1. Use a function to set new FlywheelRewards (but doesn't transfer reward), requireAuth A admin role.
2. Use another function to transfer all rewardToken, requireAuth with B admin role.

Summary

We list 2 low-critical findings and 2 non-critical findings:

• (Low) Doesn’t check whether newFlywheelRewards is valid in setFlywheelRewards()
• (Low) floating pragma
• (Non) Using ecrecover is against best practice
• (Non) It’s better to emit an event in setRewardsStream

(Low) Doesn’t check whether newFlywheelRewards is valid in setFlywheelRewards()

Impact

Doesn’t check whether newFlywheelRewards is valid in setFlywheelRewards(). It may be a invalid or malicious. it should add some kind of check

Proof of Concept

https://github.com/fei-protocol/flywheel-v2/blob/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/FlywheelCore.sol#L165

    function setFlywheelRewards(IFlywheelRewards newFlywheelRewards) external requiresAuth {
        uint256 oldRewardBalance = rewardToken.balanceOf(address(flywheelRewards));
        if (oldRewardBalance > 0) {
            rewardToken.safeTransferFrom(address(flywheelRewards), address(newFlywheelRewards), oldRewardBalance);
        }

        flywheelRewards = newFlywheelRewards;

        emit FlywheelRewardsUpdate(address(newFlywheelRewards));
    }

Tools Used

vim

Recommended Mitigation Steps

Check whether newFlywheelRewards is valid

(Low) floating pragma

Impact

Floating pragma may cause unexpected compilation time behaviour and introduce unintended bugs.

Proof of Concept

Contracts have floating pragma problems.

https://github.com/fei-protocol/xTRIBE/blob/989e47d176facbb0c38bc1e1ca58672f179159e1/src/xTRIBE.sol#L4

https://github.com/fei-protocol/flywheel-v2/blob/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/token/ERC20Gauges.sol#L3

https://github.com/fei-protocol/flywheel-v2/blob/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/token/ERC20MultiVotes.sol#L4

Tools Used

vim

Recommended Mitigation Steps

Don't use ^, lock pragma to ensure compiler version. e.g. pragma solidity 0.8.0;

(Non) Using ecrecover is against best practice

Impact

Using ecrecover is against best practice. Preferably use ECDSA.recover instead. EIP-2 still allows signature malleability for ecrecover(). Remove this possibility and make the signature unique. However it should be impossible to be a threat by now.

Proof of Concept

https://github.com/fei-protocol/flywheel-v2/blob/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/token/ERC20MultiVotes.sol#L380

Tools Used

vim

Recommended Mitigation Steps

Take these implementation into consideration

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4a9cc8b4918ef3736229a5cc5a310bdc17bf759f/contracts/utils/cryptography/draft-EIP712.sol

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/draft-ERC20Permit.sol

(Non) It’s better to emit an event in setRewardsStream

Impact

In the function setRewardsStream in flywheel-v2/src/rewards/FlywheelGaugeRewards.sol, it’s better to emit an event.

Proof of Concept

https://github.com/fei-protocol/flywheel-v2/blob/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/rewards/FlywheelGaugeRewards.sol#L273

Tools Used

vim, ethers.js

Recommended Mitigation Steps

Emit an event in set function:

     function setRewardsStream(IRewardsStream newRewardsStream) external requiresAuth {
         rewardsStream = newRewardsStream;
+        emit RewardsStreamUpdate(address(newRewardsStream));
     }

Save gas in for loops by unchecked arithmetic

The for loop has no overflow risk of i. Use an unchecked block to save gas.

Proof of Concept

flywheel-v2/src/token/ERC20MultiVotes.sol
346:        for (uint256 i = 0; i < size && (userFreeVotes + totalFreed) < votes; i++) {

flywheel-v2/src/rewards/FlywheelGaugeRewards.sol
189:        for (uint256 i = 0; i < size; i++) {

Recommendation

Use unchecked blocks to avoid overflow checks, or use ++i rather than i++ if you don't use unchecked blocks.

for (uint256 i = 0; i < length; ) {
    ...
    unchecked {
        ++i;
    }
}