search

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

4 stars 0 forks source link

QA Report #239

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

InfinityStaker

style: avoid to use uppercase notation when not needed, should be used only for constant/immutable

Usually in Solidity, the uppercase notation is used only for constant/immutable variable. The code use this notation for various variables:

Consider changing the name of those variable in camelCase style following the Solidity documentation style guide.

updateStakeLevelThreshold should emit an event

The stake level threshold is important to determinate the stake level of a user's stake. This function should emit an event when a threshold is updated

Consider creating an event when a value is updated and emit it

emit StakeLevelThreshold(stakeLevel, threshold);

updateStakeLevelThreshold is not checking the user input

From a semantic point and from how the logic of getUserStakeLevel works, each threshold should have

1) different values 2) be in a DESC order from PLATINUM to NONE

Without any check, the owner could make all the threshold equal to zero or reverse the order (bronze_threshold = 1000, platinum_threshold = 1)

Consider checking that when updating the value of threshold value, it's greater than the previous but less than the next.

For example, when updating the StakeLevel.SILVER threshold it must be

updatePenalties should emit an event

Penalty value is important to understand how much of the user vested amount will be taken as a penalty when he/she rageQuit.

When that value is updated, it should be tracked via an event. Consider emitting an event like: emit PenaltiesUpdated(threeMonthPenalty, sixMonthPenalty, twelveMonthPenalty);

updateInfinityTreasury should emit an event

When the treasury is updated, the function should emit an event

Consider emitting an event like InfinityTreasuryUpdated(_infinityTreasury)

updatePenalties should check the value of input parameter

While there should be a specific check to prevent to have a value equal to 0 for any of the penalties to prevent rageQuit to revert (already submitted as a separate issue) those values should be checked to preserve a hierarchy like this

THREE_MONTH_PENALTY < SIX_MONTH_PENALTY < TWELVE_MONTH_PENALTY as already specified at contract deployment time.

InfinityExchange

Declare event parameter as indexed when possible

Some events are missing the indexed keyword on addresses. Using the indexed parameters allow external monitoring tool to filter those logs and monitor events in a better way.

Consider declaring those event parameters as indexed to better monitor/filter those events.

Missing event for critical functions

A function that set/update state variable should always emit an event for both monitoring reason, but also as a best practice to allow users to track that a contract's parameter/behavior has changed.

All these functions are missing an event emission:

Consider adding an event emission for those functions

PROTOCOL_FEE_BPS has no min/max value and no check in setProtocolFee

Having a PROTOCOL_FEE_BPS equal to 10_000 mean that the protocol take the whole amount and the user take 0 when selling an NFT. Having a PROTOCOL_FEE_BPS equal to 0 mean that the protocol will get nothing when NFTs are sold.

There should be an explicit min/max value to be documented (for the user/creator) and checked in setProtocolFee to avoid this edge cases.

If the 0 protocol fee is a correct use case, consider to also update _transferFees to skip the calculation and skip the IERC20(currency).safeTransferFrom(buyer, address(this), protocolFee); that would waste gas transferring 0 tokens to the contract.

HardlyDifficult commented 2 years ago

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

HardlyDifficult commented 2 years ago

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