search

code-423n4 / 2022-07-juicebox-findings

0 stars 0 forks source link

QA Report #357

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

ISSUE LIST

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

C4-002 : Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom - Low

C4-003 : Missing zero-address check in the setter functions and initiliazers - Low

C4-004 : Low level calls with solidity version 0.8.14 can result in optimiser bug. - LOW

C4-005 : The Contract Should safeApprove(0) first - LOW

C4-006 : Use of Block.timestamp - Non-critical

C4-007 : Incompatibility With Rebasing/Deflationary/Inflationary tokens - LOW

C4-008 : Add disableInitializers to Prevent Front-running - LOW

C4-009 : ERC20 approve method missing return value check - LOW

ISSUES

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

Impact - NON CRITICAL

The critical procedures should be two step process. The contracts inherit OpenZeppelin's Ownable contract which enables the onlyOwner role to transfer ownership to another address. It's possible that the onlyOwner role mistakenly transfers ownership to the wrong address, resulting in a loss of the onlyOwner role. The current ownership transfer process involves the current owner calling Unlock.transferOwnership(). This function checks the new owner is not the zero address and proceeds to write the new owner's address into the owner's state variable. If the nominated EOA account is not a valid account, it is entirely possible the owner may accidentally transfer ownership to an uncontrolled account, breaking all functions with the onlyOwner() modifier. Lack of two-step procedure for critical operations leaves them error-prone if the address is incorrect, the new address will take on the functionality of the new role immediately

for Ex : -Alice deploys a new version of the whitehack group address. When she invokes the whitehack group address setter to replace the address, she accidentally enters the wrong address. The new address now has access to the role immediately and is too late to revert

Proof of Concept

  1. Navigate to the following contract.
https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBPrices.sol#L20

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-002 : Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom

Impact

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

Reference: This similar medium-severity finding from Consensys Diligence Audit of Fei Protocol: https://consensys.net/diligence/audits/2021/01/fei-protocol/#unchecked-return-value-for-iweth-transfer-call

