Infinity NFT Marketplace contest - oyc_109's results

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1230

Vulnerability details

description

the ETH amount transfered to the destination is msg.value which is provided by the caller

PoC

/2022-06-infinity/contracts/core/InfinityExchange.sol
1229:   function rescueETH(address destination) external payable onlyOwner {
1230:     (bool sent, ) = destination.call{value: msg.value}('');
1231:     require(sent, 'failed');
1232:   }

/2022-06-infinity/contracts/staking/InfinityStaker.sol
345:   function rescueETH(address destination) external payable onlyOwner {
346:     (bool sent, ) = destination.call{value: msg.value}('');
347:     require(sent, 'Failed to send Ether');
348:   }

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L326 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L362

Vulnerability details

description

in the function takeMultipleOneOrders() in InfinityExchange.sol the require statement that checks msg.value should use == rather than >=, to protect the user if they send more than is required for the order

PoC

/2022-06-infinity/contracts/core/InfinityExchange.sol
326: require(msg.value >= totalPrice, 'invalid total price');
362: require(msg.value >= totalPrice, 'invalid total price');

Event is missing indexed fields

description

Each event should use three indexed fields if there are three or more fields

findings

/2022-06-infinity/contracts/core/InfinityExchange.sol
85:   event MatchOrderFulfilled(
86:     bytes32 sellOrderHash,
87:     bytes32 buyOrderHash,
88:     address seller,
89:     address buyer,
90:     address complication, // address of the complication that defines the execution
91:     address currency, // token address of the transacting currency
92:     uint256 amount // amount spent on the order
93:   );
94: 
95:   event TakeOrderFulfilled(
96:     bytes32 orderHash,
97:     address seller,
98:     address buyer,
99:     address complication, // address of the complication that defines the execution
100:     address currency, // token address of the transacting currency
101:     uint256 amount // amount spent on the order
102:   );

missing checks for zero address

description

Checking addresses against zero-address during initialization or during setting is a security best-practice. However, such checks are missing in address variable initializations/changes in many places.

Impact: Allowing zero-addresses will lead to contract reverts and force redeployments if there are no setters for such address variables.

findings

/2022-06-infinity/contracts/core/InfinityExchange.sol
115:     WETH = _WETH;
116:     MATCH_EXECUTOR = _matchExecutor;

/2022-06-infinity/contracts/core/InfinityExchange.sol
1229:   function rescueETH(address destination) external payable onlyOwner {
1230:     (bool sent, ) = destination.call{value: msg.value}('');
1231:     require(sent, 'failed');
1232:   }

/2022-06-infinity/contracts/core/InfinityExchange.sol
1255:   function updateMatchExecutor(address _matchExecutor) external onlyOwner {
1256:     MATCH_EXECUTOR = _matchExecutor;
1257:   }
1258: 

/2022-06-infinity/contracts/staking/InfinityStaker.sol
375:   function updateInfinityTreasury(address _infinityTreasury) external onlyOwner {
376:     INFINITY_TREASURY = _infinityTreasury;
377:   }

/2022-06-infinity/contracts/token/InfinityToken.sol
55: _mint(admin, supply);

Unused receive()/fallback() function

description

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert

any funds accidently sent to the contract from a user will be lost

findings

/2022-06-infinity/contracts/core/InfinityExchange.sol
119:   fallback() external payable {}
121:   receive() external payable {}

/2022-06-infinity/contracts/staking/InfinityStaker.sol
55:   fallback() external payable {}
57:   receive() external payable {}

add protection for non ETH transfers

description

in the function takeMultipleOneOrders() and takeOrders() in InfinityExchange.sol, if the order is non ETH add a require statement to make sure msg.value == 0

should use constants rather than magic values

description

/2022-06-infinity/contracts/core/InfinityExchange.sol
725: uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
1161: uint256 PRECISION = 10**4; // precision for division; similar to bps

/2022-06-infinity/contracts/core/InfinityOrderBookComplication.sol
338: uint256 PRECISION = 10**4; // precision for division; similar to bps

owner can increase fee to any amount

description

the owner can increase protocol fee without timelock to any amount

recommending adding a timelock and sanity checks to limit the amount of fees

findings

/2022-06-infinity/contracts/core/InfinityExchange.sol
1266:   function setProtocolFee(uint16 _protocolFeeBps) external onlyOwner {
1267:     PROTOCOL_FEE_BPS = _protocolFeeBps;
1268:     emit NewProtocolFee(_protocolFeeBps);
1269:   }

Use of Block.timestamp

description

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

findings

/2022-06-infinity/contracts/staking/InfinityStaker.sol
102: userstakedAmounts[msg.sender][newDuration].timestamp = block.timestamp;

/2022-06-infinity/contracts/token/InfinityToken.sol
51:     previousEpochTimestamp = block.timestamp;
52:     currentEpochTimestamp = block.timestamp;

Don't Initialize Variables with Default Value

description

Uninitialized variables are assigned with the types default value.

Explicitly initializing a variable with it's default value costs unnecesary gas.

findings

/2022-06-infinity/contracts/core/InfinityExchange.sol
148: for (uint256 i = 0; i < numMakerOrders; ) {
200: for (uint256 i = 0; i < ordersLength; ) {
219: for (uint256 i = 0; i < ordersLength; ) {
272: for (uint256 i = 0; i < numSells; ) {
308: for (uint256 i = 0; i < numMakerOrders; ) {
349: for (uint256 i = 0; i < ordersLength; ) {
393: for (uint256 i = 0; i < numNonces; ) {

/2022-06-infinity/contracts/core/InfinityOrderBookComplication.sol
76: for (uint256 i = 0; i < ordersLength; ) {
82: for (uint256 j = 0; j < nftsLength; ) {
197: uint256 numConstructedItems = 0;
199: for (uint256 i = 0; i < nftsLength; ) {
214: uint256 numTakerItems = 0;
216: for (uint256 i = 0; i < nftsLength; ) {
244: uint256 numCollsMatched = 0;
246:     for (uint256 i = 0; i < order2NftsLength; ) {
247:       for (uint256 j = 0; j < order1NftsLength; ) {
290:     for (uint256 k = 0; k < item2TokensLength; ) {
291:       for (uint256 l = 0; l < item1TokensLength; ) {
318: uint256 sum = 0;
320: for (uint256 i = 0; i < ordersLength; ) {

Long Revert Strings

description

Shortening revert strings to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

If the contract(s) in scope allow using Solidity >=0.8.4, consider using Custom Errors as they are more gas efficient while allowing developers to describe the error in detail using NatSpec.

findings

/2022-06-infinity/contracts/staking/InfinityStaker.sol
94: 'insufficient staked amount to change duration'
96: require(newDuration > oldDuration, 'new duration must be greater than old duration');