QA Report

[code423n4]

C4-001 : transferOwnership/updateRescuer should be two step process

Impact - LOW

"FiatTokenV2.sol" 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.

Rescuer update progress should be two step procedure.

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 "https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/Ownable.sol#L60" and "https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/Rescuable.sol#L72"
2. The contracts have many onlyOwner function.
3. The contract is inherited from the Ownable which includes transferOwnership.

Tools Used

None

Recommended Mitigation Steps

Implement zero address check and Consider implementing a two step process where the owner nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

C4-002 : Missing Conditional Check In the Allowance

Impact - LOW

During the code review, It has been observed that If the allowance is given maximum uint. The check should be nice to have check if the current allowance is maximum.

Proof of Concept

1. Navigate to "https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L281"
2. The max allowance check has not been checked on the function.

Ensure that is the required checks are compatible with Openzeppelin ERC20.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L336

Tools Used

None

Recommended Mitigation Steps

Implement the following check in the related function.

 if (allowed[from][msg.sender != type(uint256).max)

Reference

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L336

C4-003 : Use of ecrecover is susceptible to signature malleability

Impact - LOW

The ecrecover function is used EIP-3009 to recover the address from the signature. The built-in EVM precompile ecrecover is susceptible to signature malleability which could lead to replay attacks (references: https://swcregistry.io/docs/SWC-117, https://swcregistry.io/docs/SWC-121 and https://medium.com/cryptronics/signature-replay-vulnerabilities-in-smart-contracts-3b6f7596df57).

Proof of Concept

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/util/ECRecover.sol#L70

Tools Used

None

Recommended Mitigation Steps

Consider using OpenZeppelin’s ECDSA library (which prevents this malleability) instead of the built-in function.

C4-004 : Missing events for admin/rescuer only functions that change critical parameters and function flows

Impact LOW

The admin only functions that change critical parameters should emit events. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services. The alternative of directly querying on-chain contract state for such changes is not considered practical for most users/usages.

Missing events and timelocks do not promote transparency and if such changes immediately affect users’ perception of fairness or trustworthiness, they could exit the protocol causing a reduction in liquidity which could negatively impact protocol TVL and reputation.

There are owner functions that do not emit any events in the contracts.

Proof of Concept

1. Navigate to the following contract functions.

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

See similar High-severity H03 finding OpenZeppelin’s Audit of Audius (https://blog.openzeppelin.com/audius-contracts-audit/#high) and Medium-severity M01 finding OpenZeppelin’s Audit of UMA Phase 4 (https://blog.openzeppelin.com/uma-audit-phase-4/)

Tools Used

None

Recommended Mitigation Steps

Add events to all admin/privileged functions that change critical parameters.

C4-005 : Front-runnable Initializers

Impact - LOW

All contract initializers were missing access controls, allowing any user to initialize the contract. By front-running the contract deployers to initialize the contract, the incorrect parameters may be supplied, leaving the contract needing to be redeployed.

Proof of Concept

1. Navigate to the following contracts.

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

2. initialize functions does not have access control. They are vulnerable to front-running.

Tools Used

Manual Code Review

Recommended Mitigation Steps

While the code that can be run in contract constructors is limited, setting the owner in the contract's constructor to the msg.sender and adding the onlyOwner modifier to all initializers would be a sufficient level of access control.

C4-006 : Missing zero-address check in the setter functions and constructor

Impact - LOW

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/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L79

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

Tools Used

Code Review

Recommended Mitigation Steps

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

C4-007 : Add a timelock to rescueERC20 Function

Impact - LOW

To give more trust to users: functions that set key/critical variables and export ERC20 functions should be put behind a timelock.

Proof of Concept

1. Navigate to the following contract function.

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

Tools Used

Code Review

Recommended Mitigation Steps

Add a timelock on the rescue operation.

C4-008 : Upgrade pragma to at least 0.8.4

Impact - NON CRITICAL

Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.

The advantages of versions 0.8.* over <0.8.0 are:

• Safemath by default from 0.8.0 (can be more gas efficient than library based safemath.)
• Low level inliner : from 0.8.2, leads to cheaper runtime gas. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls.
• Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs.
• Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

Proof of Concept

1. Navigate to the following contracts.

2. The contest repository contracts contain floating pragma ^0.8.0. The contracts pragma version is not compatible with documentation.

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/proxy/ERC1967Proxy.sol

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/proxy/Proxy.sol

Tools Used

Code Review

Recommended Mitigation Steps

Consider to upgrade pragma to at least 0.8.4.

C4-009 : Events not indexed

Impact - NON CRITICAL

None of the events in swap.sol are indexed, so it is not easy for off-chain tools to efficiently filter these events. I would recommend adding indices to the following contract events.

Proof of Concept

1. Navigate to the following contract functions.

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

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

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

Tools Used

Code Review

Recommended Mitigation Steps

Added indices as described above.

[retocrooman]

C4-002 : Missing Conditional Check In the Allowance

I would like to know the reason why the maximum value is not good.

C4-003 : Use of ecrecover is susceptible to signature malleability

We will check the details.

[thurendous]

if (allowed[from][msg.sender != type(uint256).max)

Fixed and thanks!

Final change can be viewed here.