Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA Report #98

Open
code423n4 opened this issue Jun 17, 2022 · 2 comments
Open

QA Report #98

code423n4 opened this issue Jun 17, 2022 · 2 comments
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@code423n4
Copy link
Contributor

QA (LOW & NON-CRITICAL)

[L-01] Missing checks for contract existence

OpenZeppelin's safeTransfer and safeTransferFrom functions use functionCall of Address.sol which is a low level call.
However, it's stated that the target must be a contract and code existence should be checked prior using. Reference It's advised to check code existence prior calling safeTransfer or safeTransferFrom as below;
In InfinityExchange.sol;

729: IERC20(buy.execParams[1]).safeTransferFrom(buy.signer, sell.signer, remainingAmount);

In InfinityStaker.sol;

128:     IERC20(INFINITY_TOKEN).safeTransfer(msg.sender, amount);
141:     IERC20(INFINITY_TOKEN).safeTransfer(msg.sender, totalToUser);
142:     IERC20(INFINITY_TOKEN).safeTransfer(INFINITY_TREASURY, penalty);

[L-02] Missing of zero address checks during construction

There are no address(0) or Zero value check at the constructors of:
InfinityExchange.sol, InfinityStaker.sol, InfinityToken.sol

[L-03] Missing indexes in important events

Non of the events in InfinityExchange.sol, InfinityToken.sol has indexed parameter.

[L-04] Redundant statement

The team might consider to remove the line below since the condition is already checked in Signaturechecker.verify function;

523:       order.signer == address(0) ||

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

[L-05] Critical address / parameter changes

Critical address / value changes are not done in two steps for the following methods:
At InfinityStaker.sol;
updateInfinityTreasury(), updateStakeLevelThreshold(), updatePenalties(),

Changing critical addresses in contracts should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one (i.e. claims ownership). This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible. Reference, Reference

[L-06] Signature malleability of EVM's ecrecover in verify()

The contract uses ecrecover() function after many function triggering in InfinityExchange.sol's verifyOrderSig() . EVM's ecrecover is susceptible to signature malleability which allows replay attacks, but using OpenZeppelin’s ECDSA library can be mitigation for this. Reference

[L-07] Zero check for denominator at contract level

At InfinityExchange.sol matchOneToOneOrders() function, the parameter makerOrders1.length should be checked against being zero. No matter it's checked at the interface level, if somehow it's missed and the calldata was input as 0, it will revert.

[N-01] All capitals are used for CONSTANTS

All capital letters in a variable is used for constanst/immutable variables. However, this was not followed for the below;

1161:     uint256 PRECISION = 10**4; // precision for division; similar to bps

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

@code423n4 code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jun 17, 2022
code423n4 added a commit that referenced this issue Jun 17, 2022
@nneverlander
Copy link
Collaborator

Thanks

@HardlyDifficult
Copy link
Collaborator

Merging with #99

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

4 participants