EMC Finance & Bridge

CertiK15 Oct 2024Files named in the report

CertiK reviewed Edge Matrix Computing (EMC Finance & Bridge) on 15 Oct 2024: 50 findings, 1 critical; 34 marked fixed, 7 acknowledged, 9 open.

1 critical finding, fixed

Open the report Open Edge Matrix Computing’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings50

Statuses are the security firm’s own, from its report.

Severity	Open	Acknowledged	Fixed
Critical	0	0	1
High	0	4	4
Medium	0	3	3
Low	9	0	26

CriticalNFT can be stolen during bridgingFixed

```sol=68 function burnERC721(address tokenAddress, uint256 tokenID) internal { ERC721MinterBurnerPauser erc721 = ERC721MinterBurnerPauser(tokenAddress); erc721.burn(tokenID); } ``` `ERC721Safe.burnERC721(tokenAddress, tokenID)` is called by `ERC721Handler.deposit()` if `_burnList[tokenAddress]`. It successfully burns NFT if `ERC721Handler` was approved by the token owner to transfer that NFT. However, after the user calls `ERC721.approve(ERC721Handler, tokenID)` the malicious actor can call `Bridge.deposit()` for that tokenID. As a result, the NFT will be burnt in favor of the malicious actor. So, all the bridged burnable NFT can be stolen.

High`fundERC721()` and `fundERC20()` allow blocking oFixed

```solidity=21 function fundERC721(address tokenAddress, address owner, uint tokenID) public { IERC721 erc721 = IERC721(tokenAddress); erc721.transferFrom(owner, address(this), tokenID); } ``` `ERC721Safe.fundERC721()` and `ERC20Safe.fundERC20()` allow any person to transfer someone's tokens to the Handler address, if the user has approved the Handler contract. Since normal Bridge work requires the user to approve the Handler contract before tokens are deposited, this can be used by the malicious actor to block the Bridge functioning.

HighToo small `quorumVotes()` and `proposalThreshold(Fixed

EMCFI token has `totalSupply` of 100 million tokens. ```sol=9 function quorumVotes() public pure returns (uint) { return 400000e18; } // 400,000 = 4% of Finance /// @notice The number of votes required in order for a voter to become a proposer function proposalThreshold() public pure returns (uint) { return 100000e18; } // 100,000 = 1% of Finance ``` 400_000e18 tokens is only 0.4% of the `totalSupply`, that is too little for quorum. The comment is also misleading.

HighWrong Copy Size in assembly codeFixed

The copy size of calldata used in assembly of the function `ERC20Handler.deposit()` to copy the data of `recipientAddress` is mistakenly using the 0xE. Since the `recipientAddress` is placed at the end of calldata, so the correct copy size should be the `calldatasize - 0xE4`. `GenericHandler.deposit()` uses 0xC4 as a base metadata. However, the correct value is 0xA4 since the function has 4 arguments and the fifth is a dynamic data. `GenericHandler.executeProposal()` uses 0x64 as a base metadata. However, the correct value is 0x44 since the function has 1 argument and the second is a dynamic data. Wrong data parsing can lead to unexpected behavior including funds loss. The code comments are outdated and misleading.

HighWrong result of `getFinanceAddress()`Fixed

```sol=1334 function getFinanceAddress() public view returns (address) { // return 0xc00e94Cb662C3520282E6f5717214004A7f26888; } ``` `FinancetrollerG7.getFinanceAddress()` doesn't return a value.

Medium`_deposit2()` logic differs from `deposit()`Fixed

`Bridge.fallback()` permits deposits of native tokens. However: 1. Using `fallback()` instead of a function with a declared signature is not recommended. 2. The method approves the `erc20HandlerAddress` to transfer tokens, but subsequently uses `_resourceIDToHandlerAddress[resourceID]` as the handler. The `erc20HandlerAddress` is never validated and can be arbitrary. 3. The `fallback()` function can be invoked `whenPaused`, unlike `deposit()`. 4. Since `fallback()` uses `address(this)` as the sender/depositor, the Bridge's address is recorded in `_depositRecords` of the Handler instead of the `msg.sender`. 5. The logic within `fallback()` differs for testnet chain IDs as compared to `deposit()`. These issues can lead to unexpected behavior and potential security vulnerabilities.

MediumERC721 Token May Be Transferred To A Contract ThaFixed

