Jelly -Watt2Trade

When a trader calls withdrawFutureOrder(), it iterates through the FutureCancellations array of the futureOrder to ensure the trader is eligible by first-come-first-serve approach. The issue arises from the cancellations array being a user manipulated array and also never gets decreased. Iterating through such an array where users can decide how large the array can grow could cause a revert due to out of gas error, meaning traders attempting to withdraw their futureOrder would fail causing DOS.

The withdrawOptionOrder() function doesn't delete the sell order on withdraw of sell trade as stated here:https://github.com/Watt-2-Trade/dex-smartcontracts/blob/main/contracts/EscrowAccount.sol#L707 . However, it doesn't include any update to the withdrawal. Trader of the optionSellOrder can execute this action for an infinite amount to drain out funds.

admin calling `StakingVault::whitelistContractAddress()` on any address allows that address to call the `StakingVault::consumeCoins()` function, this also gives that address the ability to DOS any user from removing their coins. However, there is NO undo function on this operation, meaning if this happens it would be permanent and unavertable.

Some mappings were used in the contract to map addresses to a boolean. This is to help know when a value is true or false. However, because these are the only two possible outcomes, it is recommended to use the mapping to serve as the condition within the require statement.  W2TContractAddress[msg.sender] is expected to return a true while !W2TContractAddress[msg.sender] for false.StakingVault: • onlyW2T modifier EscrowAccount: • placeOptionSellOrder • placeOptionBuyOrder

There are some privileged functions in all the contracts under scope that allow for either admin or whitelisted addresses to call, else it fails. Since these functions are certain to fail when triggered when unpermitted addresses, they can be marked as payable functions. Payable functions are cheaper when called and can save over 50 gas or more.

Updating state changes that pertains to addresses are a critical action especially avoiding setting any to the null address. For instance, on changing the ENERGY_COIN address in the staking vault contract to a zero address, this would disrupt the contract flow and cause a denial of service. StakingVault: • consumeCoins EscrowAccount: • adjustContractAddresses

Hardcoding the admin address set to constant and also not a multisig creates a point of target to attackers and leaves the contract permanently unprotected if compromised. This will make it impossible to carry out a swift incident response process to curtail the attack. This is common across all contracts under audit scope.

Unnecessary code implemented on update functions can be removed to save gas. In the preceding lines before the issue pointed out, a new local variable was created called newGen. The newGen variable equals the summation of the CURRENT_FEE_GEN and 1. After updating all mappings that pertains to the fee, the state variable CURRENT_FEE_GEN was afterwards set to newGen. For an optimized code, it is okay to update the CURRENT_FEE_GENhttps://github.com/Watt-2-Trade/dex-smartcontracts/blob/main/contracts/EscrowAccount.sol#L1042 https://github.com/Watt-2-Trade/dex-smartcontracts/blob/main/contracts/EscrowAccount.sol#L1062

In the removeCoins function from the StakingVault contract, an external call was made to the Energy Coin contract before state variables were updated. This exposes the contract to the possibility of a reentrancy. This negates the principle of the check-effect interaction that poses the need for all state updates before an external call. This is also common in the withdrawOptionOrder and withdrawStakingVault: • removeCoins EscrowAccount: • withdrawOptionOrder • withdrawFutureOrder