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

possible to bid for tokens that are not for sale #11

Closed
c4-bot-8 opened this issue Oct 9, 2024 · 3 comments
Closed

possible to bid for tokens that are not for sale #11

c4-bot-8 opened this issue Oct 9, 2024 · 3 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-23 🤖_11_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue upgraded by judge Original issue severity upgraded from QA/Gas by judge

Comments

@c4-bot-8
Copy link
Contributor

c4-bot-8 commented Oct 9, 2024

Lines of code

https://github.com/code-423n4/2024-10-coded-estate/blob/97efb35fd3734676f33598e6dff70119e41c7032/contracts/codedestate/src/execute.rs#L646

Vulnerability details

Impact

possible to bid for tokens that are not listed for sale. This is because setbidtobuy() does not check for if the property token.sell.isListed is true when bidding.

Proof Of Concept

For a token to be sold, the token owner must call setlListForSell() to set the price, denomination, auto-approve and the isListed value which indicates if a token/rental token is now listed on the market. Because the values for all these are provided by the user, a user may call setlListForSell() to set price, denomination and auto_approve but not set isListed yet or set it to false because rental token owner is not just ready to sell yet but wants to give other buyers in the market an idea of the pricing or for any other reason the token owner sees fit.

Now in this case, if this is done, if pricing and denom is set but isListed is false, buyers will still be able to bid via setbidtobuy() even though the rental token/property is not on the market. This means buyers are bidding for a property that is not for sale.
setbidtobuy() only checks for if the token.sell.denom isd set and equal to demonination provided by the caller/buyer and token.sell.price is lower than or equal to the amount provided by the caller/buyer. It does not check the most important property of the token.sell struct which is token.sell.isListed.

https://github.com/code-423n4/2024-10-coded-estate/blob/97efb35fd3734676f33598e6dff70119e41c7032/contracts/codedestate/src/execute.rs#L666-L691

        if position == -1 {
            if info.funds[0].denom != token.sell.denom {
                return Err(ContractError::InvalidDeposit {});
            }
            if info.funds[0].amount
                < Uint128::from(token.sell.price)
            {
                return Err(ContractError::InsufficientDeposit {});
            }

            if token.sell.auto_approve {
                // update the approval list (remove any for the same spender before adding)
                let expires = Expiration::Never {  };
                token.approvals.retain(|apr| apr.spender != info.sender);
                let approval = Approval {
                    spender: info.sender.clone(),
                    expires,
                };
                token.approvals.push(approval);
                
            }
            let bid = Bid {
                address: info.sender.to_string(),
                offer:info.funds[0].amount,
            };
            token.bids.push(bid);
        }

in the snippet above we can see that the logic does not check for if the token is listed or not before adding a new bid to the token.bids array.

The logic then proceeds to save the modified token struct to storage with the newly added bid. see below.

https://github.com/code-423n4/2024-10-coded-estate/blob/97efb35fd3734676f33598e6dff70119e41c7032/contracts/codedestate/src/execute.rs#L694-L720

        else {
            // update the approval list (remove any for the same spender before adding)
            token.bids.retain(|item| item.address != info.sender);
        }

        self.tokens.save(deps.storage, &token_id, &token)?;
        if position != -1 && (amount > Uint128::from(0u64)) {
            Ok(Response::new()
            .add_attribute("action", "setbidtobuy")
            .add_attribute("sender", info.sender.clone())
            .add_attribute("token_id", token_id)
            .add_message(BankMsg::Send {
                to_address: info.sender.to_string(),
                amount: vec![Coin {
                    denom: token.sell.denom,
                    amount: amount,
                }],
            }))
        }
        else {
            Ok(Response::new()
            .add_attribute("action", "setbidtobuy")
            .add_attribute("sender", info.sender)
            .add_attribute("token_id", token_id))
        }

Recommened Mitigation

add an if else statement which reverts if token.sell.isListed is equal to false.

Assessed type

Context

@c4-bot-8 c4-bot-8 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 9, 2024
c4-bot-3 added a commit that referenced this issue Oct 9, 2024
@c4-bot-12 c4-bot-12 added the 🤖_11_group AI based duplicate group recommendation label Oct 11, 2024
@c4-judge
Copy link
Contributor

OpenCoreCH marked the issue as duplicate of #23

@c4-judge
Copy link
Contributor

OpenCoreCH marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Oct 15, 2024
@blockchainstar12 blockchainstar12 added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Oct 15, 2024
@c4-judge c4-judge added 3 (High Risk) Assets can be stolen/lost/compromised directly upgraded by judge Original issue severity upgraded from QA/Gas by judge and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Oct 20, 2024
@c4-judge
Copy link
Contributor

OpenCoreCH changed the severity to 3 (High Risk)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-23 🤖_11_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue upgraded by judge Original issue severity upgraded from QA/Gas by judge
Projects
None yet
Development

No branches or pull requests

4 participants