The [EIP-721 standard](https://github.com/ethereum/EIPs/blob/78e2c297611f5e92b6a5112819ab71f74041ff25/EIPS/eip-721.md?plain=1#L103-L113) says the following about `transferFrom()`: ```solidity /// @notice Transfer ownership of an NFT -- THE CALLER IS RESPONSIBLE /// TO CONFIRM THAT `_to` IS CAPABLE OF RECEIVING NFTS OR ELSE /// THEY MAY BE PERMANENTLY LOST /// @dev Throws unless `msg.sender` is the current owner, an authorized /// operator, or the approved address for this NFT. Throws if `_from` is /// not the current owner. Throws if `_to` is the zero address. Throws if /// `_tokenId` is not a valid NFT. /// @param _from The current owner of the NFT /// @param _to The new owner /// @param _tokenId The NFT to transfer function transferFrom(address _from, address _to, uint256 _tokenId) external payable; ``` The function `lockERC721` transfers an ERC721 token to the input address via `transferFrom()`. This method does not check that the `recipient` is a contract, it is designed to support ERC721 tokens. If the `recipient` is a smart contract not designed to handle ERC721 tokens, they can become locked in the contract forever.

MediumIncorrect `blocksPerYear` constant across chainsFixed

The constant like `blocksPerYear` inside the contract is often used for calculation on a per-block basis. But since different chain has different block interval, this constant should be different across chains and a careful check is required on this. `blocksPerYear = 2102400` means one block per 15 seconds. Expected value for Ethereum chain. The EMC chain produces a block every 2 seconds. ```sol=21 function votingPeriod() virtual public pure returns (uint) { return 17280; } // ~3 days in blocks (assuming 15s blocks) ``` Voting period and the comment are incorrect.

Low`CentrifugeAsset` is never usedFixed

`CentrifugeAsset` is never used. `store()` is unprotected and can be called by anyone.

Low`ERC721Safe` should be `abstract`Fixed

Contracts `HandlerHelpers`, `ERC721Safe`, and `ERC20Safe` are not supposed to be deployed. That is why they should be marked as `abstract`. ```sol=63 function withdraw(address tokenAddress, address recipient, uint256 amountOrTokenID) external virtual override {} ``` Empty function should be removed from the abstract class.

Show 10 more

Low`GenericHandler` should inherit from `IDepositExeFixed

The `GenericHandler` contract implements the `IDepositExecute` interface but does not inherit from it.

Low`ProposalStatus` is casted to `uint`Fixed

```sol=411 require(uint(proposal._status) <= 1, "proposal already passed/executed/cancelled"); ... if (uint(proposal._status) == 0) { ``` Casting `ProposalStatus` to uint is redundant and potentially misleading. We recommend directly using the enum values for clarity: * `require(proposal._status == ProposalStatus.Inactive || proposal._status == ProposalStatus.Active)` * `if (proposal._status == ProposalStatus.Inactive)`

LowBad code adaptationFixed

`FinancePriceOracle.financeareStrings()` and `FinanceLens.financeareStrings()` are supposed to be `compareStrings()`. ```sol=46 require(whitelisted[msg.sender], "Asset not whitelisted"); ``` The function is not called by the asset. ```sol=7 * @title Compound's CErc20Immutable Contract * @notice CTokens which wrap an EIP-20 underlying and are immutable * @author Compound ``` The comment is outdated. ```sol=174 case 32 { // This is a financeliant ERC-20 ``` "financeliant" is supposed to be "compliant".

LowDeprecated Usage of `Counters.sol`Fixed

The linked contracts import and use OpenZeppelin's `Counters` contract. OpenZeppelin has deprecated the usage of the `Counters` contract: https://github.com/OpenZeppelin/openzeppelin-contracts/issues/4233. `Counters` is also unused.

LowIncorrect Role ManagementFixed

The direct usage of `grantRole` and `revokeRole` functions from the [AccessControl](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/AccessControl.sol) contract in functions with additional access control logic can lead to conflicts or redundant permissions. This is due to the requirement that the caller must have the admin role for the corresponding role to invoke these functions successfully. If the caller's role permissions are not properly set and synchronized with the role administration logic, it can result in inconsistencies and unintended access rights.

LowInefficient Loop Over Memory Array in Contract `Fixed

The `constructor` performs a loop over its supplied `initialRelayers` memory array while retrieving the length of the array over each iteration, which is inefficient: ```solidity for (uint256 i; i < initialRelayers.length; i++) { ``` The `transferFunds()` function performs a loop over its supplied `addrs` memory array while retrieving the length of the array over each iteration, which is inefficient: ```solidity for (uint i = 0; i < addrs.length; i++) { ```

LowInefficient Memory ParameterFixed

One or more parameters with `memory` data location are never modified in their functions and those functions are never called internally within the contract. Thus, their data location can be changed to `calldata` to avoid the gas consumption copying from calldata to memory. ```solidity=44 function mint(address to, uint256 tokenId, string memory _data) public { ``` `mint` has memory location parameters: `_data`.

LowLack Validation for Array LengthFixed

There is no validation to check whether `addrs.length` and `amounts.length` are the same

LowLocal Variable ShadowingFixed

Arguments `name`, `symbol`, `baseURI` are shadowing state fields in the base class. This means that when the contract accesses the variable by its name, it will use the one defined locally, not the one defined in the other place. The use of the variable may lead to unexpected results and unintended behavior. ```solidity=24 constructor(string memory name, string memory symbol, string memory baseURI) public ERC721(name, symbol) { ```

LowMissing Check for Duplicate Role Assignment in BrFixed

The loop that assigns the `RELAYER_ROLE` to `initialRelayers` lacks a check to prevent reassigning the role to the same address multiple times. This could lead to an inaccurate count of relayers, as the `_totalRelayers` variable is incremented without verifying whether the address has already been assigned the role.

Show 10 more

LowMissing ChecksFixed

### HandlerHelpers - The function `setResource()` does not check that `_resourceIDToContractAddress[resourceID]` and `_contractAddressToResourceID[contractAddress]` are not already set. - The function `setBurnable()` does not check if `_burnList[contractAddress]` is already true. ### GenericHandler - The function `setResource()` does not check that `_resourceIDToContractAddress[resourceID]`, `_contractAddressToResourceID[contractAddress]`, ` _contractAddressToDepositFunctionSignature[contractAddress]`, and `_contractAddressToExecuteFunctionSignature[contractAddress]` are not already set. - The functions `deposit()` and `executeProposal()` make a low level call to the `contractAddress`. While these are set via privileged roles, these contracts may self destructor or be set accidentally to EOA's, in which case the low level call will return true without executing any functionality. We recommend adding a check that the `contractAddress` is a contract. ### Bridges - The function `adminChangeRelayerThreshold()` does not check that the input `newThreshold` is greater than or equal to some minimum. In particular, the threshold should always be at least one, as a proposal must be proposed (which yields one yes vote). - The function `setHandlerAdress()` does not check if the input `_handlerAddress` is different from the zero address. - The function `adminChangeFee()` does not check that the input `newFee` is within some bounds. In particular, if a fee should always be charged the input should be greater than zero and the fee may not want to exceed a theoretical maximum amount. - In the `Bridge.executeProposal()` function, the potential expiration of a proposal is not checked. If `proposal._status == ProposalStatus.Passed` and `block.number - proposal._proposedBlock > _expiry`. A relayer can decide to execute a proposal. - In the `Bridge.cancelProposal()` function, the potential execution of a proposal is not checked. If `block.number - proposal._proposedBlock > _expiry`. A relayer or the admin can decide to cancel an already executed proposal. - The functions `adminSetResource()` and `adminSetGenericResource()` don't check that the `handlerAddress` was not yet set for `resourceID`.

LowMissing Zero Address ValidationFixed

Addresses are not validated before assignment or external calls, potentially allowing the use of zero addresses and leading to unexpected behavior or vulnerabilities. For example, transferring tokens to a zero address can result in a permanent loss of those tokens. ```solidity=167 erc20HandlerAddress = _handlerAddress; ``` - `_handlerAddress` is not zero-checked before being used. ```solidity=122 WETH_ADDRESS = wethAddress; ``` - `wethAddress` is not zero-checked before being used. ```solidity=122 WEMC_ADDRESS = wemcAddress; ``` - `wemcAddress` is not zero-checked before being used. ```solidity=50 _bridgeAddress = bridgeAddress; ``` - `bridgeAddress` is not zero-checked before being used. ```solidity=81 _bridgeAddress = bridgeAddress; ``` - `bridgeAddress` is not zero-checked before being used. ```solidity=57 _bridgeAddress = bridgeAddress; ``` - `bridgeAddress` is not zero-checked before being used.

LowOptimization in `Bridge.executeProposal()`Fixed

In `Bridge.executeProposal()`, the following checks: ```solidity=495 require(proposal._status != ProposalStatus.Inactive, "proposal is not active"); require(proposal._status == ProposalStatus.Passed, "proposal already transferred"); ``` are both checking the status of a proposal, since a proposal cannot be `Inactive` and `Passed` the first **require** statement is redundant. Moreover, a proposal not passed can be `Inactive`, `Active`, `Executed` or `Cancelled`, therefore the error message `"proposal already transferred"` is not always accurate.

LowPotential Inconsistency In `_totalRelayers`Fixed

In the contract `Bridge.sol`, state variable `_totalRelayers` is supposed to reflect the numbers of current relayers in the contract. However, it is possible to update the relayer without accurately updating `_totalRelayers` leading to inconsistencies.

LowPotential Underflow/OverflowFixed

In `Bridge.adminRemoveRelayer()`, `_totalRelayers` is decremented. Due to potential inconsistency between `_totalRelayers` and the real number of relayers, an underflow can occur.

LowRedundant codeFixed

- `FinanceLens.Double` declaration is never used. ```sol=768 struct Exp { uint mantissa; } mapping(address => FinanceMarketState) public financeSupplyState; mapping(address => mapping(address => uint)) public financeSupplierIndex; uint224 public constant financeInitialIndex = 1e36; ``` The fields and the `FinanceMarketState` declaration are not used.

LowRisk of Losing Admin Access If The Input New AdmiFixed

If the address `newAdmin` to be set as the admin role is the same as the current admin `msg.sender`, it will result in the admin role losing its administration authorization over the contract. This vulnerability arises because the function first grants the admin role to the new owner's address and then revokes the admin role from the current admin's address. However, suppose the new owner's address is the same as the current one. In that case, the admin role will be revoked, leading to the admin losing the authorization for contract administration.

LowTyposFixed

There are typos and outdated comments: - "custoday" is supposed to be "custody" - "delegatecall" is supposed to be "call" ```sol=457 @notice Executes a deposit proposal that is considered passed using a specified handler contract. @notice Only callable by relayers when Bridge is not paused. ``` - In fact the `cancelProposal()` cancels the proposal, not executes. It can also be called by admin, not only relayers. It can be called even if Bridge is paused. - "amonuts" is supposed to be "amounts"

LowUnnecessary modifierFixed

In functions `Bridge.renounceAdmin()`, `Bridge.adminAddRelayer()`, `Bridge.adminRemoveRelayer()` the use of `onlyAdmin` modifier is redundant since the same check is made by functions inherited from `AccessControl`.

LowUnsafe Type CastingFixed

Type casting refers to changing a variable of one data type into another. The code contains an unsafe cast between integer types, which may result in unexpected truncation of the recipient address. ```sol=151 uint8(lenDestinationRecipientAddress), uint8(destinationChainID), ``` ```sol=175 recipientAddress := mload(add(destinationRecipientAddress, 0x20)) ``` The `destinationRecipientAddress` is implicitly truncated to `bytes20`. No data size check is performed.

Show 4 more

LowUsage of `transfer()` for sending EtherFixed

It is not recommended to use Solidity's `transfer()` and `send()` functions for transferring native tokens, since some contracts may not be able to receive the funds. Those functions forward only a fixed amount of gas (2300 specifically) and the receiving contracts may run out of gas before finishing the transfer. Also, EVM instructions' gas costs may increase in the future. Thus, some contracts that can receive now may stop working in the future due to the gas limitation. ```solidity=36 msg.sender.transfer(amount); ``` - `WETH.withdraw` uses `transfer()`. ```solidity=34 payable(msg.sender).transfer(amount); ``` - `WEMC.withdraw` uses `transfer()`. ```solidity=515 addrs[i].transfer(amounts[i]); ``` - `Bridge.transferFunds` uses `transfer()`.

LowUse `_safeMint` instead of `_mint`Fixed

According to openzepplin's ERC721, the use of `_mint` is discouraged, use _safeMint whenever possible.

LowUseless `_noVotes` in Struct `Proposal`Fixed

When the relayers vote for the proposals, they can only vote `yes`. So it is useless to have a `_noVotes`. ```solidity struct Proposal { bytes32 _resourceID; bytes32 _dataHash; address[] _yesVotes; address[] _noVotes; ProposalStatus _status; uint256 _proposedBlock; } ```

LowUser-Defined GettersFixed

The linked functions are equivalent to the compiler-generated getter functions for the respective variables.