search

code-423n4 / 2022-06-infinity-findings

4 stars 0 forks source link

QA Report #299

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

QA Report

Table of Contents

summary

Few vulnerabilities were found examining the contracts. The main concerns are with:

Comment Missing function parameter

PROBLEM

Some of the function comments are missing function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

bool verifySellOrder\ uint256 execPrice\ address seller,address buyer,OrderTypes.OrderItem[] calldata constructedNfts,address currency,uint256 amount\ address seller,address buyer,uint256 sellNonce,uint256 buyNonce,OrderTypes.OrderItem[] calldata constructedNfts,address currency,uint256 amount\ OrderTypes.MakerOrder calldata order\ OrderTypes.MakerOrder calldata order\ address destination,address currency,uint256 amount\ address destination

InfinityOrderBookComplication.sol

OrderTypes.MakerOrder calldata sell, OrderTypes.MakerOrder calldata buy\ OrderTypes.MakerOrder calldata sell, OrderTypes.MakerOrder calldata buy\ OrderTypes.MakerOrder calldata sell,OrderTypes.MakerOrder calldata buy,OrderTypes.OrderItem[] calldata constructedNfts\ OrderTypes.MakerOrder calldata makerOrder, OrderTypes.OrderItem[] calldata takerItems\ OrderTypes.MakerOrder[] calldata orders\ OrderTypes.MakerOrder calldata order

InfinityStaker.sol

address user,uint256 amount,uint256 noVesting,uint256 vestedThreeMonths,uint256 vestedSixMonths,uint256 vestedTwelveMonths\ address user\ address destination\ StakeLevel stakeLevel, uint16 threshold\ uint16 threeMonthPenalty,uint16 sixMonthPenalty,uint16 twelveMonthPenalty\ address _infinityTreasury

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for these parameters

Commented code

PROBLEM

There are portions of commented code in some files.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

line 1169\ line1186\ line1202

TOOLS USED

Manual Analysis

MITIGATION

Remove commented code

Constants instead of magic numbers

PROBLEM

It is best practice to use constant variables rather than literal values to make the code easier to understand and maintain.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

20000\ 1000000\ 10000\ 10000\ 10000\ 10000\ 10**4

InfinityOrderBookComplication.sol

10**4

InfinityStaker.sol

10**18

TOOLS USED

Manual Analysis

MITIGATION

Define constant variables for the literal values aforementioned.

Events indexing

PROBLEM

Events should use indexed fields

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

