search

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

1 stars 0 forks source link

QA Report #26

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

QA Report

Table of Contents:

Foreword

The code is annotated at multiple places with //@audit comments to pinpoint the issues. Please, pay attention to them for more details.

Summary

proxy/ERC1967Proxy.sol:4:pragma solidity ^0.8.0;
proxy/Proxy.sol:4:pragma solidity ^0.8.0;
upgradeability/draft-IERC1822.sol:4:pragma solidity ^0.8.0;
upgradeability/ERC1967Upgrade.sol:4:pragma solidity 0.8.11;
upgradeability/UUPSUpgradeable.sol:4:pragma solidity 0.8.11;
util/Address.sol:4:pragma solidity ^0.8.0;
util/Context.sol:4:pragma solidity 0.8.11;
util/ECRecover.sol:26:pragma solidity 0.8.11;
util/EIP712.sol:25:pragma solidity 0.8.11;
util/IERC20.sol:4:pragma solidity ^0.8.0;
util/SafeERC20.sol:4:pragma solidity ^0.8.0;
util/StorageSlot.sol:3:pragma solidity ^0.8.0;
v1/AbstractFiatTokenV1.sol:25:pragma solidity 0.8.11;
v1/Blocklistable.sol:25:pragma solidity 0.8.11;
v1/EIP2612.sol:25:pragma solidity 0.8.11;
v1/EIP3009.sol:25:pragma solidity 0.8.11;
v1/EIP712Domain.sol:25:pragma solidity 0.8.11;
v1/FiatTokenV1.sol:25:pragma solidity 0.8.11;
v1/Ownable.sol:4:pragma solidity 0.8.11;
v1/Pausable.sol:26:pragma solidity 0.8.11;
v1/Rescuable.sol:25:pragma solidity 0.8.11;
v2/FiatTokenV2.sol:25:pragma solidity 0.8.11;
v2/FiatTokenV2test.sol:3:pragma solidity 0.8.11;

File: UUPSUpgradeable.sol

modifier onlyProxy()

32:     modifier onlyProxy() {
33:         require(
34:             address(this) != __self,
35:             "Function must be called through delegatecall" // missing "UUPSUpgradeable: " prefix (in all of the solution;, a prefix is used)
36:         );
37:         require(
38:             _getImplementation() == __self,
39:             "Function must be called through active proxy" // missing "UUPSUpgradeable: " prefix
40:         );
41:         _;
42:     }

Missing UUPSUpgradeable: prefix on revert string Function must be called through delegatecall

Missing UUPSUpgradeable: prefix on revert string Function must be called through active proxy

In the whole solution, revert strings are prefixed with the contract's name. Here, it's missing.

File: Blocklistable.sol

function isBlocklisted()

64:     /**
65:      * @dev Checks if account is blocklisted
66:      * @param _account The address to check //@audit missing @return bool
67:      */
68:     function isBlocklisted(address _account) external view returns (bool) {
69:         return blocklisted[_account];
70:     }

Missing comment @return bool

File: FiatTokenV1.sol

Storage

Related data should be grouped in a struct

For maps that use the same key value: having separate fields is error prone (like in case of deletion or future new fields).
In this contract, those 2 maps use the same key: 

File: FiatTokenV1.sol
59:     mapping(address => bool) internal minters;
60:     mapping(address => uint256) internal minterAllowed;

Proof: 

  333:         minters[minter] = true;
  334:         minterAllowed[minter] = minterAllowedAmount;
...
  349:         minters[minter] = false;
  350:         minterAllowed[minter] = 0;

I'd suggest these 2 related data get grouped in a struct, let's name it MinterInfo: 

struct MinterInfo {  
    bool minters;  
    uint256 minterAllowed;  
}

And it would be used as a state variable in this manner: 

mapping(address => MinterInfo) minterInfo;

It would be less error-prone, more readable, and it would be possible to delete all related fields with a simple delete minterInfo[address].

function initialize()

Front-Runnable initialize()

Even if the contract is Ownable and the default owner is the deployer:

File: Ownable.sol
28:     constructor() {
29:         _transferOwnership(_msgSender());
30:     }

there's no access-control implemented on initialize():

