Accounts occupy an excessive amount of space for allocation

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-04-lavarage/blob/9e8295b542fb71b2ba9b4693e25619585266d19e/libs/smart-contracts/programs/lavarage/src/state/position.rs#L3-L31
https://github.com/code-423n4/2024-04-lavarage/blob/9e8295b542fb71b2ba9b4693e25619585266d19e/libs/smart-contracts/programs/lavarage/src/context/borrow.rs#L7-L27
https://github.com/code-423n4/2024-04-lavarage/blob/9e8295b542fb71b2ba9b4693e25619585266d19e/libs/smart-contracts/programs/lavarage/src/context/create_trading_pool.rs#L10

Vulnerability details

Impact

There is a leak of funds because accounts are oversized (as there is a cost to initialize storage).

This occurs every time a new trading pool, or a new position, is created. The latter is more impactful as it happens more frequently. Even if the leak is small on each operation, it will add up to large amounts.

Proof of Concept

This is the Position struct:

#[account]
pub struct Position {
    // The interest rate for borrowing from the pool, often expressed as a percentage.
    pub pool: Pubkey, //@audit-info 32

    // 9997 repaid
    // 9998 sold
    // 9999 liquidated
    // >10000 timestamp of recall
    pub close_status_recall_timestamp: u64, //@audit-info 8

    pub amount: u64, //@audit-info 8

    pub user_paid: u64, //@audit-info 8

    pub collateral_amount: u64, //@audit-info 8

    pub timestamp: i64, //@audit-info 8
    
    pub trader: Pubkey, //@audit-info 32

    pub seed: Pubkey, //@audit-info 32

    pub close_timestamp: i64, //@audit-info 8

    pub closing_position_size: u64, //@audit-info 8

    pub interest_rate: u8, //@audit-info 1
}

https://github.com/code-423n4/2024-04-lavarage/blob/9e8295b542fb71b2ba9b4693e25619585266d19e/libs/smart-contracts/programs/lavarage/src/state/position.rs#L3-L31

Considering the Anchor discriminator (8), the maximum theoretical space allocated would be 8 + 153, but instead it's 8 + 170:

	#[derive(Accounts)]
	pub struct Borrow1<'info> {
@>	  #[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>,
	  #[account(mut)]
	  pub trader: Signer<'info>, // The account of the trader initiating the swap
	  #[account(mut)]
	  pub trading_pool: Account<'info, Pool>, // The trading pool defines the terms
	  #[account(mut)]
	  pub node_wallet: Account<'info, NodeWallet>, // The node wallet provides the funds
	  /// CHECK: check instructions account
	  #[account(address = sysvar::instructions::ID @FlashFillError::AddressMismatch)]
	  pub instructions: UncheckedAccount<'info>,
	  pub system_program: Program<'info, System>,
	  pub clock: Sysvar<'info, Clock>,
	  /// CHECK: We just want the value of this account
	  pub random_account_as_id: UncheckedAccount<'info>,
	  /// CHECK: We just want the value of this account
	  #[account(mut)]
	  pub fee_receipient: UncheckedAccount<'info>,
	}

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

A similar argument can be made for CreateTradingPool, which uses 8 + 107 when allocating space for trading_pool instead of using 8 + 72:

	#[derive(Accounts)]
	pub struct CreateTradingPool<'info> {
	  #[account(init, 
	    payer = operator,
@>	    space = 8 + 107, // Specify the required space
	    seeds = [b"trading_pool", operator.key().as_ref(), mint.key().as_ref()],
	    bump
	    )] // Define your seeds)]
	  pub trading_pool: Account<'info, Pool>,
	  #[account(mut)]
	  pub operator: Signer<'info>, // The account of the operator
	  pub node_wallet: Account<'info, NodeWallet>, // The account of the trader initiating the swap
	  pub mint: Account<'info, Mint>,
	  pub system_program: Program<'info, System>,
	}

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

The documentation about how space works and the appropriate values can be found here.

Tools Used

Manual review

Recommended Mitigation Steps

Consider modifying the previous structs so they occupy the correct amount of space:

	#[derive(Accounts)]
	pub struct Borrow1<'info> {
-	  #[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)]
+	  #[account(init, payer = trader, space = 8+153, 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>,
	  #[account(mut)]
	  pub trader: Signer<'info>, // The account of the trader initiating the swap
	  #[account(mut)]
	  pub trading_pool: Account<'info, Pool>, // The trading pool defines the terms
	  #[account(mut)]
	  pub node_wallet: Account<'info, NodeWallet>, // The node wallet provides the funds
	  /// CHECK: check instructions account
	  #[account(address = sysvar::instructions::ID @FlashFillError::AddressMismatch)]
	  pub instructions: UncheckedAccount<'info>,
	  pub system_program: Program<'info, System>,
	  pub clock: Sysvar<'info, Clock>,
	  /// CHECK: We just want the value of this account
	  pub random_account_as_id: UncheckedAccount<'info>,
	  /// CHECK: We just want the value of this account
	  #[account(mut)]
	  pub fee_receipient: UncheckedAccount<'info>,
	}

	#[derive(Accounts)]
	pub struct CreateTradingPool<'info> {
	  #[account(init, 
	    payer = operator,
