search

code-423n4 / 2022-06-badger-findings

0 stars 0 forks source link

QA Report #106

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

ISSUE LIST

C4-001 : Missing events for only functions that change critical parameters - Non Critical

C4-002 : Critical changes should use two-step procedure - Non Critical

C4-003 : Missing zero-address/values check in the constructor - Low

C4-004 : The Contract Should approve(0) first - Low

C4-005 : Incompatibility With Rebasing/Deflationary/Inflationary tokens - Non Critical

C4-006 : Front-runnable Initializers - LOW

C4-007 : Bump OZ packages to ^4.5.0. - Non critical

C4-008 : Natspec documentation is missing in the contract - LOW

C4-009 : Open TODOs - LOW

C4-010 : Unbounded loops with external calls - Non critical

ISSUES

C4-001 : Missing events for only functions that change critical parameters

Impact - Non critical

The afunctions that change critical parameters should emit events. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services. The alternative of directly querying on-chain contract state for such changes is not considered practical for most users/usages.

Missing events and timelocks do not promote transparency and if such changes immediately affect users’ perception of fairness or trustworthiness, they could exit the protocol causing a reduction in liquidity which could negatively impact protocol TVL and reputation.

Proof of Concept

  1. Navigate to the following contract.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L86

https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L92

https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L98

See similar High-severity H03 finding OpenZeppelin’s Audit of Audius (https://blog.openzeppelin.com/audius-contracts-audit/#high) and Medium-severity M01 finding OpenZeppelin’s Audit of UMA Phase 4 (https://blog.openzeppelin.com/uma-audit-phase-4/)

Tools Used

None

Recommended Mitigation Steps

Add events to all functions that change critical parameters.

C4-002 : Critical changes should use two-step procedure

Impact - NON CRITICAL

The critical procedures should be two step process.

Proof of Concept

  1. Navigate to the following contract.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L98

Tools Used

Code Review

Recommended Mitigation Steps

Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.

C4-003 : # Missing zero-address&values check

Impact

Missing checks for zero-addresses&values may lead to infunctional protocol, if the variable addresses are updated incorrectly.

Proof of Concept

There are a few validations that could be added to the system:

https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L98

Tools Used

Code Review

Recommended Mitigation Steps

Consider adding zero-address and zero value checks.

C4-004 : The Contract Should approve(0) first

Impact - LOW

Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

IERC20(token).safeApprove(address(operator), 0);
IERC20(token).safeApprove(address(operator), amount);

Proof of Concept

  1. Navigate to the following contract.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L67

https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L68
  1. When trying to re-approve an already approved token, all transactions revert and the protocol cannot be used.

Tools Used

None

Recommended Mitigation Steps

Approve with a zero amount first before setting the actual amount. Consider use safeIncreaseAllowance and safeDecreaseAllowance.

C4-005 : Incompatibility With Rebasing/Deflationary/Inflationary tokens

Impact - LOW

PrePo protocol do not appear to support rebasing/deflationary/inflationary tokens whose balance changes during transfers or over time. The necessary checks include at least verifying the amount of tokens transferred to contracts before and after the actual transfer to infer any fees/interest.

Proof of Concept

  1. Navigate to the following contract.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L107

Tools Used

Manual Code Review

Recommended Mitigation Steps

C4-006 : Front-runnable Initializers

Impact - LOW

All contract initializers were missing access controls, allowing any user to initialize the contract. By front-running the contract deployers to initialize the contract, the incorrect parameters may be supplied, leaving the contract needing to be redeployed.

Proof of Concept

  1. Navigate to the following contract.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L56
  1. initialize functions does not have access control. They are vulnerable to front-running.

Tools Used

Manual Code Review

Recommended Mitigation Steps

While the code that can be run in contract constructors is limited, setting the owner in the contract's constructor to the msg.sender and adding the onlyOwner modifier to all initializers would be a sufficient level of access control.

C4-007: Bump OZ packages to ^4.5.0.

Impact - NON CRITICAL

Line Reference

https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/brownie-config.yaml#L11

Description

I can verify that the installed version is 3.4.0 by executing the following commands:

https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/brownie-config.yaml#L11

Recommended Mitigation Steps

Update the versions of @openzeppelin/contracts and @openzeppelin/contracts-upgradeable to be the latest in package.json. I also recommend double checking the versions of other dependencies as a precaution, as they may include important bug fixes.

C4-008: Natspec Documentation Is Missing

Impact - NON CRITICAL

Line Reference

https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L78

Ensure that the code is well commented on both with NatSpec and inline comments for better readability and maintainability. The comments should accurately reflect what the corresponding code does.

Recommended Mitigation Steps

Follow the official NatSpec comment guidelines to make the contract more readable. https://docs.soliditylang.org/en/develop/natspec-format.html

C4-009 : Open TODOs

Impact

Open TODOs can point to architecture or programming issues that still need to be resolved.

Proof of Concept

  1. Navigate to the following contract.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L284

Tools Used

None

Recommended Mitigation Steps

Consider resolving the TODOs before deploying.

C4-010 : Unbounded loops with external calls

Impact

The interface and the function should require a start index and a lenght, so that the index composition can be fetched in batches without running out of gas. If there are thousands of index components (e.g. like the Wilshire 5000 index), the function may revert.

Proof of Concept

  1. Navigate to the following contract.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L288

Tools Used

None

Recommended Mitigation Steps

Define proper bounds on the for loops.

GalloDaSballo commented 2 years ago
Screenshot 2022-06-19 at 04 06 27

Let's just penalize this submission