code-423n4 / 2022-05-backd-findings

0 stars 0 forks source link

QA Report #159

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago
  1. Reentrancy issue in a function The functions below have an external call which can allow user to reenter into the function. Although reentering the function would cause harm to the caller than good.

*Occurrences in: https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/AmmGauge.sol#L130-L134 *https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/AmmGauge.sol#L108-L111


  1. Missing events and emit Certain events and emits are necessary for core changes and admin/critical activities to allow monitoring on third party tools. The following below are missing;

*Occurrences in: https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/VestedEscrow.sol#L68 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/VestedEscrow.sol#L74 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/Minter.sol#L99


3.. Missing zero address check The following are missing checks for existence of zero address which may lead to transfers to zero address or causing some functions to no longer be accessible.

*Occurrences in: https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/AmmGauge.sol#L124 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/AmmGauge.sol#L56 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/AmmGauge.sol#L103 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/BkdLocker.sol#L70 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/FeeBurner.sol#L31 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/FeeBurner.sol#L31 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/StakerVault.sol#L111 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/StakerVault.sol#L139 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/StakerVault.sol#L359 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/Minter.sol#L126 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/Minter.sol#L144 *https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/VestedEscrow.sol#L65


  1. Use of Deprecated safeApprove() function The OpenZeppelin ERC20 SafeApprove() function has been deprecated, replace safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead.

*Occurrences in: https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/RewardHandler.sol#L52 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/RewardHandler.sol#L64 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/FeeBurner.sol#L118 *https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/zaps/PoolMigrationZap.sol#L27


  1. Max approvals are risky Maximum approvals are widely considered as unsafe if the approved contract becomes compromised/malicious.

*Occurrences in: https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/RewardHandler.sol#L64 *https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/FeeBurner.sol#L118


6.. Costly external calls in a loop https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/Controller.sol#L127 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/RewardHandler.sol#L44 *https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/RewardHandler.sol#L44 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/FeeBurner.sol#L70 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/VestedEscrow.sol#L99 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/VestedEscrow.sol#L102


  1. Use of unsafe approve() https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/VestedEscrow.sol#L25
GalloDaSballo commented 2 years ago

Reentrancy issue in a function

Would rephrase to lack of CEI pattern, as no reEntrancy was demonstrated

Missing events and emit

Informational

3.. Missing zero address check

Valid

Use of Deprecated safeApprove() function

Technically valid but safeApprove is being used correctly throughout the codebase

Max approvals are risky

In lack of alternative, the one line comment is not useful

6.. Costly external calls in a loop

In lack of a suggested refactoring, the one line comment is not useful

Use of unsafe approve()

Agree that this needs to be changed, and believe Low Severity to be appropriate because this is happening in a constructor