✅Fei Protocol - ConsenSys

Fei Protocol | ConsenSys DiligenceConsenSys Diligence

Fei Protocol - ConsenSys

Progress

• [C-1] GenesisGroup.commit overwrites previously-committed values

• [C-2] Purchasing and committing still possible after launch

• [H-1] UniswapIncentive overflow on pre-transfer hooks

• [M-1] BondingCurve allows users to acquire FEI before launch

• [M-2] Timed.isTimeEnded returns true if the timer has not been initialized

• [M-3] Overflow/underflow protection

• [M-4] Unchecked return value for IWETH.transfer call

• [M-5] GenesisGroup.emergencyExit remains functional after launch

• [M-6] Unchecked return value for transferFrom calls

Scope

GitHub - fei-protocol/fei-protocol-core at ff892c5d0b9697f249d713bbb2b3bd1da7980ed2GitHub

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, 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

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

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

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

/// @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

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

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

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 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

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.