File: FiatTokenV1.sol
40: contract FiatTokenV1 is
41:     Ownable,
...
068:     function initialize(
069:         string memory tokenName,
070:         string memory tokenSymbol,
071:         string memory tokenCurrency,
072:         uint8 tokenDecimals,
073:         address newMasterMinter,
074:         address newPauser,
075:         address newBlocklister,
076:         address newOwner
077:     ) public {
078:         require(!initialized, "FiatToken: contract is already initialized");
079:         require(
080:             newMasterMinter != address(0),
081:             "FiatToken: new masterMinter is the zero address"
082:         );
083:         require(
084:             newPauser != address(0),
085:             "FiatToken: new pauser is the zero address"
086:         );
087:         require(
088:             newBlocklister != address(0),
089:             "FiatToken: new blocklister is the zero address"
090:         );
091:         require(
092:             newOwner != address(0),
093:             "FiatToken: new owner is the zero address"
094:         );
095: 
096:         name = tokenName;
097:         symbol = tokenSymbol;
098:         currency = tokenCurrency;
099:         decimals = tokenDecimals;
100:         masterMinter = newMasterMinter;
101:         pauser = newPauser;
102:         blocklister = newBlocklister;
103:         _transferOwnership(newOwner);
104:         blocklisted[address(this)] = true;
105:         DOMAIN_SEPARATOR = EIP712.makeDomainSeparator(name, "1");
106:         CHAIN_ID = block.chainid;
107:         NAME = name;
108:         VERSION = "1";
109:         initialized = true;
110:     }

Furthermore, as this method calls L103: _transferOwnership(newOwner);, it's possible for the front-runner to claim ownership, as this is calling the default implementation without access-control:

File: Ownable.sol
56:     /**
57:      * @dev Transfers ownership of the contract to a new account (`newOwner`).
58:      * Internal function without access restriction.
59:      */
60:     function _transferOwnership(address newOwner) internal virtual {
61:         address oldOwner = _owner;
62:         _owner = newOwner;
63:         emit OwnershipTransferred(oldOwner, newOwner);
64:     }

I suggest adding the onlyOwner modifier to the initialize() method. There's exactly the same issue on FiatTokenV2.sol

function minterAllowance()

Missing comment @return uint256

163:     /**
164:      * @dev Get minter allowance for an account
165:      * @param minter The address of the minter //@audit missing @return uint256
166:      */
167:     function minterAllowance(address minter) external view returns (uint256) {
168:         return minterAllowed[minter];
169:     }

function isMinter()

Missing comment @return bool

171:     /**
172:      * @dev Checks if account is a minter
173:      * @param account The address to check //@audit missing @return bool
174:      */
175:     function isMinter(address account) external view returns (bool) {
176:         return minters[account];
177:     }

function balanceOf()

Missing comment @return uint256

202:     /**
203:      * @dev Get token balance of an account
204:      * @param account address The account //@audit missing @return uint256
205:      */
206:     function balanceOf(address account)
207:         external
208:         view
209:         override
210:         returns (uint256)
211:     {
212:         return balances[account];
213:     }

File: FiatTokenV2.sol

Storage

Related data should be grouped in a struct

The same suggestion as in FiatTokenV1.sol applies here for grouping minters and minterAllowed in a struct.

function initialize()

Front-Runnable initialize()

Same as FiatTokenV1.sol: Front-Runnable initialize()

function minterAllowance()

Missing comment @return uint256

File: FiatTokenV2.sol
170:     /**
171:      * @dev Get minter allowance for an account
172:      * @param minter The address of the minter //@audit missing @return uint256
173:      */
174:     function minterAllowance(address minter) external view returns (uint256) {
175:         return minterAllowed[minter];
176:     }

function isMinter()

Missing comment @return bool

File: FiatTokenV2.sol
178:     /**
179:      * @dev Checks if account is a minter
180:      * @param account The address to check //@audit missing @return bool
181:      */
182:     function isMinter(address account) external view returns (bool) {
183:         return minters[account];
184:     }

function balanceOf()

Missing comment @return uint256

File: FiatTokenV2.sol
209:     /**
210:      * @dev Get token balance of an account
211:      * @param account address The account //@audit missing @return uint256
212:      */
213:     function balanceOf(address account)
214:         external
215:         view
216:         override
217:         returns (uint256)
218:     {
219:         return balances[account];
220:     }

function _approve()

File: FiatTokenV2.sol
248:     function _approve(
249:         address owner,
250:         address spender,
251:         uint256 value
252:     ) internal override {
253:         require(owner != address(0), "ERC20: approve from the zero address"); //@audit replace prefix from "ERC20:" to "FiatToken:"
254:         require(spender != address(0), "ERC20: approve to the zero address"); //@audit replace prefix from "ERC20:" to "FiatToken:"
255:         allowed[owner][spender] = value;
256:         emit Approval(owner, spender, value);
257:     }

Wrong revert string prefix (1)

ERC20: approve from the zero address should be FiatToken: approve from the zero address

Wrong revert string prefix (2)

