Slither tautology error here. uint256 always >= 0. I think the dev here mean totalStaked > 0.
2.L- admin config updateStakeLevelThreshold() should call with array input and not spread out into multiple transactions
Since there is no implementation for user staking power yet. Admin should be aware of user might use protocol when admin changing staking level.
If admin update stake level of Bronze,Silver,Gold,Plat one by one between the average of 4 blocks (assuming using JS script). There is very small chance of user do something not intended as Threshold is changing.
function _updateStakeLevelThreshold(StakeLevel stakeLevel, uint16 threshold) internal {
if (stakeLevel == StakeLevel.BRONZE) {
BRONZE_STAKE_THRESHOLD = threshold;
} else if (stakeLevel == StakeLevel.SILVER) {
SILVER_STAKE_THRESHOLD = threshold;
} else if (stakeLevel == StakeLevel.GOLD) {
GOLD_STAKE_THRESHOLD = threshold;
} else if (stakeLevel == StakeLevel.PLATINUM) {
PLATINUM_STAKE_THRESHOLD = threshold; //@audit missing input check
}
}
function updateStakeLevelThreshold(StakeLevel[] stakeLevel, uint16[] threshold) external onlyOwner {
for (uint8 i = 0; i < stakeLevel.length; i++) {
_updateStakeLevelThreshold(stakeLevel[i], threshold[i]);
}
// emit event
}
// Or this one is much better as there are fixed amount of enum and threshold
function updateStakeLevelThreshold(uint16 bronzeThreshold,uint16 silverThreshold, uint16 goldThresHold, uint16 platThresHold ) external onlyOwner
{
require (silverThreshold >= bronzeThreshold, "Silver threshold must be greater than Bronze threshold");
require (goldThresHold >= silverThreshold, "Gold threshold must be greater than Silver threshold");
require (platThresHold >= goldThresHold, "Platinum threshold must be greater than Gold threshold");
if(BRONZE_STAKE_THRESHOLD != bronzeThreshold) BRONZE_STAKE_THRESHOLD = bronzeThreshold;
if(SILVER_STAKE_THRESHOLD != silverThreshold) SILVER_STAKE_THRESHOLD = silverThreshold;
if(GOLD_STAKE_THRESHOLD != goldThresHold) GOLD_STAKE_THRESHOLD = goldThresHold;
if(PLATINUM_STAKE_THRESHOLD != platThresHold) PLATINUM_STAKE_THRESHOLD = platThresHold;
// emit event
}
6.L- Admin config ProtocolFee and gasFee missing max amount check which can be used to take fund from user
With PROTOCOL_FEE_BPS > 10000 (more than 100%), the exchange can steal user WETH who might approve max WETH allowance for InfinityExchange.sol
To provide safety for user and in case of compromised both admin and MATCH_EXECUTOR bot server.
Protocol should prevent potential further damage that might steal WETH from all user approve Exchange before.
With maximum amount of WETH_TRANSFER_GAS_UNITS is 4_294_967_296 and default value is 50_000, basically exchange can take any amount of WETH from user if gas price set high enough.
function updateWethTranferGas(uint32 _wethTransferGasUnits) external onlyOwner {
require(_wethTransferGasUnits <= 200_000); // for deflation token or voting token transfer cannot really be more than 200000 gas per transaction
WETH_TRANSFER_GAS_UNITS = _wethTransferGasUnits;
emit NewWethTransferGasUnits(_wethTransferGasUnits);
}
7.L- Not all NFT implement IERC165 interface and _transferNFTs() revert for case like this
All NFT transfer goes through _transferNFTs().
But if-else only check for NFT contract support ERC721 or ERC1155. When nonstandard NFT does not have function to return support interface, the transaction revert.
Either add final else case as fail-safe call ERC721 anyway or throw error to prevent any malicious NFT from entering the exchange.
QA
totalStaked >= 0
is always trueupdateStakeLevelThreshold()
should call with array input and not spread out into multiple transactionsupdatePenalties()
missing input checkupdateInfinityTreasury()
ProtocolFee
andgasFee
missing max amount check which can be used to take fund from user_transferNFTs()
revert for case like this1.N-
totalStaked >= 0
is always trueSlither tautology error here.
uint256
always >= 0. I think the dev here meantotalStaked > 0
.2.L- admin config
updateStakeLevelThreshold()
should call with array input and not spread out into multiple transactionsSince there is no implementation for user staking power yet. Admin should be aware of user might use protocol when admin changing staking level.
If admin update stake level of Bronze,Silver,Gold,Plat one by one between the average of 4 blocks (assuming using JS script). There is very small chance of user do something not intended as Threshold is changing.
3.L- admin config
updatePenalties()
missing input checkSolidity throw error when divided by 0. Admin should prevent this so user wont be locked out of
ragequit()
early.4.L- Missing admin Zero-address check for
updateInfinityTreasury()
Cant have user send token to burn address thanks to javascript.
5.N- Missing admin event in general
With admin event, it is for easy tracking from off chain for rare case like compromised admin key. It just simply look much better with it.
Missing event all of these functions:
updateInfinityTreasury
updatePenalties
updateStakeLevelThrehold
updateMatchExecutor
complication and currency update
6.L- Admin config
ProtocolFee
andgasFee
missing max amount check which can be used to take fund from userWith
PROTOCOL_FEE_BPS > 10000
(more than 100%), the exchange can steal user WETH who might approve max WETH allowance forInfinityExchange.sol
To provide safety for user and in case of compromised both admin andMATCH_EXECUTOR
bot server. Protocol should prevent potential further damage that might steal WETH from all user approve Exchange before.With maximum amount of
WETH_TRANSFER_GAS_UNITS
is4_294_967_296
and default value is50_000
, basically exchange can take any amount of WETH from user if gas price set high enough.7.L- Not all NFT implement IERC165 interface and
_transferNFTs()
revert for case like thisAll NFT transfer goes through _transferNFTs(). But if-else only check for NFT contract support ERC721 or ERC1155. When nonstandard NFT does not have function to return support interface, the transaction revert.
Either add final else case as fail-safe call ERC721 anyway or throw error to prevent any malicious NFT from entering the exchange.