search

code-423n4 / 2022-02-jpyc-findings

1 stars 0 forks source link

QA Report #35

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Codebase Impressions & Summary

Overall, code quality for the JPYC contracts is very high. Supporting documentation provided adequate information on design choices made, such as why the UUPS proxy was chosen over the transparent proxy pattern.

The test suite could be easily run, and are rather comprehensive. One thing that stood out and that I’m impressed with was a test checklist. すばらしい! Test coverage was close to 100% with the following functions / branches missed (not significant IMO):

I could be mistaken, but I would like to mention that while the coverage tool highlights that zero inputs for FiatTokenV2’s initializer weren’t tested, they actually are in the FiatTokenV2_proxy test file.

The findings I made revolved around the upgradeability aspect of the contracts. I also made recommendations on adding / removing functionality when the contract is paused.

Low Severity Findings

L01: Add constructor initializer in implementation contracts

Description

As per OpenZeppelin’s (OZ) recommendation, “The guidelines are now to make it impossible for anyone to run initialize on an implementation contract, by adding an empty constructor with the initializer modifier. So the implementation contract gets initialized automatically upon deployment.”

Note that this behaviour is also incorporated the OZ Wizard since the UUPS vulnerability discovery: “Additionally, we modified the code generated by the Wizard 19 to include a constructor that automatically initializes the implementation when deployed.”

Furthermore, this thwarts any attempts to frontrun the initialization tx of the implementation contract.

Incorporating this change would require inheriting the Initializable contract instead of having an explicit initialized variable.

Recommended Mitigation Steps

FiatTokenV1, FiatTokenV2 and subsequent implementation contracts should inherit OZ’s Initializable contract and have the following constructor method:

import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

// TODO: remove bool internal initialized;
// TODO: remove initialized = true;
contract FiatTokenV1 is 
    Initializable
    ...
{
    ...
    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor() initializer {
        // so that users won't accidentally send JPYC to the implementation contract
        blocklisted[address(this)] = true;
    }
}

L02: Use OZ upgrades (hardhat) plugin to handle proxy deployments and upgrades

Description

The project manually deploys and manages their own proxy contract and upgrades. I strongly recommend that the team use the upgrades plugin from OpenZeppelin instead, because it provides an important feature of validating that the incoming implementations are upgrade safe.

I note that the plugin is part of package.json and was imported into hardhat config file, but am puzzled why it wasn’t used (at least in tests).

Recommended Mitigation Steps

Strongly consider using the OZ upgrades plugin to manage deployments. More information about its usage can be found in their documentation and UUPS Proxy guide.

An example is provided below:

const { ethers, upgrades } = require("hardhat");

contract('TestDeploymentAndUpgrade', async function (accounts) {
  const minter = accounts[6];
  const masterMinter = accounts[3];
  const pauser = accounts[4];
  const blocklister = accounts[5];

  it('should do deployment and upgrades', async () => {
    // Deploying
    const FiatTokenV1 = await ethers.getContractFactory("FiatTokenV1");
    const instance = await upgrades.deployProxy(FiatTokenV1, [
      'JPY Coin',
      'JPYC',
      'JPY',
      18,
      masterMinter,
      pauser,
      blocklister,
      minter
      ],
      { kind: 'uups' });
      await instance.deployed();

      // Upgrading
      const FiatTokenV2 = await ethers.getContractFactory("FiatTokenV2");
      const upgraded = await upgrades.upgradeProxy(instance.address, FiatTokenV2);
    });
});

L03: Contracts are not using their OZ upgradeable counterparts

Tools Used

Diffchecker

Description

The non-upgradeable standard version of OpenZeppelin’s library, such as Ownable, Pausable, Address, Context, SafeERC20, ERC1967Upgrade etc, are inherited / used by both the proxy and the implementation contracts.

As a result, when attempting to use the upgrades plugin mentioned, the following errors are raised:

Error: Contract `FiatTokenV1` is not upgrade safe

contracts/v1/FiatTokenV1.sol:58: Variable `totalSupply_` is assigned an initial value
  Move the assignment to the initializer
  https://zpl.in/upgrades/error-004

contracts/v1/Pausable.sol:49: Variable `paused` is assigned an initial value
  Move the assignment to the initializer
  https://zpl.in/upgrades/error-004

contracts/v1/Ownable.sol:28: Contract `Ownable` has a constructor
  Define an initializer instead
  https://zpl.in/upgrades/error-001

contracts/util/Address.sol:186: Use of delegatecall is not allowed
  https://zpl.in/upgrades/error-002

Having reviewed these errors, none had any adversarial impact:

Nevertheless, it would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour.

Recommended Mitigation Steps

Where applicable, use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts.

L04: FiatTokenV1 / V2: Remove whenNotPaused modifier from cancelAuthorization() and decreaseAllowance() functions

Line References

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L409-L414

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L535-L541

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L420-L425

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L558-L564

Description

Just like how removeMinter() doesn’t have the whenNotPaused modifier, it would be very beneficial useful (eg. when a hack / rug pull occurs) to allow users to revoke allowances and cancel authorizations whilst having transfers paused.

Recommended Mitigation Steps

Remove the whenNotPaused modifiers for the cancelAuthorization() and decreaseAllowance() functions.

L05: FiatTokenV2: whitelist() should be unusable if contract is paused

Line References

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L645

Description

Should the contract be paused, it would be safer to prevent additional addresses from being whitelisted.

Recommended Mitigation Steps

Add the whenNotPaused modifier for the whitelist() function.

L06: Incorrect versioning of FiatTokenV2

Line References

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L111

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L114

Description

Since V2 is an upgrade of V1, its versioning should be updated to reflect the upgrade as well. It is important for the correct version to be reflected since it also part of the EIP712 data to be signed, which is used by EIP2612 and EIP3009 for fungible asset transfer authorizations. Authorizations given for outdated versions should rightfully be made invalid.

Recommended Mitigation Steps

DOMAIN_SEPARATOR = EIP712.makeDomainSeparator(name, "2");
VERSION = "2";

Non-Critical Findings

NC01: Bump OZ packages to ^4.5.0.

Line Reference

https://github.com/code-423n4/2022-02-jpyc#about-soliditys-version

https://github.com/OpenZeppelin/openzeppelin-contracts/commit/e192fac2769386b7d4b61a3541073ab47bb7723a

Description

The section referenced referred to a change in the UUPSUpgradeable and ERC1967Upgrade contracts that are only included in the latest package version of the OZ npm packages . However, the version specified in package-lock.json is 4.4.1, which does not include this change (and hence I assume was manually imported).

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

yarn install
yarn list @openzeppelin/contracts

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.

0xywzx commented 2 years ago

Thank you for your report. It is very helpful. ありがとうございます！

L01

Your solution looks good. We will change the code to use initializable for constructor.

L02

Since we don't use the hardhat plugin in the actual deployment, we din't use it in the test code. By the way, if you know how to deploy in a secure way, I would appreciate it if you could us me know.

L03

We have compliant USDC, so the code is looks like. However, 'functionDelegateCall()' was deleted from OZ upgradable contract, so we will check it.

L04

We didn't include 'whenNotPaused()' for whitelist because it doesn't have a significant impact on the contract itself.

L5 & L6

We agree with your advice.

thurendous commented 2 years ago

Incorrect versioning of FiatTokenV2

Fixed and thanks!

The change can be viewed here.