-	    space = 8 + 107, // Specify the required space
+	    space = 8 + 72, // Specify the required space
	    seeds = [b"trading_pool", operator.key().as_ref(), mint.key().as_ref()],
	    bump
	    )] // Define your seeds)]
	  pub trading_pool: Account<'info, Pool>,
	  #[account(mut)]
	  pub operator: Signer<'info>, // The account of the operator
	  pub node_wallet: Account<'info, NodeWallet>, // The account of the trader initiating the swap
	  pub mint: Account<'info, Mint>,
	  pub system_program: Program<'info, System>,
	}

Assessed type

Math

[c4-sponsor]

piske-alex (sponsor) confirmed

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as selected for report

[koolexcrypto]

I don't see this as an issue because it is common to add extra space for any extra fields to be added in future. Because once you allocate space for it, you can not add to it. So, the space should be considered from the beginning.

Here is an example from NodeWallet :

use anchor_lang::prelude::*;
#[account]
pub struct NodeWallet {

    // Total funds currently available for lending in the pool.
    pub total_funds: u64,

    // Total funds currently lent out from the pool.
    pub total_borrowed: u64,

    // The maintenance LTV, at which borrowers need to add more collateral or pay back part of the loan.
    pub maintenance_ltv: u8,

    // The liquidation LTV, at which the collateral can be liquidated to cover the loan.
    pub liquidation_ltv: u8,

    // The address of the node operator managing this pool.
    pub node_operator: Pubkey,

    // Additional fields can be added as needed.
}

[DadeKuma]

You can use realloc to resize accounts in the future if needed. Anyway, users are paying for storage that they are not currently using, so it's still a loss of funds

[alcueca]

This one I'm going to consider dust amounts, and downgrade to QA

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-a

[c4-judge]

alcueca marked the issue as not selected for report

[DadeKuma]

@alcueca

Not dust. There is an excess of 52 bytes in total, so:

dadekuma@DADEKUMA-DESKTOP:/mnt/d/Auditing/2024-04-lavarage$ solana rent 52
Rent-exempt minimum: 0.0012528 SOL

The user has to pay 0.0012528 SOL more than needed, i.e. same cost as #14 which is considered a valid Med, and $0.18 USD is not considered dust:

I don't think that $0.18 USD is dust on solana nowadays

[Arabadzhiew]

Hey @DadeKuma,

Please note that the losses here should be calculated based on the differences between the current space values and the actual needed values.

For positions, the results are as follows: 178 bytes currently are currently used - costing 0.00212976 SOL; 161 bytes are actually needed - costing 0.00201144 SOL. The difference between those two is 0.00011832 SOL, meaning that 0.017 USD is currently lost per each one of those PDAs.

And for trading pools, the results are as follows: 115 bytes currently are currently used - costing 0.00169128 SOL; 80 bytes are actually needed - costing 0.00144768 SOL. The difference between those two is 0.0002436 SOL, meaning that 0.036 USD is currently lost per each one of those PDAs.

This is because the rent prices seem to increase logarithmically rather than linearly.

[DadeKuma]

I've re-checked and your math seems correct. In this case, the total loss would be $0.053 USD, which is 1/3 of what I initially thought.

I'm not sure what the threshold for dust amounts at this point is, I will let the judge decide.

But these amounts are so close, I strongly believe that these two issues should be treated with the same risk rating, especially because we don't have any rules that define how much should be considered dust, so that would be a subjective choice (given the fact that there is basically no difference between 5.3 cents and 18 cents in terms of value).

[Arabadzhiew]

Keep in mind that the number of trading pool and position PDAs will most definitely not be 1:1, but rather 1:100 or something along those lines. This means that simply summing up the losses per single account of each type is not correct, and also that the position accounts should be the most important ones when considering the losses coming from that issue.

[DadeKuma]

Both issues are dust in a single operation/instance, a few cents of difference doesn't matter to any user.

The main impact was the accumulation of these losses as those are the most frequently used operations in the protocol.

Let's just put both as QA at this point to keep everything fair. I'm not going to discuss further.

[Arabadzhiew]

The main impact was the accumulation of these losses as those are the most frequently used operations in the protocol.

Yes, and this is where the big difference between those two issues comes, given that position accounts are the most frequently created ones. I am also ending my input under this issue with this comment, as I believe that enough has been said already.

[alcueca]

This issue makes each position $0.017 more expensive than it should be, #14 makes each position $0.31 more expensive than it should be. Although there is an order of magnitude between both, I'm going to draw a completely arbitrary dust line of $1 per position and make both QA.