code-423n4 / 2022-04-xtribe-findings

2 stars 0 forks source link

QA Report #27

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Low Risk Issues

1. Nonce used for multiple purposes

The nonce mapping used for permit() calls is the same as the one used for delegateBySig(). This should at the very least be documented so signers know that the order of operations between the two functions matters, and so that multicall()s can be organized appropriately

File: lib/flywheel-v2/src/token/ERC20MultiVotes.sol   #1

392        require(nonce == nonces[signer]++, "ERC20MultiVotes: invalid nonce");

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

2. multicall()s involving permit() and delegateBySig() can be DOSed

Attackers monitoring the blockchain for multicalls can front-run by calling permit() and delegateBySig() before the multicall(), causing it to revert. Have separate flavors of the functions where the multicall() data is included in the hash

File: lib/flywheel-v2/src/token/ERC20MultiVotes.sol   #1

392        require(nonce == nonces[signer]++, "ERC20MultiVotes: invalid nonce");

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

3. Misleading comments

File: lib/flywheel-v2/src/FlywheelCore.sol   #1

82      @return the cumulative amount of rewards accrued to user (including prior)

the cumulative amount of rewards accrued to the user since the last claim https://github.com/fei-protocol/flywheel-v2/blob/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/FlywheelCore.sol#L82

4. require() should be used instead of assert()

File: lib/flywheel-v2/src/rewards/FlywheelGaugeRewards.sol   #1

196             assert(queuedRewards.storedCycle == 0 || queuedRewards.storedCycle >= lastCycle);

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

File: lib/flywheel-v2/src/rewards/FlywheelGaugeRewards.sol   #2

235         assert(queuedRewards.storedCycle >= cycle);

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

Non-critical Issues

1. require()/revert() statements should have descriptive reason strings

File: lib/flywheel-v2/src/rewards/FlywheelGaugeRewards.sol   #1

114         require(rewardToken.balanceOf(address(this)) - balanceBefore >= totalQueuedForCycle);

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

File: lib/flywheel-v2/src/rewards/FlywheelGaugeRewards.sol   #2

153             require(rewardToken.balanceOf(address(this)) - balanceBefore >= newRewards);

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

File: lib/flywheel-v2/src/rewards/FlywheelGaugeRewards.sol   #3

154             require(newRewards <= type(uint112).max); // safe cast

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

File: lib/flywheel-v2/src/rewards/FlywheelGaugeRewards.sol   #4

195             require(queuedRewards.storedCycle < currentCycle);

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

File: lib/flywheel-v2/src/rewards/FlywheelGaugeRewards.sol   #5

200             require(nextRewards <= type(uint112).max); // safe cast

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

File: lib/flywheel-v2/src/token/ERC20Gauges.sol   #6

345             require(_userGauges[user].remove(gauge));

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

File: lib/flywheel-v2/src/token/ERC20MultiVotes.sol   #7

266             require(_delegates[delegator].remove(delegatee));

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

File: lib/flywheel-v2/src/token/ERC20MultiVotes.sol   #8

352                 require(_delegates[user].remove(delegatee)); // Remove from set. Should never fail.

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

File: lib/flywheel-v2/src/token/ERC20MultiVotes.sol   #9

393         require(signer != address(0));

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

2. public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

File: lib/flywheel-v2/src/FlywheelCore.sol   #1

84     function accrue(ERC20 strategy, address user) public returns (uint256) {

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

File: lib/flywheel-v2/src/FlywheelCore.sol   #2

101     function accrue(
102         ERC20 strategy,
103         address user,
104         address secondUser
105     ) public returns (uint256, uint256) {

https://github.com/fei-protocol/flywheel-v2/tree/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/FlywheelCore.sol#L101-L105

3. Use a more recent version of solidity

Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>) Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)

File: lib/flywheel-v2/src/token/ERC20MultiVotes.sol   #1

4 pragma solidity ^0.8.0;

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

4. Constant redefined elsewhere

Consider defining in only one contract so that values cannot become out of sync when only one location is updated. If the variable is a local cache of another contract's value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don't get out of sync

File: lib/flywheel-v2/src/token/ERC20Gauges.sol   #1

47     uint32 public immutable gaugeCycleLength;

seen in lib/flywheel-v2/src/rewards/FlywheelGaugeRewards.sol https://github.com/fei-protocol/flywheel-v2/tree/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/token/ERC20Gauges.sol#L47

5. Non-library/interface files should use fixed compiler versions, not floating ones

File: lib/xTRIBE/src/xTRIBE.sol   #1

4 pragma solidity ^0.8.0;

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

6. Typos

File: lib/flywheel-v2/src/FlywheelCore.sol   #1

17          The Core contract maintaings three important pieces of state:

maintaings https://github.com/fei-protocol/flywheel-v2/tree/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/FlywheelCore.sol#L17

File: lib/flywheel-v2/src/FlywheelCore.sol   #2

262         // accumulate rewards by multiplying user tokens by rewardsPerToken index and adding on unclaimed

rewardsPerToken https://github.com/fei-protocol/flywheel-v2/tree/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/FlywheelCore.sol#L262

File: lib/flywheel-v2/src/token/ERC20Gauges.sol   #3

230     /// @notice thrown when incremending during the freeze window.

incremending https://github.com/fei-protocol/flywheel-v2/tree/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/token/ERC20Gauges.sol#L230

File: lib/flywheel-v2/src/token/ERC20MultiVotes.sol   #4