Proof of Concept

  1. Navigate to the following contract.

  2. transfer/transferFrom functions are used instead of safe transfer/transferFrom on the following contracts.

  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol::87 => ? IERC20(token).transfer(_to, _amount)
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol::88 => : IERC20(token).transferFrom(_from, _to, _amount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol::99 => IERC20(token).approve(_to, _amount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20ProjectPayer.sol::271 => IERC20(_token).transferFrom(msg.sender, address(this), _amount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20ProjectPayer.sol::315 => IERC20(_token).transferFrom(msg.sender, address(this), _amount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20ProjectPayer.sol::364 => if (_token != JBTokens.ETH) IERC20(_token).approve(address(_terminal), _amount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20ProjectPayer.sol::412 => if (_token != JBTokens.ETH) IERC20(_token).approve(address(_terminal), _amount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol::256 => IERC20(_token).transferFrom(msg.sender, address(this), _amount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol::301 => IERC20(_token).transfer(
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol::348 => IERC20(_token).transferFrom(msg.sender, address(this), _amount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol::384 => IERC20(_token).transfer(
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol::493 => IERC20(_token).approve(address(_split.allocator), _splitAmount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol::534 => IERC20(_token).transfer(

Tools Used

Code Review

Recommended Mitigation Steps

Consider using safeTransfer/safeTransferFrom or require() consistently.

C4-003 : # Missing zero-address check in the setter functions and initiliazers

Impact

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

Proof of Concept

  1. Navigate to the following contracts.
https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol#L37

Tools Used

Code Review

Recommended Mitigation Steps

Consider adding zero-address checks in the discussed constructors: require(newAddr != address(0));.

C4-004 : Low level calls with solidity version 0.8.6 can result in optimiser bug.

Impact

The protocol is using low level calls with solidity version 0.8.6 which can result in optimizer bug.

https://medium.com/certora/overly-optimistic-optimizer-certora-bug-disclosure-2101e3f7994d

Proof of Concept

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHPaymentTerminal.sol#L25

Tools Used

Code Review

Recommended Mitigation Steps

Consider upgrading to solidity 0.8.15.

C4-005 : The Contract Should safeApprove(0) first - LOW

Impact

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.

  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol::99 => IERC20(token).approve(_to, _amount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol::493 => IERC20(_token).approve(address(_split.allocator), _splitAmount);

When trying to re-approve an already approved token, all transactions revert and the protocol cannot be used.

Proof of Concept

(https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/LiquidityReserve.sol#L81)

Tools Used

None

Recommended Mitigation Steps

Approve with a zero amount first before setting the actual amount.

C4-006 : Use of Block.timestamp

Impact - Non-Critical

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Proof of Concept

  1. Navigate to the following contract.
juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBSplitsStore.sol:206:      if (block.timestamp >= _currentSplits[_i].lockedUntil) continue;
juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol:162:      if (fundingCycle.start > block.timestamp)
juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol:230:      if (!_isApproved(_projectId, _fundingCycle) || block.timestamp < _fundingCycle.start)
juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol:332:    uint256 _configuration = block.timestamp;
juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol:340:      _mustStartAtOrAfter > block.timestamp ? _mustStartAtOrAfter : block.timestamp
juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol:408:    if (!_isApproved(_projectId, _baseFundingCycle) || block.timestamp < _baseFundingCycle.start)
juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol:550:    if (block.timestamp >= _fundingCycle.start) return 0;
juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol:561:      block.timestamp < _fundingCycle.start - _baseFundingCycle.duration
juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol:589:      _fundingCycle.duration > 0 && block.timestamp >= _fundingCycle.start + _fundingCycle.duration
juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol:593:    if (block.timestamp >= _fundingCycle.start) return _fundingCycle.configuration;
juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol:602:      block.timestamp >= _baseFundingCycle.start + _baseFundingCycle.duration
juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol:632:      ? block.timestamp + 1
juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol:633:      : block.timestamp - _baseFundingCycle.duration + 1;
juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol:815:    else if (_ballotFundingCycle.ballot.duration() >= block.timestamp - _configuration)
juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/system_tests/TestTokenFlow.sol:64:      block.timestamp,

Tools Used

Manual Code Review

Recommended Mitigation Steps

Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

C4-007 : 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.

Example Test

During the lending, If the inflationary/deflationary tokens are used excepted amount will be lower than deposit.

Proof of Concept

  1. Navigate to the following contract.
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol::87 => ? IERC20(token).transfer(_to, _amount)
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol::88 => : IERC20(token).transferFrom(_from, _to, _amount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol::99 => IERC20(token).approve(_to, _amount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20ProjectPayer.sol::271 => IERC20(_token).transferFrom(msg.sender, address(this), _amount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20ProjectPayer.sol::315 => IERC20(_token).transferFrom(msg.sender, address(this), _amount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20ProjectPayer.sol::364 => if (_token != JBTokens.ETH) IERC20(_token).approve(address(_terminal), _amount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20ProjectPayer.sol::412 => if (_token != JBTokens.ETH) IERC20(_token).approve(address(_terminal), _amount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol::256 => IERC20(_token).transferFrom(msg.sender, address(this), _amount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol::301 => IERC20(_token).transfer(
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol::348 => IERC20(_token).transferFrom(msg.sender, address(this), _amount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol::384 => IERC20(_token).transfer(
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol::493 => IERC20(_token).approve(address(_split.allocator), _splitAmount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol::534 => IERC20(_token).transfer(

Tools Used

Manual Code Review

Recommended Mitigation Steps

C4-008 : Add disableInitializers to Prevent Front-running

Impact

Defining initial values for variables when declaring them in a contract like in the code below does not work for upgradeable contracts.

Refer to explanation below:

https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#avoid-initial-values-in-field-declarations

Also, one should not leave the implementation contract uninitialized. None of the implementation contracts in the code base contains the code recommended by OpenZeppelin below, or an empty constructor with the initializer modifier.

Tools Used

Code Review

Recommended Mitigation Steps


/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
_disableInitializers();
}

Refer to the link below:

https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#initializing_the_implementation_contract

C4-009 : ERC20 approve method missing return value check - LOW

Impact

The following contract functions performs an ERC20.approve() call but does not check the success return value. Some tokens do not revert if the approval failed but return false instead.

Proof of Concept

  1. Navigate to the following contracts.
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol::99 => IERC20(token).approve(_to, _amount);
  juice-contracts-v2-code4rena-828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol::493 => IERC20(_token).approve(address(_split.allocator), _splitAmount);
  1. Tokens that don't actually perform the approve and return false are still counted as a correct approve.

Tools Used

None

Recommended Mitigation Steps

Its recommend to using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handles the return value check as well as non-standard-compliant tokens.

Reference : https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.1/contracts/token/ERC20/utils/SafeERC20.sol#L74

drgorillamd commented 2 years ago

This looks like a copy-paste of tooling with some "issues" not part of Juicebox codebase