Incorrect use of u64 for arg amount in withdrawtolandlord can cause withdrawal failure

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-10-coded-estate/blob/main/contracts/codedestate/src/execute.rs#L1786-L1796
https://github.com/code-423n4/2024-10-coded-estate/blob/main/contracts/codedestate/src/msg.rs#L156-L162

Vulnerability details

Impact

The use of u64 for token amount in the withdrawtolandlord function can lead to failed withdrawals when handling tokens with 18 decimals, limiting the landlord’s ability to withdraw their entitled funds if the amount exceeds the maximum value of u64.

Proof-of-Concept

In the withdrawtolandlord function, the token amount is defined as a u64 value. However, this can cause issues when handling tokens with 18 decimals, as the u64 data type can only store values up to approximately 1.8446744e+19 ~ 18 token of token with 18 decimals. This limit is significantly lower than what is supported by u128, which is used in other parts of the contract to handle token amount, such as info.funds[0].amount is a U128 type.

File: contracts/codedestate/src/execute.rs
pub fn withdrawtolandlord(
    &self,
    deps: DepsMut,
    env: Env,
    info: MessageInfo,
    token_id: String,
    tenant: String,
    renting_period: Vec<String>,
    amount:u64,
    address:String
) -> Result<Response<C>, ContractError> { ... snipped ...}

File: contracts/codedestate/src/msg.rs
pub enum ExecuteMsg<T, E> {
    ...snipped...
    ...
    WithdrawToLandlord {
        token_id: String,
        tenant: String,
        renting_period: Vec<String>,
        amount: u64,
        address: String,
    },
    ...snipped...
    ...

This discrepancy between the data types can create an issue. If the token amount owed to the landlord exceeds the maximum value supported by u64, the landlord will not be able to withdraw their entitled funds through the withdrawtolandlord function.

This is problematic as the Nibiru chain, the chain on which Coded Estate is deployed, supports custom denominated tokens, and users can specify tokens with 18 decimals as their payment currency. For example, if a large payment is made in such a token, the landlord would be unable to withdraw the full amount due to the limitations of the u64 type.

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

Example Scenario:

1. A user pays deposit using a token with 18 decimals.
2. The total deposit amount exceeds the maximum value of u64 (~18 tokens for 18-decimal tokens).
3. When the landlord tries to withdraw their funds via the withdrawtolandlord function, the function fails because the u64 type cannot accommodate the large token amount.
4. As a result, the landlord is unable to withdraw their funds, leading to loss of access to legitimate payments.

Recommended Mitigations

• Change type of amount to u128 for consistency with other parts in the system.

Assessed type

Context

[OpenCoreCH]

Does not seem to be a significant problem to me at first sight, if such a scenario would ever happen, withdrawal should be possible with multiple calls

[c4-judge]

OpenCoreCH marked the issue as primary issue

[OpenCoreCH]

Unlike #29, this does not impact the functionality of the protocol significantly. While a larger data type could still be a good idea here, the owner can still withdraw funds by splitting up the withdrawals into multiple calls

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)

[nnez]

Hi @OpenCoreCH

I might have overstate the impact in the report (unable to withdraw funds).
However, I still believe that this issue should be classified as Medium severity.

This bug does impact the protocol's functionality.

Consider the scenario wherein the required deposit is 5_000e18 tokens.
In this scenario, the token owner would have to split their transaction into 5_000e18 / (2^64-1) = 271.05 → 272 separate transactions in order to withdraw all the funds.

That’s a lot of transactions and this is just for one long-term rental. An individual token owner’s can have more than one property and they can have more than one active long-term rental with deposit to withdraw.

Instead of paying gas for one transaction, users unnecessarily have to pay 200x+ more of gas in order to withdraw the full amount.

Additionally, 5_000e18 is just an arbitrary reasonable number for a 18 decimals token worth $1, the problem could get worse with a larger amount of tokens. For example, 50_000e18 of $0.1 would take 2711 transactions to withdraw the full amount.

[OpenCoreCH]

That's true, 5,000$ is a reasonable amount for such a protocol to handle. Potentially even low, with business apartments in cites like Zurich that often cost 5,000$ per month, so you could easily have 30,000$ for a longer rental. 18 decimal stable coins are also very common.

So it is not that unlikely that a landlord would have to perform ~1,632 calls for one withdrawal. On the one hand, this would be of course very cumbersome (especially if the UI did not support this), but it can also become pretty expensive (if one call were roughly $1, this would be an almost 5% fee on top). So based on that, Medium is indeed more appropriate.

[c4-judge]

This previously downgraded issue has been upgraded by OpenCoreCH

[c4-judge]

OpenCoreCH marked the issue as selected for report

[blockchainstar12]

we update u64 to u128 to address this issue