event CancelAllOrders(address user, uint256 newMinNonce)\ event CancelMultipleOrders(address user, uint256[] orderNonces)\ event NewWethTransferGasUnits(uint32 wethTransferGasUnits))\ event NewProtocolFee(uint16 protocolFee)\ event MatchOrderFulfilled(bytes32 sellOrderHash,bytes32 buyOrderHash,address seller,address buyer,address complication, // address of the complication that defines the execution ,address currency, // token address of the transacting currency,uint256 amount // amount spent on the order)\ event TakeOrderFulfilled(bytes32 orderHash,address seller,address buyer,address complication, // address of the complication that defines the executionaddress currency, // token address of the transacting currencyuint256 amount // amount spent on the order)

InfinityStaker.sol

event Staked(address indexed user, uint256 amount, Duration duration)\ event DurationChanged(address indexed user, uint256 amount, Duration oldDuration, Duration newDuration)\ event UnStaked(address indexed user, uint256 amount)\ event RageQuit(address indexed user, uint256 totalToUser, uint256 penalty)

InfinityToken.sol

event EpochAdvanced(uint256 currentEpoch, uint256 supplyMinted)

TOOLS USED

Manual Analysis

MITIGATION

Add indexed fields to these events so that they have the maximum number of indexed fields possible.

Event should be emitted in setters

PROBLEM

Setters should emit an event so that Dapps can detect important changes to storage

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

All the functions editing the currencies and complications of the exchange should emit an event\ function addCurrency\ function addComplication\ function removeCurrency\ function removeComplication

function updateMatchExecutor

InfinityStaker.sol

function updateStakeLevelThreshold\ function updatePenalties\ function updateInfinityTreasury

TOOLS USED

Manual Analysis

MITIGATION

Emit an event in all setters.

Function missing comments

PROBLEM

Some functions are missing comments.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

function _emitMatchEvent\ function _emitTakerEvent\ function _nftsHash\ function _tokensHash

InfinityStaker.sol

function _getDurationInSeconds

InfinityToken.sol

function advanceEpoch()\ function _beforeTokenTransfer()\ function _afterTokenTransfer()\ function _mint()\ function _burn()\ function getAdmin()\ function getTimelock()\ function getInflation()\ function getCliff()\ function getMaxEpochs()\ function getEpochDuration()

TOOLS USED

Manual Analysis

MITIGATION

Add comments to these functions

Public functions can be external

PROBLEM

It is good practice to mark functions as external instead of public if they are not called by the contract where they are defined.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

InfinityStaker.sol

function getUserTotalStaked()\ function getUserTotalVested()

TOOLS USED

Manual Analysis

MITIGATION

Declare these functions as external instead of public

Related data should be grouped in struct

PROBLEM

When there are mappings that use the same key value, having separate fields is error prone, for instance in case of deletion or with future new fields.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

mapping(address => uint256) public userMinOrderNonce;
mapping(address => mapping(uint256 => bool)) public isUserOrderNonceExecutedOrCancelled;

TOOLS USED

Manual Analysis

MITIGATION

Group the related data in a struct and use one mapping:

struct OrderNonce {
  uint256 userMin;
  mapping(uint256 => bool) isExecutedOrCancelled;
}

And it would be used as a state variable:

mapping(address =>  OrderNonce) orderNonces;

Scientific notation

PROBLEM

For readability, it is best to use scientific notation (e.g 10e5) rather than decimal literals(100000) or exponentiation(10**5)

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

uint32 public WETH_TRANSFER_GAS_UNITS = 50000\ 20000\ 1000000\ 10000\ 10000\ 10000\ 10000\ 10**4

InfinityOrderBookComplication.sol

10**4

InfinityStaker.sol

uint16 public BRONZE_STAKE_THRESHOLD = 1000\ uint16 public SILVER_STAKE_THRESHOLD = 5000\ uint16 public GOLD_STAKE_THRESHOLD = 10000\ uint16 public PLATINUM_STAKE_THRESHOLD = 20000\ 10**18

TOOLS USED

Manual Analysis

MITIGATION

Replace the numbers aforementioned with their scientific notation

Check zero denominator

PROBLEM

When a division is computed, it must be ensured that the denominator is non-zero to prevent failure of the function call.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

InfinityStaker.sol

((threeMonthLock - threeMonthVested) / THREE_MONTH_PENALTY)\ ((sixMonthLock - sixMonthVested) / SIX_MONTH_PENALTY)\ ((twelveMonthLock - twelveMonthVested) / TWELVE_MONTH_PENALTY)

All these storage variables in the denominators are set by the owner in updatePenalties(), and can be 0 as there is no non-zero check.

TOOLS USED

Manual Analysis

MITIGATION

Before doing these computations, add a non-zero check to these variables. Or alternatively, add a non-zero check in updatePenalties().

Immutable addresses lack zero-address check

IMPACT

constructors should check the address written in an immutable address variable is not the zero address

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

WETH = _WETH

TOOLS USED

Manual Analysis

MITIGATION

Add a zero address check for _WETH.

Payable functions when using ERC20

PROBLEM

There should be a require(0 == msg.value) to ensure no Ether is being sent to the exchange when the currency used in an order is a ERC20 token.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

scope: takeMultipleOneOrders

scope: takeOrders

TOOLS USED

Manual Analysis

MITIGATION

Add require(0 == msg.value) in both condition blocks mentioned above.

Receive function

PROBLEM

InfinityStaker.sol is not supposed to receive ETH. Instead of using a rescue function, remove receive() and fallback() altogether.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

InfinityStaker.sol

fallback() external payable\ receive() external payable\ function rescueETH(address destination) external payable

TOOLS USED

Manual Analysis

MITIGATION

Remove these functions, or include a call to rescueETH in receive(), so that a user that mistakenly sends ETH to the Staker retrieves it immediately.

Setters should check the input value

PROBLEM

Setters should check the input value - ie make revert if it is the zero address or zero

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

function updateMatchExecutor

InfinityStaker.sol

function updateStakeLevelThreshold There should be a check that the new threshold does not break the following: BRONZE < SILVER < GOLD < PLATINUM\ function updatePenalties\ function updateInfinityTreasury

TOOLS USED

Manual Analysis

MITIGATION

Add non-zero checks - address or uint - to the setters aforementioned.

Timelock or maximum amount on updatePenalties

PROBLEM

updatePenalties() changes how much a user loses upon "ragequiting" - ie withdrawing their tokens from the staker without waiting for the vesting period. It currently does not have any timelock, or any maximum amount: the owner can set the penalties such that a user calling rageQuit() loses all their Infinity tokens (all would be transferred to INFINITY_TREASURY). Adding a timelock would provide more guarantees to users and reduces the level of trust required.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

InfinityStaker.sol

updatePenalties

TOOLS USED

Manual Analysis

MITIGATION

Either add a timelock to updatePenalties(), or add a maximum penalty check.

nneverlander commented 2 years ago

Thanks

HardlyDifficult commented 2 years ago

Merging with https://github.com/code-423n4/2022-06-infinity-findings/issues/301

HardlyDifficult commented 2 years ago

I love how you name the inlined links -- really improves the readability.

HardlyDifficult commented 2 years ago

Low:

Non-critical: