search

code-423n4 / 2024-04-lavarage-findings

2 stars 2 forks source link

There is no way to close unused PDAs, leading to the SOL deposited into them for their rent exemption being lost forever #14

Open c4-bot-7 opened 4 months ago

c4-bot-7 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-04-lavarage/blob/9e8295b542fb71b2ba9b4693e25619585266d19e/libs/smart-contracts/programs/lavarage/src/context/borrow.rs#L8-L10 https://github.com/code-423n4/2024-04-lavarage/blob/9e8295b542fb71b2ba9b4693e25619585266d19e/libs/smart-contracts/programs/lavarage/src/context/create_trading_pool.rs#L8-L14 https://github.com/code-423n4/2024-04-lavarage/blob/9e8295b542fb71b2ba9b4693e25619585266d19e/libs/smart-contracts/programs/lavarage/src/context/node_wallet.rs#L7-L11

Vulnerability details

Impact

Users of the Lavarage program can not close the PDAs created by them that are no longer used, leading to them permanently loosing the SOL that they deposited into those in order to make them rent exempt

Proof of Concept

The current creation of the position, trading pool and node wallet PDAs is done in such a way that the signers of the functions that initiate those actions are enforced to deposit the minimum rent exemption SOL amount into those accounts, so that they can be stored on the Solana blockchain, without being prematurely closed. This is achieved by specifying the payer value on the #account attributes that initialize the accounts:

    #[account(init, payer = trader, space = 8+170, seeds = [b"position", trader.key().as_ref(), trading_pool.key().as_ref(), random_account_as_id.key().as_ref()],
    bump)]
    pub position_account: Account<'info, Position>,

This is a pretty standard procedure, and it works just as it should. However, there is a problem with it. There is no way to close the user created PDAs once they do not need to be used anymore. Since the PDAs are actually owned by the program that they were created from, what this means is that the rent exemption SOL that the users deposited into them when creating them will become lost forever. To put things into perspective, the minimal rent exemption amount for 64 bytes of storage is ~0.0013 SOL or ~0.18 USD. Given that there will likely be thousands of PDAs that will be created through Lavarage, we can see that the lost funds will clearly not be a negligible amount. This will be especially true for borrow position accounts (which use 178 bytes of storage each), as those will always become practically useless once they are repaid or liquidated.

Tools Used

Manual review

Recommended Mitigation Steps

Implement a mechanism that lets the creators of unused position, trading pool and node wallet accounts close them. You can find an example of how this can be achieved in this resource from the Solana Cookbook.

Assessed type

Other

c4-sponsor commented 4 months ago

piske-alex (sponsor) acknowledged

piske-alex commented 4 months ago

We will consider adding the close function. But currently there is no mechanism to store history. After implementing the history storage we will let users close the positions.

c4-judge commented 4 months ago

alcueca marked the issue as satisfactory

c4-judge commented 4 months ago

alcueca marked the issue as selected for report

koolexcrypto commented 4 months ago

Hi @Arabadzhiew

I believe this should be a QA because the rent is too small (0.0013 SOL) . If a user want to close the account, it should be done through the program. This means, transaction fee will be paid too. Furthermore, In general, closing accounts in Solana if not done with care, will lead to undesired security issues because closed accounts can be reinitialised again. Please note that, this lost dust amount can still be reclaimed by adding a functionality later to the program (I don't recommend it though) . The developer has up to 2 years to do that.

According to the severity categorization on C4:

Loss of fees as Low Loss of dust amounts are QA

https://docs.code4rena.com/awarding/judging-criteria/severity-categorization#loss-of-fees-as-low

Loss of yield as high Loss of dust amounts are QA https://docs.code4rena.com/awarding/judging-criteria/severity-categorization#loss-of-yield-as-high

CC: @alcueca

Arabadzhiew commented 4 months ago

Hi @koolexcrypto

I believe this should be a QA because the rent is too small (0.0013 SOL) .

I want to put special emphasis on this sentence from my report: Given that there will likely be thousands of PDAs that will be created through Lavarage, we can see that the lost funds will clearly not be a negligible amount

If a user want to close the account, it should be done through the program. This means, transaction fee will be paid too.

The transaction fees on Solana are usually < $0.01, so it will be profitable for users to close their unused positions, even if they only execute one close instruction per transaction.

Furthermore, In general, closing accounts in Solana if not done with care, will lead to undesired security issues because closed accounts can be reinitialised again.

Obviously, that is true. The biggest concern is the values of the closed accounts not being zeroed-out, but the article that I have linked to in the recommendation section describes the risks of exactly that, and how it can be avoided.

Please note that, this lost dust amount can still be reclaimed by adding a functionality later to the program (I don't recommend it though) . The developer has up to 2 years to do that.

This is assuming that the program is configured to be upgradable in the first place.

alcueca commented 4 months ago

Given the amount lost per user is 18x the cost of recovering it if a fix would be implemented, I don't think this is a dust amount.

Dust usually refers to amounts so small that are unprofitable to deal with.

Arabadzhiew commented 4 months ago

To be crystal clear, the storage used per position PDA is 178 bytes, so the cost will be ~0.0021 SOL or ~0.31 USD for each one :)

c4-judge commented 4 months ago

alcueca marked the issue as not selected for report

c4-judge commented 4 months ago

alcueca changed the severity to QA (Quality Assurance)

c4-judge commented 4 months ago

alcueca marked the issue as grade-a

Arabadzhiew commented 3 months ago

@alcueca

I am bringing this issue up one last time. Each position PDA costs $0.31 in order to be made rent exempt, while the transaction fees on Solana usually cost < $0.01.

Solana offers some of the cheapest transaction fees in the cryptocurrency market, typically costing between $0.003 and $0.005. (reference article published in February 2024)

And regarding the definition of a dust amount in the Web3 space - A dust amount is universally recognised as an amount of funds that is worth less than the tx fee cost required for spending it. Source 1 Source 2 Source 3

This means that what qualifies as dust and what doesn't is entirely dependent on the blockchain that the protocol in question is deployed on. Using your $1 heuristic as an example, while that amount does qualify as a non-dust amount of chains such as Solana, Polygon, Arbitrum and others that have very cheap transaction fees, it will also absolutely not qualify as a such on chains with expensive transaction fees such as Ethereum Mainnet. So saying that "$1 is the set in stone border for determining what is dust and what is not" is simply not correct, no matter how you look at it.

And while I absolutely don't want repeat myself for the nth time, I will say once again for one last time that this issue has a strong accumulative factor, since position PDAs will be created on each instance of the most common action that the protocol is supposed to be used for - borrowing.

alcueca commented 3 months ago

When setting the financial limit of what makes a Medium, I did think about transaction costs, and how they are negligible in most chains.

We can't declare that every financial loss is a Medium, as we would do if we take that metric. As a user, I don't care about amounts less than $1. I'm not going to use the product any less, I'm not going to complain in a support ticket.

If we look at it from the perspective of the sponsor, this issue will have a minimal impact, because no one will complain, and the users won't find it any less competitive than its peers.

Arabadzhiew commented 3 months ago

When setting the financial limit of what makes a Medium, I did think about transaction costs, and how they are negligible in most chains.

So you did think about the transaction costs and still decided to put a fixed value on what is dust and what is not? As I've said above, the universal definition of a dust amount is an amount of funds that is worth less than the tx fee cost required for spending it, so you can't just say that dust is any amount of funds worth less than a dollar. And actually, in the PJQA discussion of this contest even you yourself initially stated the following: I don't think that $0.18 USD is dust on solana nowadays.

We can't declare that every financial loss is a Medium, as we would do if we take that metric. As a user, I don't care about amounts less than $1. I'm not going to use the product any less, I'm not going to complain in a support ticket.

Actually, on Solana it is a standard practice for most protocols to have a PDA closing functionality. This is why the rent exemption amount is enforced in the first place - so that the users of the chain are encouraged to free up the storage space that is no longer needed by them. So if I was a user of the protocol who knew that this is how the chain is supposed to be used and that I should actually be able to get those assets back, I definitely would care, especially if I was a regular user of the protocol (as I would most certainly accumulate significant losses if I was such an user).

If we look at it from the perspective of the sponsor, this issue will have a minimal impact, because no one will complain, and the users won't find it any less competitive than its peers.

Will the sponsor really not care though? Even if we assume that the users of the protocol won't mind their individual losses, the protocol itself will at some point accumulate losses in the magnitudes of hundreds, if not thousand of dollars that could otherwise be claimed by the sponsor if there was functionality for letting them do so. So I don't think that those losses can be simply shrugged off.

Picodes commented 3 months ago

Lavarage Appellate Court Decisions

Summary

Issue #14 describes the fact that to create accounts, users deposit SOL to pay for the storage rent. There is currently no way to claim back this deposit once the account isn't used anymore. The debate mainly revolves around the fact that the amounts at stake are low (<1$). An other similar issue in impact was found during the contest: #8

0xTheC0der’s view

I am leaning towards Medium severity for the following reasons:

Concerning the fix: See recommended (via Anchor constraint) and secure (via pure Solana code) way of closing an account: https://github.com/coral-xyz/sealevel-attacks/tree/master/programs/9-closing-accounts

Alternatively, also secure (via Anchor method): https://docs.rs/anchor-lang/latest/anchor_lang/accounts/account/struct.Account.html#method.close

Picodes' view

To me, these are QA because it's exactly like a gas-saving issue in an evm blockchain. If this is Med I don't see why any gas savings on Ethereum that saves >1$ per transaction isn't Med as well. No functionality is broken, and there is no loss of funds in terms of assets managed by the protocol, it's just the setup costs and the transaction price that are clearly suboptimals. I understand that there is a difference in intent between the 2 networks but for instance, we also have gas refunds when you clear storage slots on EVM chains even if capped to a part of the transaction fees.

Hickuphh3’s view

Valid points on each side:

QA:

Med:

Ultimately I think the reasons justifying QA outweigh that for M

Verdict

By a 2/3 consensus, the conclusion from the appellate court is that the issue should remain of QA severity.