β 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
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
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
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
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
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
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
/// @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
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
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
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
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
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
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
uint amountFei = feiBalance() * totalCommittedFGEN / (totalSupply() + totalCommittedFGEN);
if (amountFei != 0) {
totalCommittedTribe = ido.swapFei(amountFei);
}
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
stakedToken.transferFrom(from, address(this), amount);
code/contracts/genesis/IDO.sol:L58
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
.
Last updated
Was this helpful?