search

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

0 stars 0 forks source link

QA Report #358

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

QA Report

Table of Contents

summary

Few vulnerabilities were found examining the contracts. The main concerns are with the unsafe use of IERC20 methods.

Function reverting for some tokens

PROBLEM

approve() reverts if called on a non-zero allowance without approving 0 first for some tokens (e.g: USDT). This means the logic in JBERC20PaymentTerminal will not work for these tokens

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

JBERC20PaymentTerminal.sol

99:     IERC20(token).approve(_to, _amount);

TOOLS USED

Manual Analysis

MITIGATION

Consider adding an extra step to approve for _amount = 0 before approving the new _amount

Immutable addresses lack zero-address check

IMPACT

constructors should check the address written in an immutable address variable is not the zero address

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

JBTokenStore.sol

160:   constructor(
161:     IJBOperatorStore _operatorStore,
162:     IJBProjects _projects,
163:     IJBDirectory _directory
164:   ) JBOperatable(_operatorStore) JBControllerUtility(_directory) {
165:     projects = _projects;
166:   }

JBSplitsStore.sol

120:   constructor(
121:     IJBOperatorStore _operatorStore,
122:     IJBProjects _projects,
123:     IJBDirectory _directory
124:   ) JBOperatable(_operatorStore) {
125:     projects = _projects;
126:     directory = _directory;
127:   }

JBDirectory.sol

