# Fei Protocol - ConsenSys {% embed url="" %} Fei Protocol - ConsenSys {% endembed %} ## Progress * [x] \[C-1] `GenesisGroup.commit` overwrites previously-committed values * [x] \[C-2] Purchasing and committing still possible after launch * [x] \[H-1] `UniswapIncentive` overflow on pre-transfer hooks * [x] \[M-1] `BondingCurve` allows users to acquire FEI before launch * [x] \[M-2] `Timed.isTimeEnded` returns `true` if the timer has not been initialized * [x] \[M-3] Overflow/underflow protection * [x] \[M-4] Unchecked return value for `IWETH.transfer` call * [x] \[M-5] `GenesisGroup.emergencyExit` remains functional after launch * [x] \[M-6] Unchecked return value for transferFrom calls ## Scope {% embed url="" %} Our review focused on the commit hash `ff892c5d`. The list of files in scope and the priorities of the audit are defined by the client and can be found [here](https://github.com/fei-protocol/fei-protocol-core/wiki/ConsenSys-Audit-Info). The infrastructure assessment focused on the following assets: * The `fei.money` domain, specifically `ropsten-app.fei.finance` * The `icaruscryptolab` AWS organization * The `fei-protocol/fei-app` at commit hash `eee5d29` ## \[C-1] `GenesisGroup.commit` overwrites previously-committed values #### **Description** `commit` allows anyone to commit purchased FGEN to a swap that will occur once the genesis group is launched. This commitment may be performed on behalf of other users, as long as the calling account has sufficient allowance: **code/contracts/genesis/GenesisGroup.sol:L87-L94** ```solidity function commit(address from, address to, uint amount) external override onlyGenesisPeriod { burnFrom(from, amount); committedFGEN[to] = amount; totalCommittedFGEN += amount; emit Commit(from, to, amount); } ``` The `amount` stored in the recipient’s `committedFGEN` balance overwrites any previously-committed value. Additionally, this also allows anyone to commit an amount of “0” to any account, deleting their commitment entirely. #### **Recommendation** Ensure the committed amount is added to the existing commitment. ## \[C-2] Purchasing and committing still possible after launch #### **Description** Even after `GenesisGroup.launch` has successfully been executed, it is still possible to invoke `GenesisGroup.purchase` and `GenesisGroup.commit`. #### **Recommendation** Consider adding validation in `GenesisGroup.purchase` and `GenesisGroup.commit` to make sure that these functions cannot be called after the launch. ## \[H-1] `UniswapIncentive` overflow on pre-transfer h ooks #### **Description** Before a token transfer is performed, `Fei` performs some combination of mint/burn operations via `UniswapIncentive.incentivize`: **code/contracts/token/UniswapIncentive.sol:L49-L65** ```solidity function incentivize( address sender, address receiver, address operator, uint amountIn ) external override onlyFei { updateOracle(); if (isPair(sender)) { incentivizeBuy(receiver, amountIn); } if (isPair(receiver)) { require(isSellAllowlisted(sender) || isSellAllowlisted(operator), "UniswapIncentive: Blocked Fei sender or operator"); incentivizeSell(sender, amountIn); } } ``` Both `incentivizeBuy` and `incentivizeSell` calculate buy/sell incentives using overflow-prone math, then mint / burn from the target according to the results. This may have unintended consequences, like allowing a caller to mint tokens before transferring them, or burn tokens from their recipient. #### **Examples** `incentivizeBuy` calls `getBuyIncentive` to calculate the final minted value: **code/contracts/token/UniswapIncentive.sol:L173-L186** ```solidity function incentivizeBuy(address target, uint amountIn) internal ifMinterSelf { if (isExemptAddress(target)) { return; } (uint incentive, uint32 weight, Decimal.D256 memory initialDeviation, Decimal.D256 memory finalDeviation) = getBuyIncentive(amountIn); updateTimeWeight(initialDeviation, finalDeviation, weight); if (incentive != 0) { fei().mint(target, incentive); } } ``` `getBuyIncentive` calculates price deviations after casting `amount` to an `int256`, which may overflow: **code/contracts/token/UniswapIncentive.sol:L128-L134** ```solidity function getBuyIncentive(uint amount) public view override returns( uint incentive, uint32 weight, Decimal.D256 memory initialDeviation, Decimal.D256 memory finalDeviation ) { (initialDeviation, finalDeviation) = getPriceDeviations(-1 * int256(amount)); ``` #### **Recommendation** Ensure casts in `getBuyIncentive` and `getSellPenalty` do not overflow. ## \[M-1] `BondingCurve` allows users to acquire FEI before launch #### **Description** `BondingCurve.allocate` allocates the protocol’s held PCV, then calls `_incentivize`, which rewards the caller with FEI if a certain amount of time has passed: **code-update/contracts/bondingcurve/BondingCurve.sol:L180-L186** ```solidity /// @notice if window has passed, reward caller and reset window function _incentivize() internal virtual { if (isTimeEnded()) { _initTimed(); // reset window fei().mint(msg.sender, incentiveAmount); } } ``` `allocate` can be called before genesis launch, as long as the contract holds some nonzero PCV. By force-sending the contract 1 wei, anyone can bypass the majority of checks and actions in `allocate`, and mint themselves FEI each time the timer expires. #### **Recommendation** Prevent `allocate` from being called before genesis launch. ## \[M-2] `Timed.isTimeEnded` returns `true` if the timer has not been initialized #### **Description** `Timed` initialization is a 2-step process: * `Timed.duration` is set in the constructor. * `Timed.startTime` is set when the method `_initTimed` is called. Before this second method is called, `isTimeEnded()` calculates remaining time using a `startTime` of 0, resulting in the method returning `true` for most values, even though the timer has not technically been started. #### **Recommendation** If `Timed` has not been initialized, `isTimeEnded()` should return `false`, or `revert` ## \[M-3] Overflow/underflow protection #### **Description** Having overflow/underflow vulnerabilities is very common for smart contracts. It is usually mitigated by using `SafeMath` or using solidity version ^0.8 (after solidity 0.8 arithmetical operations already have default overflow/underflow protection). In this code, many arithmetical operations are used without the ‘safe’ version. The reasoning behind it is that all the values are derived from the actual ETH values, so they can’t overflow. On the other hand, some operations can’t be checked for overflow/underflow without going much deeper into the codebase that is out of scope: **code/contracts/genesis/GenesisGroup.sol:L131** ```solidity uint totalGenesisTribe = tribeBalance() - totalCommittedTribe; ``` #### **Recommendation** In our opinion, it is still safer to have these operations in a safe mode. So we recommend using `SafeMath` or solidity version ^0.8 compiler. ## \[M-4] Unchecked return value for `IWETH.transfer` call #### **Description** In `EthUniswapPCVController`, there is a call to `IWETH.transfer` that does not check the return value: **code/contracts/pcv/EthUniswapPCVController.sol:L122** ```solidity weth.transfer(address(pair), amount); ``` It is usually good to add a require-statement that checks the return value or to use something like `safeTransfer`; unless one is sure the given token reverts in case of a failure. #### **Recommendation** Consider adding a require-statement or using `safeTransfer`. ## \[M-5] `GenesisGroup.emergencyExit` remains functional after launch #### **Description** `emergencyExit` is intended as an escape mechanism for users in the event the genesis `launch` method fails or is frozen. `emergencyExit` becomes callable 3 days after `launch` is callable. These two methods are intended to be mutually-exclusive, but are not: either method remains callable after a successful call to the other. This may result in accounting edge cases. In particular, `emergencyExit` fails to decrease `totalCommittedFGEN` by the exiting user’s commitment: **code/contracts/genesis/GenesisGroup.sol:L185-L188** ```solidity burnFrom(from, amountFGEN); committedFGEN[from] = 0; payable(to).transfer(total); ``` As a result, calling launch after a user performs an exit will incorrectly calculate the amount of FEI to swap: **code/contracts/genesis/GenesisGroup.sol:L165-L168** ```solidity uint amountFei = feiBalance() * totalCommittedFGEN / (totalSupply() + totalCommittedFGEN); if (amountFei != 0) { totalCommittedTribe = ido.swapFei(amountFei); } ``` #### **Recommendation** * Ensure `launch` cannot be called if `emergencyExit` has been called * Ensure `emergencyExit` cannot be called if `launch` has been called * In `emergencyExit`, reduce `totalCommittedFGEN` by the exiting user’s committed amount ## \[M-6] Unchecked return value for transferFrom calls #### **Description** There are two `transferFrom` calls that do not check the return value (some tokens signal failure by returning false): **code/contracts/pool/Pool.sol:L121** ```solidity stakedToken.transferFrom(from, address(this), amount); ``` **code/contracts/genesis/IDO.sol:L58** ```solidity fei().transferFrom(msg.sender, address(pair), amountFei); ``` It is usually good to add a require-statement that checks the return value or to use something like `safeTransferFrom`; unless one is sure the given token reverts in case of a failure. #### **Recommendation** Consider adding a require-statement or using `safeTransferFrom`.