Pichi Finance Marketplace audit report

ABSTRACT

Dedaub was commissioned to perform a security audit of Pichi Finance’s Marketplace contracts that offers a marketplace for points via NFTs, ensuring secure transactions with no collateral required.

BACKGROUND

The Pichi Finance protocol allows users to create ERC-6551 (tokenbound) accounts to earn points. Each account is owned by an NFT and can be traded through the Pichi marketplace by trading the NFT.

The protocol consists of three main contracts:

The PichiWalletNFT contract allows the minting of NFTs at a price set by the owner of the contract.
The PichiHelper contract uses the PichiWalletNFT contract to simultaneously mint NFTs and create the tokenbound accounts controlled by these NFTs. It also provides functionality for depositing whitelisted tokens into a wallet controlled by the owner of an NFT, potentially against a deposit fee.
The PichiMarketplace contract allows the trading of NFTs from whitelisted collections for various whitelisted currencies. A buyer can either execute a listing which was signed off-chain by a seller, or alternatively a seller can accept an offer which was signed off-chain by a buyer, using the EIP-712 signature scheme. A user can cancel their orders individually or in bulk by updating a nonce. The contract also has functionality to validate off-chain the above off-chain orders, as well as to handle the transfer of NFTs and settle payments.

SETTING & CAVEATS

This audit report mainly covers the contracts of the michi-contracts repository of the Pichi Finance protocol at commit d9cee365b59ed7798a8f958cd8b23574c1e08b9a.

Two auditors worked on the codebase for 2 days on the following contracts:

src/
├── PichiHelper.sol
├── PichiWalletNFT.sol
├── interfaces/
│   ├── IPichiMarketplace.sol
│   └── IPichiWalletNFT.sol
├── libraries/
│   ├── OrderTypes.sol
│   └── SignatureAuthentication.sol
└── marketplace/
    └── PichiMarketplace.sol

Prior to the audit the Pichi team communicated to the audit team that there is a known “issue” in the PichiHelper contract allowing users to bypass the deposit fee. The Pichi team has indicated that for the moment the deposit fee is set to 0. If in the future it is increased, Pichi will reward users with additional benefits in case they choose to pay it.

The audit’s main target is security threats, i.e., what the community understanding would likely call “hacking”, rather than the regular use of the protocol. Functional correctness (i.e. issues in “regular use”) is a secondary consideration. Typically it can only be covered if we are provided with unambiguous (i.e. full-detail) specifications of what is the expected, correct behavior. In terms of functional correctness, we often trusted the code’s calculations and interactions, in the absence of any other specification. Functional correctness relative to low-level calculations (including units, scaling and quantities returned from external protocols) is generally most effectively done through thorough testing rather than human auditing.

PROTOCOL-LEVEL CONSIDERATIONS

TBA trustless sales can be vulnerable to order misrepresentation

Protocol Level Consideration | Status: INFO

Token bound accounts are designed to hold different kinds of tokens and thus In order to enable trustless sales of TBAs, decentralized marketplaces such as PichiMarketplace will need to implement safeguards against fraudulent behavior by malicious account owners. Consider the following potential vulnerability taken from the Security Considerations section of ERC6551:

Alice owns an ERC-721 token X, which owns token bound account Y.
Alice deposits 10 ETH into account Y.
Bob offers to purchase token X for 11 ETH via a decentralized marketplace, assuming he will receive the 10 ETH stored in account Y along with the token.
Alice withdraws 10 ETH from the token bound account, and immediately accepts Bob’s offer.
Bob receives token X, but account Y is empty.

PichiMarketplace does not implement any mitigations for such fraudulent behavior on the contract level. Pichi implements the following alerts in the application’s UI (frontend) to inform and protect users of its marketplace:

A TBA owner who wants to list their TBA for sale on the PichiMarketplace, will be warned by Pichi’s UI prompts that they should withdraw all the tokens owned by the TBA before listing it for sale.
On the buyer side, there are prompts to inform the buyer that the owner of the TBA they are bidding on can withdraw tokens before accepting the offer and that they should make their offer solely on the Pichi points accrued in that TBA.