143     /// @notice An event thats emitted when an account changes its delegate

thats https://github.com/fei-protocol/flywheel-v2/tree/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/token/ERC20MultiVotes.sol#L143

File: lib/flywheel-v2/src/token/ERC20MultiVotes.sol   #5

189      * @param delegatee the receivier of votes.

receivier https://github.com/fei-protocol/flywheel-v2/tree/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/token/ERC20MultiVotes.sol#L189

File: lib/flywheel-v2/src/token/ERC20MultiVotes.sol   #6

364    /*///////////////////////////////////////////////////////////////
365                             EIP-712 LOGIC
366    //////////////////////////////////////////////////////////////*/

did you mean EIP-2612? https://github.com/fei-protocol/flywheel-v2/blob/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/token/ERC20MultiVotes.sol#L364-L366

7. NatSpec is incomplete

File: lib/flywheel-v2/src/FlywheelCore.sol   #1

93     /** 
94       @notice accrue rewards for a two users on a strategy
95       @param strategy the strategy to accrue a user's rewards on
96       @param user the first user to be accrued
97       @param user the second user to be accrued
98       @return the cumulative amount of rewards accrued to the first user (including prior)
99       @return the cumulative amount of rewards accrued to the second user (including prior)
100     */
101     function accrue(
102         ERC20 strategy,
103         address user,
104         address secondUser
105     ) public returns (uint256, uint256) {

Missing: @param secondUser https://github.com/fei-protocol/flywheel-v2/tree/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/FlywheelCore.sol#L93-L105

File: lib/flywheel-v2/src/token/ERC20Gauges.sol   #2

130       @param num the number of gauges to return
131     */
132     function gauges(uint256 offset, uint256 num) external view returns (address[] memory values) {

Missing: @return https://github.com/fei-protocol/flywheel-v2/tree/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/token/ERC20Gauges.sol#L130-L132

File: lib/flywheel-v2/src/token/ERC20Gauges.sol   #3

176       @param num the number of gauges to return.
177     */
178     function userGauges(
179         address user,
180         uint256 offset,
181         uint256 num
182     ) external view returns (address[] memory values) {

Missing: @return https://github.com/fei-protocol/flywheel-v2/tree/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/token/ERC20Gauges.sol#L176-L182

8. Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

File: lib/flywheel-v2/src/FlywheelCore.sol   #1

66     event AccrueRewards(ERC20 indexed strategy, address indexed user, uint256 rewardsDelta, uint256 rewardsIndex);

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

File: lib/flywheel-v2/src/FlywheelCore.sol   #2

73     event ClaimRewards(address indexed user, uint256 amount);

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

File: lib/flywheel-v2/src/rewards/FlywheelGaugeRewards.sol   #3

44     event CycleStart(uint32 indexed cycleStart, uint256 rewardAmount);

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

File: lib/flywheel-v2/src/rewards/FlywheelGaugeRewards.sol   #4

47     event QueueRewards(address indexed gauge, uint32 indexed cycleStart, uint256 rewardAmount);

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

File: lib/flywheel-v2/src/token/ERC20Gauges.sol   #5

234     event IncrementGaugeWeight(address indexed user, address indexed gauge, uint256 weight, uint32 cycleEnd);

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

File: lib/flywheel-v2/src/token/ERC20Gauges.sol   #6

237     event DecrementGaugeWeight(address indexed user, address indexed gauge, uint256 weight, uint32 cycleEnd);

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

File: lib/flywheel-v2/src/token/ERC20Gauges.sol   #7

440     event MaxGaugesUpdate(uint256 oldMaxGauges, uint256 newMaxGauges);

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

File: lib/flywheel-v2/src/token/ERC20Gauges.sol   #8

443     event CanContractExceedMaxGaugesUpdate(address indexed account, bool canContractExceedMaxGauges);

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

File: lib/flywheel-v2/src/token/ERC20MultiVotes.sol   #9

102     event MaxDelegatesUpdate(uint256 oldMaxDelegates, uint256 newMaxDelegates);

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

File: lib/flywheel-v2/src/token/ERC20MultiVotes.sol   #10

105     event CanContractExceedMaxDelegatesUpdate(address indexed account, bool canContractExceedMaxDelegates);

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

File: lib/flywheel-v2/src/token/ERC20MultiVotes.sol   #11

135     event Delegation(address indexed delegator, address indexed delegate, uint256 amount);

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

File: lib/flywheel-v2/src/token/ERC20MultiVotes.sol   #12

138     event Undelegation(address indexed delegator, address indexed delegate, uint256 amount);

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

File: lib/flywheel-v2/src/token/ERC20MultiVotes.sol   #13

141     event DelegateVotesChanged(address indexed delegate, uint256 previousBalance, uint256 newBalance);

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

9. Consider addings checks for signature malleability

File: lib/flywheel-v2/src/token/ERC20MultiVotes.sol   #1

380        address signer = ecrecover(
381            keccak256(
382                abi.encodePacked(
383                    "\x19\x01",
384                    DOMAIN_SEPARATOR(),
385                    keccak256(abi.encode(DELEGATION_TYPEHASH, delegatee, nonce, expiry))
386                )
387            ),
388            v,
389            r,
390            s
391        );
392        require(nonce == nonces[signer]++, "ERC20MultiVotes: invalid nonce");
393        require(signer != address(0));
394        _delegate(signer, delegatee);

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

0xean commented 2 years ago

The severities listed in this QA report are correct as-is