Haqq Network (Vesting Contract for OnSails)

A reentrancy attack can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. If the attacker can control the untrusted contract, they can make a recursive call back to the original function, repeating interactions that would have otherwise not run after the external call resolved the effects. Here in the contract `LockedDeposit`, in the functions: - `deposit()`; - `claimDeposit()`; - `claimOnDeadline()`; The `permitsStatus[hash]` is modified after the external calls. This could be used by a malicious user to perform a reentrancy attack. ### External call(s) ```solidity=346 _safeTransferFrom(token, permit.investor, address(_thisAsOperator), permit.amount); ``` * This function call executes the following external call(s). * In `LockedDeposit._safeTransferFrom`, * `sent = token.transferFrom(sender,recipient,amount)` ### State variables written after the call(s) ```solidity=347 permitsStatus[hash] = PermitStatus.WaitPermitSignature; ``` --- ### External call(s) ```solidity=396 _safeTransfer(token, permit.team, permit.amount); ``` * This function call executes the following external call(s). * In `LockedDeposit._safeTransfer`, * `sent = token.transfer(recipient,amount)` ### State variables written after the call(s) ```solidity=397 permitsStatus[hash] = PermitStatus.Claimed; ``` --- ### External call(s) ```solidity=416 _safeTransfer(token, permit.investor, permit.amount); ``` * This function call executes the following external call(s). * In `LockedDeposit._safeTransfer`, * `sent = token.transfer(recipient,amount)` ### State variables written after the call(s) ```solidity=417 permitsStatus[hash] = PermitStatus.Refunded; ```

A reentrancy attack can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. If the attacker can control the untrusted contract, they can make a recursive call back to the original contract, repeating interactions that would have otherwise not run after the external call resolved the effects. In the `HaqqVesting` contract, the `nonReentrant` modifier prevents reentering: - from `deposit()`to `deposit()`; - from `withdraw()`to `withdraw()`; - from `withdraw()`to `deposit()`; - from `deposit()`to `withdraw()`; However, it is possible to reenter from one of these functions to `transferDepositRights()` because there is no `nonReentrant` modifier on this function. * If a user reenters from the function `deposit()`, then `depositsCounter[_beneficiaryAddress]` will be wrongly updated. This will cause out-of-order events. * If a user reenters from the function `withdraw()`, it will cause out-of-order events.

The functions which are never called internally within the contract should have external visibility for gas optimization. ```solidity=309 function createDeal(ApproveDepositPermit memory permit) public { ``` --- ```solidity=337 function deposit(ApproveDepositPermit calldata permit) payable onlyCreatedStatus(permit) public { ``` --- ```solidity=362 function approve(ApproveDepositPermit calldata permit, Signature calldata signature) ``` --- ```solidity=388 function claimDeposit(ApproveDepositPermit calldata permit, Signature calldata signature) ``` --- ```solidity=411 function claimOnDeadline(ApproveDepositPermit calldata permit) public onlyExpiredDeadline(permit) { ```

There should always be events emitted in the sensitive functions that are controlled by centralization roles.

Addresses should be checked before assignment or external call to make sure they are not zero addresses. ```solidity=61 (bool sent, bytes memory data) = payable(_beneficiaryAddress).call{value : firstWithdrawalAmount}(""); ``` - `_beneficiaryAddress` is not zero-checked before being used. --- ```solidity=161 vestingContractAddress = _vestingContract; ``` - `_vestingContract` is not zero-checked before being used.

The linked statements do not affect the functionality of the codebase and appear to be either leftover from test code or older functionality. Two of these modifiers have been commented in the function `approve()`. Should they not be used in this function?

The signature malleability is possible within the Elliptic Curve cryptographic system. An Elliptic Curve is symmetric on the X-axis, meaning two points can exist with the same `X` value. In the `r`, `s`, and `v` representation this permits us to carefully adjust `s` to produce a second valid signature for the same `r`, thus breaking the assumption that a signature cannot be replayed in what is known as a replay-attack.

The linked variable could be declared as `constant` since this state variable is never modified.

The linked variables assigned in the constructor can be declared as `immutable`. Immutable state variables can be assigned during contract creation but will remain constant throughout the lifetime of a deployed contract. A big advantage of immutable variables is that reading them is significantly cheaper than reading from regular state variables since they will not be stored in storage.