VULNERABILITIES & FUNCTIONAL ISSUES

This section details issues affecting the functionality of the contract. Dedaub generally categorizes issues according to the following severities, but may also take other considerations into account such as impact or difficulty in exploitation:

Issue resolution includes “dismissed” or “acknowledged” but no action taken, by the client, or “resolved”, per the auditors.

CRITICAL SEVERITY

[No Critical Severity Issues]

HIGH SEVERITY

[No High Severity Issues]

MEDIUM SEVERITY

[No Medium Severity Issues]

LOW SEVERITY

Missing check in initialize function of PichiMarketplace

Low | Status: RESOLVED

PichiMarketplace::setMarketplaceFee checks that the newFee <= 1000, but this is not enforced in PichiMarketplace::initialize for the marketplaceFee\_ parameter.

Missing check in constructor of PichiHelper

Low | Status: RESOLVED

PichiHelper’s setDepositFee function checks that the newDepositFee <= 500, but this is not enforced on the _depositFee parameter of the constructor.

Occurrence of owner mismatch when using PichiHelper::createWallet requires manual intervention to unblock

Low | Status: RESOLVED

PichiHelper::createWallet checks whether the owner of the newWallet != msg.sender and reverts if this is the case. Our understanding is that this can occur if another user coincidentally generates the same address when creating his own tokenbound account (potentially when using a different NFT contract).

However if this call reverts, the currentIndex of the NFT contract will not advance, because _mint will also revert, resulting in this error being reproduced during subsequent calls. Hence the wallet creation mechanism will be stuck until someone goes into the NFT contract and manually mints a dummy NFT to skip the problematic index.

Fee percentage’s precision should be hardcoded

Low | Status: RESOLVED

The variables PichiHelper::feePrecision and PichiMarketplace::precision, which represent the fee percentage’s precision (at 100%), can be initialized to an arbitrary value, possibly to something even smaller than the actual fee percentage. A definitive value should be chosen and hardcoded to increase the contracts’ clarity.

PichiMarketplace implementation contract’s initializer is not disabled

Low | Status: RESOLVED

The initializer function of the PichiMarketplace implementation contract should be disabled as suggested by the OpenZeppelin best practices. This can be done by calling the Initializable::_disableInitializers() function in the PichiMarketplace constructor.

constructor() {
    _disableInitializers();
}

The version of the OZ libraries used is not the latest

Low | Status: ACKNOWLEDGED

The protocol uses version 4.6 of the OpenZeppelin libraries instead of the latest standard that is 5.0.2.

CENTRALIZATION ISSUES

It is often desirable for DeFi protocols to assume no trust in a central authority, including the protocol’s owner. Even if the owner is reputable, users are more likely to engage with a protocol that guarantees no catastrophic failure even in the case the owner gets hacked/compromised. We list issues of this kind below. (These issues should be considered in the context of usage/deployment, as they are not uncommon. Several high-profile, high-value protocols have significant centralization threats.)

PichiMarketplace is upgradeable

Centralization | Status: ACKNOWLEDGED

The PichiMarketplace contract inherits from the OpenZeppelin OwnableUpgradeable contract meaning that the protocol owners are able to upgrade its implementation.

PichiHelper’s owner controls the ERC-6551 account creation

Centralization | Status: ACKNOWLEDGED

PichiHelper’s purpose is to ease the process of minting PichiWalletNFTs and of the creation of ERC-6551 accounts owned by the minted NFTs. This means that the PichiHelper owner defines the ERC-6551 registry, proxy and implementation used for the tokenbound account creation. The owner can also upgrade the proxy and implementation used for the creation at any time via the updateImplementation and updateProxy functions.

OTHER / ADVISORY ISSUES

This section details issues that are not thought to directly affect the functionality of the project, but we recommend considering them.