ERC20: approve to the zero address should be FiatToken: approve to the zero address

function transferFrom()

Wrong revert string prefix

File: FiatTokenV2.sol
280:         require(
281:             value <= allowed[from][msg.sender],
282:             "ERC20: transfer amount exceeds allowance" //@audit replace prefix from "ERC20:" to "FiatToken:"
283:         );

ERC20: transfer amount exceeds allowance should be FiatToken: transfer amount exceeds allowance

function _transfer()

File: FiatTokenV2.sol
314:     function _transfer(
315:         address from,
316:         address to,
317:         uint256 value
318:     ) internal override {
319:         require(from != address(0), "ERC20: transfer from the zero address"); //@audit replace prefix from "ERC20:" to "FiatToken:"
320:         require(to != address(0), "ERC20: transfer to the zero address"); //@audit replace prefix from "ERC20:" to "FiatToken:"
321:         require(
322:             value <= balances[from],
323:             "ERC20: transfer amount exceeds balance" //@audit replace prefix from "ERC20:" to "FiatToken:"
324:         );
325: 
326:         balances[from] = balances[from] - value;
327:         balances[to] = balances[to] + value;
328:         emit Transfer(from, to, value);
329:     }

Wrong revert string prefix (1)

ERC20: transfer from the zero address should be FiatToken: transfer from the zero address

Wrong revert string prefix (2)

ERC20: transfer to the zero address should be FiatToken: transfer to the zero address

Wrong revert string prefix (3)

ERC20: transfer amount exceeds balance should be FiatToken: transfer amount exceeds balance

function _decreaseAllowance()

Wrong revert string prefix

File: FiatTokenV2.sol
280:         require(
281:             value <= allowed[from][msg.sender],
282:             "ERC20: transfer amount exceeds allowance" //@audit replace prefix from "ERC20:" to "FiatToken:"
283:         );

ERC20: decreased allowance below zero should be FiatToken: decreased allowance below zero

modifier onlyWhitelister()

File: FiatTokenV2.sol
610:     modifier onlyWhitelister() {
611:         require(
612:             msg.sender == whitelister,
613:             "Whitelistable: caller is not the whitelister" //@audit replace prefix from "Whitelistable:" to "FiatToken:"
614:         );
615:         _;
616:     }

Wrong revert string prefix

Whitelistable: caller is not the whitelister should be FiatToken: caller is not the whitelister

modifier checkWhitelist()

File: FiatTokenV2.sol
623:     modifier checkWhitelist(address _account, uint256 _value) {
624:         if (_value > 100000 * 10 ** 18) {
625:             require(
626:                 whitelisted[_account],
627:                 "Whitelistable: account is not whitelisted"//@audit replace prefix from "Whitelistable:" to "FiatToken:"
628:             );
629:         }
630:         _;
631:     }

Wrong revert string prefix

Whitelistable: account is not whitelisted should be FiatToken: account is not whitelisted

function isWhitelisted()

File: FiatTokenV2.sol
633:     /**
634:      * @dev Checks if account is whitelisted
635:      * @param _account The address to check //@audit missing @return bool
636:      */
637:     function isWhitelisted(address _account) external view returns (bool) {
638:         return whitelisted[_account];
639:     }

Missing comment @return bool

function updateWhitelister()

Wrong revert string prefix

Whitelistable: new whitelister is the zero address should be FiatToken: new whitelister is the zero address

File: Ownable.sol

function transferOwnership()

transferOwnership should be two step process

47:     /**
48:      * @dev Transfers ownership of the contract to a new account (`newOwner`).
49:      * Can only be called by the current owner.
50:      */
51:     function transferOwnership(address newOwner) public virtual onlyOwner {
52:         require(newOwner != address(0), "Ownable: new owner is the zero address");
53:         _transferOwnership(newOwner); //@audit should be 2 step
54:     }
55: 
56:     /**
57:      * @dev Transfers ownership of the contract to a new account (`newOwner`).
58:      * Internal function without access restriction.
59:      */
60:     function _transferOwnership(address newOwner) internal virtual {
61:         address oldOwner = _owner;
62:         _owner = newOwner;
63:         emit OwnershipTransferred(oldOwner, newOwner);
64:     }

The current ownership transfer process 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.

I suggest 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.

thurendous commented 2 years ago

Wrong revert string prefix (1)

This one is valid and we will fix it!

thurendous commented 2 years ago

Wrong revert string prefix (1)

Fixed

thurendous commented 2 years ago

The pragmas used are not the same everywhere:

fixed

thurendous commented 2 years ago

Final change can be viewed here.