search

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

1 stars 0 forks source link

QA Report #52

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Title

Using address(0) does not make sense

Vulnerability details

Impact

Using address(0) does not make sense for the event. It should use a more explicit address such as msg.sender. Developers cannot figure out where Transfer event is emitted from if it uses address(0).

Proof of Concept

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

emit Transfer(address(0), _to, _amount);

Tools Used

Static code analysis

Recommended Mitigation steps

Use either msg.sender or address(this) depending on the specification of the contract.

emit Transfer(msg.sender, _to, _amount);

Title

Inconsistent usage of whenNotPaused modifier

Vulnerability details

Impact

configureMinter function has whenNotPaused modifier while removeMinter function does not have whenNotPaused.

Inconsistent usage of whenNotPaused would cause unexpected behavior.

Proof of Concept

removeMinter function does not use whenNotPaused modifier https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L344-L348

    function removeMinter(address minter)
        external
        onlyMasterMinter
        returns (bool)
    {

Tools Used

Static code analysis

Recommended Mitigation steps

Add whenNotPaused modifier at removeMinter function to be consistent with other functions.

    function removeMinter(address minter)
        external
        whenNotPaused
        onlyMasterMinter
        returns (bool)
    {

Title

Inconsistency of messages in require function

Vulnerability details

Impact

The require check has the prefix of FiatToken: or ERC20: at FiatTokenV1.sol. If require check is done under FiatTokenV1.sol, it should use one of them.

Proof of Concept

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

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

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

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

Tools Used

Static code analysis

Recommended Mitigation steps

Use FiatToken: for the consistency

Title

Naming inconsistency at variables

Vulnerability details

Impact

FiatTokenV1.sol does not have naming consistency on variables. It may confuse developers without standardized naming styles.

Proof of Concept

Function arguments

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

function mint(address _to, uint256 _amount)

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

function burn(uint256 _amount)

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

function updateMasterMinter(address _newMasterMinter) external onlyOwner {

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

modifier checkWhitelist(address _account, uint256 _value) {

function isWhitelisted(address _account) external view returns (bool) {

function whitelist(address _account) external onlyWhitelister {

function unWhitelist(address _account) external onlyWhitelister {

function updateWhitelister(address _newWhitelister) external onlyOwner {

State variables

Only totalSupply_ contains _. Other state variables do not contain _. https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L58

uint256 internal totalSupply_ = 0;

Event

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

    event Whitelisted(address indexed _account);
    event UnWhitelisted(address indexed _account);

Tools Used

Static code analysis

Recommended Mitigation steps

Set the standards for variables naming styles. It seems that other function arguments do not have _ in their prefixes, so removing _ looks consistent.

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

function mint(address to, uint256 amount)

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

function burn(uint256 amount)

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

function updateMasterMinter(address newMasterMinter) external onlyOwner {

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

modifier checkWhitelist(address account, uint256 value) {

function isWhitelisted(address account) external view returns (bool) {

function whitelist(address account) external onlyWhitelister {

function unWhitelist(address account) external onlyWhitelister {

function updateWhitelister(address newWhitelister) external onlyOwner {

Of course, change remove _ from their prefixes inside these functions as well.

As for state variables, other state variables do not use _ in their suffixes, so removing _ seems legit. https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L58

uint256 internal totalSupply = 0;

Arguments used in the Event can follow the other patterns as well. https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L70-L71

    event Whitelisted(address indexed account);
    event UnWhitelisted(address indexed account);

Title

Is minters variable needed?

Vulnerability details

Impact

It looks like the role of minters and minterAllowed looks similar, and it may be able to combine them into one. Using extra state variables will increase gas usage.

Proof of Concept

State variable definitions

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

    mapping(address => bool) internal minters;
    mapping(address => uint256) internal minterAllowed;

Relevant functions

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

    function configureMinter(address minter, uint256 minterAllowedAmount)
        external
        whenNotPaused
        onlyMasterMinter
        returns (bool)
    {
        minters[minter] = true;
        minterAllowed[minter] = minterAllowedAmount;
        emit MinterConfigured(minter, minterAllowedAmount);
        return true;
    }

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

    function removeMinter(address minter)
        external
        onlyMasterMinter
        returns (bool)
    {
        minters[minter] = false;
        minterAllowed[minter] = 0;
        emit MinterRemoved(minter);
        return true;
    }

When removing minter role from account, it sets 0 at minterAllowed variable. minterAllowed[minter] = 0 So technically minters variable seems not needed, and without minters it can maintain the same logic. By removing minters variable, it can save followings:

Tools Used

manual check

Recommended Mitigation steps

Title

The original file should follow the recommended order of functions

Vulnerability details

Impact

Solidity defines the style guide for the order of functions. https://docs.soliditylang.org/en/v0.8.12/style-guide.html?highlight=coding%20style#order-of-functions

it is not efficient to reorder codes of libraries such as OpenZeppelin, but at least FiatTokenV1.sol can follow the style guide.

Following the style guide of solidity increases the readability.

Proof of Concept

https://docs.soliditylang.org/en/v0.8.12/style-guide.html?highlight=coding%20style#order-of-functions

Tools Used

Static code analysis

Recommended Mitigation steps

Follow the style guide of solidity

Title

Remove functionDelegateCall function from Address library

Vulnerability details

As for the usage of delegatecall at the logic contract, OpenZeppalin manual explains as follows.

A similar effect can be achieved if the logic contract contains a delegatecall operation. If the contract can be made to delegatecall into a malicious contract that contains a selfdestruct, then the calling contract will be destroyed.

Because of this reason, OpenZeppelin's upgradeable version (AddressUpgradeable.sol) does not include functionDelegateCall function.

Impact

Proof of Concept

Address.sol#L169-L188 contains functionDelegateCall function which should not be included at the logic contract.

function functionDelegateCall(address target, bytes memory data) internal returns (bytes memory) {
    return functionDelegateCall(target, data, "Address: low-level delegate call failed");
}
...
function functionDelegateCall(
    address target,
    bytes memory data,
    string memory errorMessage
) internal returns (bytes memory) {
    require(isContract(target), "Address: delegate call to non-contract");

    (bool success, bytes memory returndata) = target.delegatecall(data);
    return verifyCallResult(success, returndata, errorMessage);
}

Tools Used

Static analysis

Recommended Mitigation steps

Either remove functionDelegateCall function from Address.sol or use AddressUpgradeable.sol instead of Address.sol.

thurendous commented 2 years ago

Thank you very much for submitting the issues!

Using address(0) does not make sense

In terms of this issue, because minter mints tokens from thin air and we think this kind of event is quite standard. We are not going to make a cahnge.

Naming inconsistency at variables

This is very helpful and we will fix the variable name e.g. totalSupply and some other arguments naming, e.g. _to -> to After a while we found out totalSupply_ was set because it has a totalSupply() function, so we are not going to fix this.

Inconsistency of messages in require function

This is valid too.

Remove functionDelegateCall function from Address library

About this one we are not sure yet. Need to check. duplicate of #35

thurendous commented 2 years ago

Use FiatToken: for the consistency

This issue is the same as #26 and it is fixed.

thurendous commented 2 years ago

Final change can be viewed here.