Infinity NFT Marketplace contest - abhinavmir's results

Audit of Infinity NFT Marketplace

Transfers ETH without require check

_transferFees() - Found here

While this is in an internal function, it is being called a few times earlier. The require functions to protect this internal function seem to focus on metadata of currency and such. A require function that whitelists who can and cannot access _transferFees() will probably add an extra layer of security. Related details here (SWC-105).

Divides before multiply

This is a low level severity, but can be exploited in specific scenarios. Since the variable dependency runs deep, there is added layer of ambiguity.

Instances found -

1. Calculating price difference - also found in InfinityOrderBookComplication.
2. PortionBPS - also found in InfinityOrderBookComplication.
3. epochsPassedSinceLastAdvance - depends on division before multiplication and block.timestamp - both unreliable in the right circumstance (although, latter is practically a non-threat on main-net Ethereum due to chain maturity). Found here.

Functions that update parameters don't emit events

Usually, it is a good idea to emit events whenever variables that act as parameters later are changed. updateStakeLevelThreshold and updatePenalties both have this property as seen here.

Calls inside loops

This is probably fundamentally necessary to Infinity, but with some rearchitecting/using the frontend, is it possible to reduce this? Ref. this.

Another thing to keep in mind is that Ethereum usually prefers pull over push when giving out NFTs.

There are around 12 instances of this - maybe all multiple calls have no other option. Anyway, listed as follows-

1. _transferERC1155
2. _nftsHash
3. _tokensHash
4. matchOneToOneOrders
5. matchOneToManyOrders has two instances, here's another one.
6. matchOrders
7. takeMultipleOneOrders
8. takeOrders
9. cancelMultipleOrders
10. _transferMultipleNFTs
11. _transferERC721
12. _tokensHash

Gas optimisation by using the external keyword

getUserTotalStaked() uses public, which could instead be external (since it isn't used anywhere else in the codebase). Saves you a small amount of gas. This is also true for getUserTotalVested().