dexalot / contracts Goto Github PK

• Version bumps
• Relocate MockToken.sol out of the library folder into its own mock folder
• Upgrade packages.

The function setPortfolio updates a critical contract value therefore should emit an event for better tracking off-chain.
Recommendation: Please emit an event when changing the portfolio value.

Lines: Exchange.sol#105-108

function setPortfolio(IPortfolio _portfolio) public {
    require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "E-OACC-05");
    portfolio = _portfolio;
}

Calling length() method of the EnumerableSetUpgradeable for the state variable is burning gas.
Recommendation: Please store result of the length() call to the local variable and use it in the loop.

Lines: Fee.sol#77

for (uint i=0; i<tokenList.length(); i++) {

Lines: Fee.sol#104

for (uint j= 0; j < tokenList.length(); j++) {

Lines: Fee.sol#145

for (uint j=0; j<tokenList.length(); j++) {

Accessing the state variable in the function multiple times just burns the gas.
Recommendation: Please store the value of the state variable in the local variable.

Lines: Fee.sol#69-74

if (orderBookMap[_orderBookID].orderBook.exists(_price)) {
    (price, parent, left, right, red) = orderBookMap[_orderBookID].orderBook.getNode(_price);
    ( , head, ) = orderBookMap[_orderBookID].orderList[_price].getNode('');
    size = orderBookMap[_orderBookID].orderList[_price].sizeOf();
    return (price, parent, left, right, red, head, size);
}

The openzeppelin-upgrades package fixed a bug in their latest version that handled transitive contracts correctly. It requires Dexalot contracts to switch to openzeppelin-contracts-upgradeable.

The function setTradePairs updates a critical contract value therefore should emit an event for better tracking off-chain.
Recommendation: Please emit an event when changing the tradePairs value.

Lines: Exchange.sol#116-119

function setTradePairs(ITradePairs _tradePairs) public {
    require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "E-OACC-06");
    tradePairs = _tradePairs;
}

When visibility is not explicitly declared it is assumed to be internal. But it could be unclear to reviewers.
Recommendation: Please add an explicit visibility declaration.

Lines: Fee.sol#36-49

// bytes32 symbols to ERC20 token map
mapping (bytes32 => IERC20) tokenMap;

// map for numerator for share percentages
mapping (address => uint) share;

// total witdrawn by all users mapped to asset
mapping (bytes32 => uint) totalWithdrawn;

// starting total for a specific user mapped to user and asset
mapping (address => mapping (bytes32 => uint)) userTotalStart;

// total withdrawn by a specific user mapped to user and asset
mapping (address => mapping (bytes32 => uint)) userWithdrawn;

Implemented a fee collection mechanism that does not need a separate Fee contract.