IOP _ ThunderNFT 34967 - [Smart Contract - Insight] Insights Report

Insights Report

Submitted on Mon Sep 02 2024 00:10:04 GMT-0400 (Atlantic Standard Time) by @Blockian for IOP | ThunderNFT

Report ID: #34967

Report type: Smart Contract

Report severity: Insight

Target: https://github.com/ThunderFuel/smart-contracts/tree/main/contracts-v1/royalty_manager

Impacts:

• Contract fails to deliver promised returns, but doesn't lose value

• Insight Report

Description

Thunder Exchange

Insights Report

Description

This report compiles a series of insights gathered during the review process. While these observations are not eligible for rewards, I think they might be useful for improving the protocol.

For your convenience, I have consolidated them into a single report.

Redundant use of only_owner

In several contract functions such as transfer_ownership and renounce_ownership, there is a redundancy where the only_owner check is called multiple times. For instance, in the asset_manager contract:

    #[storage(read, write)]
    fn transfer_ownership(new_owner: Identity) {
        storage.owner.only_owner();
        storage.owner.transfer_ownership(new_owner);
    }

However, the transfer_ownership function already includes an only_owner check:

    #[storage(read, write)]
    pub fn transfer_ownership(self, new_owner: Identity) {
        self.only_owner();

        self.write(Ownership::initialized(new_owner));

        log(OwnershipTransferred {
            new_owner,
            previous_owner: msg_sender().unwrap(),
        });
    }

This results in unnecessary storage reads and increased gas costs for users. Removing the redundant check would optimize the contract’s performance.

Royalty Fee Receiver can be Zero Identity

The Royalty Fee Receiver address can be set to zero by any collection registering royalty information. Although this does not have an immediate functional impact, allowing funds to be directed to a zero address is generally undesirable and could lead to future issues. It is recommended to implement a validation check to prevent this scenario.

Missing pub in ExtraParams

In order_types, the ExtraParams struct is currently private, which limits its usability. This restricts users of the protocol who wish to create valid orders by importing the library.

Making the ExtraParams struct public would enhance the protocol’s usability.

Proof of concept

POC

Here is a simple POC for setting the Royalty Fee Receiver to Zero Identity

1. First, we need to remove this else statement in the register_royalty_info function:

    #[storage(read, write)]
    fn register_royalty_info(
        collection: ContractId,
        receiver: Identity,
        fee: u64
    ) {
        let ownable = abi(Ownable, collection.into());

        if (ownable.owner().is_some()) {
            let caller = msg_sender().unwrap();
            let collection_owner = ownable.owner().unwrap();
            require(caller == collection_owner, RoyaltyManagerErrors::CallerMustBeOwnerOrAdmin);
        } else if (ownable.admin().is_some()) {
            let caller = msg_sender().unwrap();
            let collection_admin = ownable.admin().unwrap();
            require(caller == collection_admin, RoyaltyManagerErrors::CallerMustBeOwnerOrAdmin);
        } else { // this else
            // revert(111)
        }

We need to remove it for the POC simplicity, otherwise we need to set an Ownable contract and so on. 2. Run this simple POC code:

contract;

use interfaces::{
  royalty_manager_interface::*,
};

#[test()]
fn test_wrong_log() {
  pub const ZERO_B256 = 0x0000000000000000000000000000000000000000000000000000000000000000;
  pub const ZERO_CONTRACT_ID = ContractId::from(ZERO_B256);

  let royalty_manager = abi(RoyaltyManager, royalty_manager::CONTRACT_ID);
  royalty_manager.initialize();
  royalty_manager.register_royalty_info(ContractId::from(royalty_manager::CONTRACT_ID), Identity::ContractId(ZERO_CONTRACT_ID), 0); // worked fine
}