182:   constructor(
183:     IJBOperatorStore _operatorStore,
184:     IJBProjects _projects,
185:     IJBFundingCycleStore _fundingCycleStore,
186:     address _owner
187:   ) JBOperatable(_operatorStore) {
188:     projects = _projects;
189:     fundingCycleStore = _fundingCycleStore;

JBController.sol

369:   constructor(
370:     IJBOperatorStore _operatorStore,
371:     IJBProjects _projects,
372:     IJBDirectory _directory,
373:     IJBFundingCycleStore _fundingCycleStore,
374:     IJBTokenStore _tokenStore,
375:     IJBSplitsStore _splitsStore
376:   ) JBOperatable(_operatorStore) {
377:     projects = _projects;
378:     directory = _directory;
379:     fundingCycleStore = _fundingCycleStore;
380:     tokenStore = _tokenStore;
381:     splitsStore = _splitsStore;
382:   }

JBPayoutRedemptionPaymentTerminal.sol

283:   constructor(
284:     // payable constructor save the gas used to check msg.value==0
285:     address _token,
286:     uint256 _decimals,
287:     uint256 _currency,
288:     uint256 _baseWeightCurrency,
289:     uint256 _payoutSplitsGroup,
290:     IJBOperatorStore _operatorStore,
291:     IJBProjects _projects,
292:     IJBDirectory _directory,
293:     IJBSplitsStore _splitsStore,
294:     IJBPrices _prices,
295:     IJBSingleTokenPaymentTerminalStore _store,
296:     address _owner
297:   )
298:     payable
299:     JBSingleTokenPaymentTerminal(_token, _decimals, _currency)
300:     JBOperatable(_operatorStore)
301:   {
302:     baseWeightCurrency = _baseWeightCurrency;
303:     payoutSplitsGroup = _payoutSplitsGroup;
304:     projects = _projects;
305:     directory = _directory;
306:     splitsStore = _splitsStore;
307:     prices = _prices;
308:     store = _store;

JBSingleTokenPaymentTerminalStore.sol

276:   constructor(
277:     IJBDirectory _directory,
278:     IJBFundingCycleStore _fundingCycleStore,
279:     IJBPrices _prices
280:   ) {
281:     directory = _directory;
282:     fundingCycleStore = _fundingCycleStore;
283:     prices = _prices;
284:   }

TOOLS USED

Manual Analysis

Potential revert on transfer with IERC20

PROBLEM

Some popular tokens do not implement the ERC20 standard properly. For example Tether 's transfer() and transferFrom() functions do not return booleans as the specification requires: when these tokens are cast to IERC20, their function signatures do not match and the function calls revert. This means these tokens cannot be used by the protocol's payment terminal.

SEVERITY

Low

PROOF OF CONCEPT

JBERC20PaymentTerminal.sol

87:       ? IERC20(token).transfer(_to, _amount)
88:       : IERC20(token).transferFrom(_from, _to, _amount);

TOOLS USED

Manual Analysis

MITIGATION

Use OpenZeppelin’s SafeERC20's safeTransfer()

Return value of transfer not checked

PROBLEM

Some ERC20 tokens do not revert upon failure in transfer()/transferFrom(), but simply return false. The function _transferFrom() in JBERC20PaymentTerminal.sol does not check the return value of IERC20.transfer and IERC20.transferFrom, meaning some token transfers may fail without getting noticed.

SEVERITY

Low

PROOF OF CONCEPT

JBERC20PaymentTerminal.sol

87:       ? IERC20(token).transfer(_to, _amount)
88:       : IERC20(token).transferFrom(_from, _to, _amount);

TOOLS USED

Manual Analysis

MITIGATION

Check the return values of these function calls.

Comment Missing function parameter

PROBLEM

Some of the function comments are missing Natspec function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Missing natspec comments include:

JBController.sol

258 @param _configuration

TOOLS USED

Manual Analysis

MITIGATION

Add a Natspec comment for this parameter

Return statements

PROBLEM

Adding a return statement when the function defines a named return variable is redundant

SEVERITY

Non-Critical

PROOF OF CONCEPT

JBController.sol

574:     return tokenStore.issueFor(_projectId, _name, _symbol);

TOOLS USED

Manual Analysis

MITIGATION

-574:     return tokenStore.issueFor(_projectId, _name, _symbol);
+574:     IJBToken token = tokenStore.issueFor(_projectId, _name, _symbol);

Return value of approve not checked

PROBLEM

Not all IERC20 implementations revert when there's a failure in approve(). The function signature has a boolean return value and they indicate errors that way instead. When calling that function, it is hence recommended to check the return value.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

JBERC20PaymentTerminal.sol

99:     IERC20(token).approve(_to, _amount);

TOOLS USED

Manual Analysis

MITIGATION


-99:     IERC20(token).approve(_to, _amount);
+99:     (bool success) = IERC20(token).approve(_to, _amount);
+ require(success);

Consider also this issue that can happen with the .approve() method.

Scientific notation

PROBLEM

For readability, it is best to use scientific notation (e.g 10e5) rather than decimal literals(100_000) or exponentiation(10**5). Underscores are used throughout the contracts and do improve readability too, so this is more of a suggestion.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

JBPayoutRedemptionPaymentTerminal.sol

87:   uint256 internal constant _FEE_CAP = 50_000_000;
167:   uint256 public override fee = 25_000_000;

TOOLS USED

Manual Analysis

Timelock for critical parameter change

PROBLEM

It is good practice to add timelock to critical parameters changes, such as admin changes, to give users time to react. This is especially true for fee changes that directly affect protocol users. Merely a suggestion here as there is already a maximum value to which fee can be set, which can be deemed sufficient for users.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

JBPayoutRedemptionPaymentTerminal.sol

622:   function setFee(uint256 _fee) external virtual override onlyOwner {
623:     // The provided fee must be within the max.
624:     if (_fee > _FEE_CAP) revert FEE_TOO_HIGH();
625: 
626:     // Store the new fee.
627:     fee = _fee;

TOOLS USED

Manual Analysis

MITIGATION

Add a timelock to setFee

Transfer of ownership can be two-steps

PROBLEM

Consider using a 2-step ownership transfer for JBToken, so that the new owner must approve the ownership transfer, preventing issues if ownership was to be transferred to a random address.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

JBTokenStore.sol

266:       oldToken.transferOwnership(_projectId, _newOwner);

TOOLS USED

Manual Analysis

drgorillamd commented 2 years ago

This is a comprehensive list, well documented, nice!