QA Report

[code423n4]

Low

Centralized risks

Owner can change the thresholds wherever he wants.

• InfinityStaker.sol#L351

Also he can change the penalties, and it could facilitate a front-running issues with bad actors.

• InfinityStaker.sol#L364-L372

PROTOCOL_FEE_BPS could be higher than factor (10_000) and it could facilitate a front-running or denial of services with bad actors.

• InfinityExchange.sol#L1267

Unfair staking increase

If the user makes any changes or changeDuration, the staking timestamp is reset and loses all the time already spent in the previous deposit.

Affected source code:

• InfinityStaker.sol#L72
• InfinityStaker.sol#L102

No compatible with fee tokens

The current lock logic does not contemplate ERC20 tokens with fee during the transferFrom, therefore, the amount received by BribeVault will be less than the expected.

Some tokens may implement a fee during transfers, this is the case of USDT, even though the project has currently set it to 0. So, the transferFrom function would return true despite receiving less than expected.

It's recommended to use balance difference in:

• InfinityStaker.sol#L74
• InfinityExchange.sol#L729
• InfinityExchange.sol#L844

Ownable / Pausable

The contract InfinityStaker is Ownable and Pausable, so the owner could resign while the contract is paused, causing a Denial of Service. Owner resignation while paused should be avoided.

Affected source code:

• InfinityStaker.sol#L15

Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

requestChange(ADMIN,X):

• TimelockConfig.sol#L121

Ownable:

• InfinityExchange.sol#L50
• InfinityStaker.sol#L15
• InfinityOrderBookComplication.sol#L14
• MockERC721.sol#L7

Lack of checks

The following methods have a lack checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code:

address(0):

• TimelockConfig.sol#L44
• InfinityStaker.sol#L375
• InfinityExchange.sol#L116

Integer values:

• InfinityToken.sol#L39-L44
• TimelockConfig.sol#L45

Non critical

Update packages

The packages used are out of date, it is good practice to use the latest version of these packages:

"@openzeppelin/contracts": "4.5.0"

Use encode instead of encodePacked for hashig

Use of abi.encodePacked in ConnextMessage is safe, but unnecessary and not recommended. abi.encodePacked can result in hash collisions when used with two dynamic arguments (string/bytes).

There is also discussion of removing abi.encodePacked from future versions of Solidity (ethereum/solidity#11593), so using abi.encode now will ensure compatibility in the future.

Affected source code:

• TimelockConfig.sol#L77

[nneverlander]

Thanks

[HardlyDifficult]

Merging with #15

[HardlyDifficult]

Merging with #14