Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use of u64 for price_per_day and price_per_month limits handling tokens with 18 decimals #29

Open
c4-bot-9 opened this issue Oct 11, 2024 · 6 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working M-03 primary issue Highest quality submission among a set of duplicates 🤖_primary AI based primary recommendation 🤖_27_group AI based duplicate group recommendation selected for report This submission will be included/highlighted in the audit report sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@c4-bot-9
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-10-coded-estate/blob/main/contracts/codedestate/src/msg.rs#L168
https://github.com/code-423n4/2024-10-coded-estate/blob/main/contracts/codedestate/src/msg.rs#L111

Vulnerability details

Impact

The use of u64 for price_per_day and price_per_month prevents setting rental prices higher than approximately 18 tokens when using tokens with 18 decimals, potentially restricting landlords from setting appropriate rental prices in tokens with 18 decimals.

Proof-of-Concept

The SetListForShortTermRental and SetListForLongTermRental enums in the contract use u64 for price_per_day and price_per_month respectively, while the corresponding functions, setlistforshorttermrental and setlistforlongtermrental, also define these prices as u64.

File: contracts/codedestate/src/msg.rs
pub enum ExecuteMsg<T, E> {
    SetListForShortTermRental {
        token_id: String,
        denom: String,
        price_per_day: u64, // <-- here
        auto_approve: bool,
        available_period: Vec<String>,
        minimum_stay: u64,
        cancellation: Vec<CancellationItem>,
    },
    SetListForLongTermRental {
        token_id: String,
        denom: String,
        price_per_month: u64, // <-- here
        auto_approve: bool,
        available_period: Vec<String>,
        minimum_stay: u64,
        cancellation: Vec<CancellationItem>,
    },
}

File: contracts/codedestate/src/execute.rs
pub fn setlistforshorttermrental(
    &self,
    deps: DepsMut,
    env: Env,
    info: MessageInfo,
    token_id: String,
    denom: String,
    price_per_day: u64, // <--
    auto_approve: bool,
    available_period: Vec<String>,
    minimum_stay:u64,
    cancellation:Vec<CancellationItem>,
) -> Result<Response<C>, ContractError> {... snipped ...}

pub fn setlistforlongtermrental(
    &self,
    deps: DepsMut,
    env: Env,
    info: MessageInfo,
    token_id: String,
    denom: String,
    price_per_month: u64, // <--
    auto_approve: bool,
    available_period: Vec<String>,
    minimum_stay: u64,
    cancellation: Vec<CancellationItem>,
) -> Result<Response<C>, ContractError> {... snipped ...}

This poses a problem when dealing with tokens with 18 decimals, as the maximum value u64 can store is approximately 1.8446744e+19. In contrast, u128, which is used elsewhere in the contract for handling token amounts (e.g., info.funds[0].amount), can accommodate much larger values, fully supporting tokens with 18 decimals.

This mismatch can create issues when landlords attempt to specify rental prices. For example, when a token is worth $1 (with 18 decimals), the maximum price that can be set per day or month is capped at approximately 18 tokens ~ $18, potentially preventing landlords from setting appropriate rental prices for their properties.

Additionally, since Nibiru chain, the deployment chain for Coded Estate, supports custom denominated tokens, landlords may select tokens with 18 decimals as their payment token.

See: https://github.com/NibiruChain/nibiru/blob/main/x/tokenfactory/keeper/msg_server.go#L18-L41

Example Scenario:

  1. A landlord want to list their property on Coded Estate with a rental price of 20 tokens per day (20e18).
  2. The payment token used has 18 decimals.
  3. Since the rental price exceeds the u64 limit (2e19 > 1.8446744e+19), the landlord cannot list the property at the desired price.

Recommended Mitigations

  • Change from type u64 to u128 instead.

Assessed type

Context

@c4-bot-9 c4-bot-9 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Oct 11, 2024
c4-bot-8 added a commit that referenced this issue Oct 11, 2024
@c4-bot-11 c4-bot-11 added 🤖_27_group AI based duplicate group recommendation 🤖_primary AI based primary recommendation labels Oct 11, 2024
@OpenCoreCH
Copy link

Somewhat similar to #27 (u64 when withdrawing) and #16 (overflow for calculations because of high prices). Leaving them unduped for the moment

@c4-judge
Copy link
Contributor

OpenCoreCH marked the issue as primary issue

@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Oct 12, 2024
@blockchainstar12 blockchainstar12 added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Oct 14, 2024
@OpenCoreCH
Copy link

This can indeed limit the functionality of the protocol under reasonable assumptions. 18 decimal stablecoins are very common and it can be expected that some bridged asset will have 18 decimals. In such scenarios, a maximum price of $18 per month or day will be too low for many properties, meaning that these tokens cannot be used.

@c4-judge
Copy link
Contributor

OpenCoreCH marked the issue as selected for report

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Oct 15, 2024
@blockchainstar12
Copy link

blockchainstar12 commented Oct 18, 2024

We use tokens with 6 decimals in the platform, don't need to handle 18 decimals tokens

@C4-Staff C4-Staff added the M-03 label Oct 22, 2024
@blockchainstar12 blockchainstar12 removed the selected for report This submission will be included/highlighted in the audit report label Nov 5, 2024
@c4-sponsor
Copy link

@blockchainstar12 Sponsors can only use these labels: sponsor confirmed, sponsor disputed, sponsor acknowledged, critical.

@c4-sponsor c4-sponsor added the selected for report This submission will be included/highlighted in the audit report label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working M-03 primary issue Highest quality submission among a set of duplicates 🤖_primary AI based primary recommendation 🤖_27_group AI based duplicate group recommendation selected for report This submission will be included/highlighted in the audit report sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Development

No branches or pull requests

7 participants