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 #265

Open
code423n4 opened this issue Jun 19, 2022 · 0 comments
Open

QA Report #265

code423n4 opened this issue Jun 19, 2022 · 0 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

[L-01] Immutable addresses should 0-Check

I recommend adding check of 0-address for immutable addresses.
Not doing so might lead to non-functional contract when it is updated to 0-address accidentally.

Issue found at

InfinityExchange.sol
 55:  address public immutable WETH;
104:  constructor(address _WETH, address _matchExecutor) {
115:    WETH = _WETH;

[L-02] abi.encodePacked should not be used with dynamic types

This is because using abi.encodePacked with dynamic types will cause a hash collisions.
link: https://docs.soliditylang.org/en/v0.8.13/abi-spec.html#non-standard-packed-mode
I recommend using abi.encode instead.

./InfinityExchange.sol:1197:    bytes32 nftsHash = keccak256(abi.encodePacked(hashes));
./InfinityExchange.sol:1213:    bytes32 tokensHash = keccak256(abi.encodePacked(hashes));

[N-01] Event is missing indexed fields

Each event should have 3 indexed fields if there are 3 or more fields.

Issue found at

./InfinityToken.sol:35:  event EpochAdvanced(uint256 currentEpoch, uint256 supplyMinted);
./InfinityStaker.sol:44:  event Staked(address indexed user, uint256 amount, Duration duration);
./InfinityStaker.sol:45:  event DurationChanged(address indexed user, uint256 amount, Duration oldDuration, Duration newDuration);
./InfinityStaker.sol:46:  event UnStaked(address indexed user, uint256 amount);
./InfinityStaker.sol:47:  event RageQuit(address indexed user, uint256 totalToUser, uint256 penalty);
./InfinityExchange.sol:80:  event CancelAllOrders(address user, uint256 newMinNonce);
./InfinityExchange.sol:81:  event CancelMultipleOrders(address user, uint256[] orderNonces);
./InfinityExchange.sol:82:  event NewWethTransferGasUnits(uint32 wethTransferGasUnits);
./InfinityExchange.sol:83:  event NewProtocolFee(uint16 protocolFee);
./InfinityExchange.sol:85-93:  event MatchOrderFulfilled(bytes32 sellOrderHash, bytes32 buyOrderHash, address seller, address buyer, address complication, // address of the complication that defines the execution address currency, // token address of the transacting currency uint256 amount // amount spent on the order);
./InfinityExchange.sol:95:  event TakeOrderFulfilled(bytes32 orderHash, address seller, address buyer, address complication, // address of the complication that defines the execution address currency, // token address of the transacting currency uint256 amount // amount spent on the order);

[N-02] Unnecessary use of named returns

Several function adds return statement even thought named returns variable are used.
Remove unnecessary named returns variable to improve code readability.
Also keeping the use of named returns or return statement consistent through out the whole project if possible is recommended.

Issue found at
InfinityToken.sol

Remove returns variable "admin"
113:  function getAdmin() public view returns (address admin) {
114:    return address(uint160(TimelockConfig.getConfig(TimelockConfig.ADMIN).value));
115:  }
Remove returns variable "timelock"
117:  function getTimelock() public view returns (uint256 timelock) {
118:    return TimelockConfig.getConfig(TimelockConfig.TIMELOCK).value;
119:  }
Remove returns variable "inflation"
121:  function getInflation() public view returns (uint256 inflation) {
122:    return TimelockConfig.getConfig(EPOCH_INFLATION).value;
123:  }
Remove returns variable "cliff"
125:  function getCliff() public view returns (uint256 cliff) {
126:    return TimelockConfig.getConfig(EPOCH_CLIFF).value;
127:  }
Remove returns variable "totalEpochs"
129:  function getMaxEpochs() public view returns (uint256 totalEpochs) {
130:    return TimelockConfig.getConfig(MAX_EPOCHS).value;
131:  }
Remove returns variable "epochDuration"
133:  function getEpochDuration() public view returns (uint256 epochDuration) {
134:    return TimelockConfig.getConfig(EPOCH_DURATION).value;
135:  }

[N-03] Large Multiples of Ten should use Scientific Notation

Large multiples of ten is hard to read so use scientific notation instead for readability.

Issue found at

./InfinityStaker.sol:35:  uint16 public GOLD_STAKE_THRESHOLD = 10000;
./InfinityExchange.sol:381:    require(minNonce < userMinOrderNonce[msg.sender] + 1000000, 'too many');
./InfinityExchange.sol:725:    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
./InfinityExchange.sol:775:    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
./InfinityExchange.sol:819:    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
./InfinityExchange.sol:873:    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
./InfinityExchange.sol:1135:    uint256 protocolFee = (PROTOCOL_FEE_BPS * amount) / 10000;
@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 19, 2022
code423n4 added a commit that referenced this issue Jun 19, 2022
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

3 participants