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

1 stars 0 forks source link

QA Report #44

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

QA (Low and NC) report to JPYC by PeritoFlores

Transfer Ownership pattern incomplete implementation

In function _transferOwnership#Ownable.sol

function _transferOwnership(address newOwner) internal virtual { @audit using transfer ownership
    address oldOwner = _owner;
    _owner = newOwner;
    emit OwnershipTransferred(oldOwner, newOwner);
}

It seems that you tried to implement two-step ownership transfer to make it safer. It is a good idea to add the function acceptOwnership() to complete the pattern.

Rescuer uninialitized at FiatTokenV2

In the function initialize()#FiatTokenV2.sol rescuer in never initialized so it is zero at deployment time.

Consider calling updateRescuer() inside initialize()

Lack emit event after rescueERC20()

In rescuer.sol the function rescueERC20() lacks an event emit.

function rescueERC20(
    IERC20 tokenContract,
    address to,
    uint256 amount
) external onlyRescuer {
    tokenContract.safeTransfer(to, amount);  @audit  emit event  
}

Consider adding it

Typo error

At EIP3009.sol#L227 there is a typo error

`"FEIP3009: authorization is not yet valid"  `

It says "FEIP" instead of "EIP"

Consider using inheritance when using upgradeable contracts

When using upgradeable contracts it is recommended that the new version inherits from previous. One of the reason for that is that you avoid storage collision. A minimum error in the order of parameters on the new version could break the protocol.

Maybe you can consider for the next version something like

contract FiatTokenV3 is FiatTokenV2

retocrooman commented 2 years ago

Lack emit event after rescueERC20()

There is no problem because the Transfer Event of the specified ERC20 fires.

Typo error

Thank you for noticing the typo!

Consider using inheritance when using upgradeable contracts

We agree. We also think that if it can be inherited, and it should be inherited. For this time, the reason why we did not inherit FiatTokenV1 in FiatTokenV2 is because we want to add the "whitelist" check into the existing functions.

thurendous commented 2 years ago

At EIP3009.sol#L227 there is a typo error

Typo fixed and thanks!

Final change can be viewed here.