JPYC contest - TerrierLover's results

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:

• Removal of the definition of minters state variable
• Removal of the logic setting true at minters variables in configureMinter function
• Removal of the logic setting false at minters variable in removeMinter function

Tools Used

manual check

Recommended Mitigation steps

• Remove minters state variables
• Change the logic to use minterAllowed state variables where minters was previously used Specifically following codes need to be changed: https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L116 https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L176

---

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.

---

Title

Use != 0 instead of > 0 to have gas efficiency

Vulnerability details

Impact

Using != 0 can increase the gas cost. It should use > 0 instead.

Proof of Concept

FiatTokenV1.sol#L136

require(_amount > 0, "FiatToken: mint amount not greater than 0");

FiatTokenV1.sol#L368

require(_amount > 0, "FiatToken: burn amount not greater than 0");

_amount is uint256, so it does not need to check minus.

Tools Used

Static code analysis

Recommended Mitigation steps

FiatTokenV1.sol#L136

require(_amount != 0, "FiatToken: mint amount not greater than 0");

FiatTokenV1.sol#L368

require(_amount != 0, "FiatToken: burn amount not greater than 0");

---

Title

Reordering the state variables can reduce slot size

Vulnerability details

Impact

There is an opportunity to reduce the slot size by reordering the state variables.

Proof of Concept

FiatTokenV1.sol#L51-L54

    uint8 public decimals;
    string public currency;
    address public masterMinter;
    bool internal initialized;

Tools Used

Used test/storageSlot/storageSlots.behavior.js to check that the slot size can be reduced from 520 to 519.

Recommended Mitigation steps

• Reorder the state variable
• Update the relevant test: test/storageSlot/storageSlots.behavior.js

---

Title

Adding checked directive can save gas at FiatTokenV1.sol

Vulnerability details

Impact

Solidity v0.8 has default over and under-flown checks. Hence, the unchecked directives can be used for arithmetic operations that will never over or under-flown. Please refer to the Solidity manual

Proof of Concept

Following codes can be wrapped by unchecked directives since the required check before these lines assert that the line will not be less than 0.

FiatTokenV1.sol#L146

require(
    _amount <= mintingAllowedAmount,
    "FiatToken: mint amount exceeds minterAllowance"
);

totalSupply_ = totalSupply_ + _amount;
balances[_to] = balances[_to] + _amount;
minterAllowed[msg.sender] = mintingAllowedAmount - _amount; // <--- this line can be wrapped by unchecked

FiatTokenV1.sol#L276

require(
    value <= allowed[from][msg.sender],
    "ERC20: transfer amount exceeds allowance"
);
_transfer(from, to, value);
allowed[from][msg.sender] = allowed[from][msg.sender] - value; // <--- this line can be wrapped by unchecked

FiatTokenV1.sol#L316

require(
    value <= balances[from],
    "ERC20: transfer amount exceeds balance"
);

balances[from] = balances[from] - value; // <--- this line can be wrapped by unchecked

FiatTokenV1.sol#L371-L372

require(balance >= _amount, "FiatToken: burn amount exceeds balance");

totalSupply_ = totalSupply_ - _amount;     // <--- this line can be wrapped by unchecked
balances[msg.sender] = balance - _amount;  // <--- this line can be wrapped by unchecked

FiatTokenV1.sol#L449

require(
    decrement <= allowed[owner][spender],
    "ERC20: decreased allowance below zero"
);
_approve(owner, spender, allowed[owner][spender] - decrement); // <--- this line can be wrapped by unchecked

Tools Used

Static code analysis

Recommended Mitigation steps

The above-mentioned codes can be written as follows to use unchecked directives:

FiatTokenV1.sol#L146

require(
    _amount <= mintingAllowedAmount,
    "FiatToken: mint amount exceeds minterAllowance"
);

totalSupply_ = totalSupply_ + _amount;
balances[_to] = balances[_to] + _amount;
unchecked {
    minterAllowed[msg.sender] = mintingAllowedAmount - _amount;
}

FiatTokenV1.sol#L276

require(
    value <= allowed[from][msg.sender],
    "ERC20: transfer amount exceeds allowance"
);
_transfer(from, to, value);
unchecked {
    allowed[from][msg.sender] = allowed[from][msg.sender] - value;
}

FiatTokenV1.sol#L316

require(
    value <= balances[from],
    "ERC20: transfer amount exceeds balance"
);
unchecked {
    balances[from] = balances[from] - value;
}

FiatTokenV1.sol#L371-L372

require(balance >= _amount, "FiatToken: burn amount exceeds balance");
unchecked {
    totalSupply_ = totalSupply_ - _amount;
    balances[msg.sender] = balance - _amount;
}

FiatTokenV1.sol#L449

require(
    decrement <= allowed[owner][spender],
    "ERC20: decreased allowance below zero"
);
unchecked {
    _approve(owner, spender, allowed[owner][spender] - decrement);
}

---