✅Fei Protocol - ConsenSys
Progress
Scope
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.
The infrastructure assessment focused on the following assets:
The
fei.money
domain, specificallyropsten-app.fei.finance
The
icaruscryptolab
AWS organizationThe
fei-protocol/fei-app
at commit hasheee5d29
[C-1] GenesisGroup.commit
overwrites previously-committed values
GenesisGroup.commit
overwrites previously-committed valuesDescription
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
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
UniswapIncentive
overflow on pre-transfer h ooksDescription
Before a token transfer is performed, Fei
performs some combination of mint/burn operations via UniswapIncentive.incentivize
:
code/contracts/token/UniswapIncentive.sol:L49-L65
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
getBuyIncentive
calculates price deviations after casting amount
to an int256
, which may overflow:
code/contracts/token/UniswapIncentive.sol:L128-L134
Recommendation
Ensure casts in getBuyIncentive
and getSellPenalty
do not overflow.
[M-1] BondingCurve
allows users to acquire FEI before launch
BondingCurve
allows users to acquire FEI before launchDescription
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
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
Timed.isTimeEnded
returns true
if the timer has not been initializedDescription
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
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
IWETH.transfer
callDescription
In EthUniswapPCVController
, there is a call to IWETH.transfer
that does not check the return value:
code/contracts/pcv/EthUniswapPCVController.sol:L122
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
GenesisGroup.emergencyExit
remains functional after launchDescription
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
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
Recommendation
Ensure
launch
cannot be called ifemergencyExit
has been calledEnsure
emergencyExit
cannot be called iflaunch
has been calledIn
emergencyExit
, reducetotalCommittedFGEN
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
code/contracts/genesis/IDO.sol:L58
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
.
Last updated