RustFund
First Flight #36
Beginner FriendlyRust
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Invalid

Violation of Checks-Effects-Interactions Pattern (Design Flaw + Increased Complexity)

sloth

[M-1] Violation of Checks-Effects-Interactions Pattern (Design Flaw + Increased Complexity)

Description:

The original refund function performs lamport transfers before updating contribution.amount, violating the CEI pattern, which increases complexity and exacerbates risks like state inconsistency.

Impact:
The reversed order makes the code harder to audit and reason about, potentially leading to logical errors or vulnerabilities if the program is extended. For example, a mid-transaction check could observe an outdated contribution.amount.

Proof of Concept:

  1. Lamport transfers occur before state update.

  2. If an external call or logging instruction were added mid-execution, it could observe contribution.amount as non-zero after funds are transferred but before the reset, leading to inconsistent behavior.

Proof of Code:

const anchor = require("@project-serum/anchor");
const { PublicKey, SystemProgram, Transaction, TransactionInstruction } = anchor.web3;
const assert = require("assert");
​
describe("CEI Violation", () => {
const provider = anchor.AnchorProvider.env();
anchor.setProvider(provider);
const program = anchor.workspace.Rustfund;
​
it("Shows interactions before effects can lead to observable inconsistency", async () => {
const fundKp = anchor.web3.Keypair.generate();
const contributorKp = anchor.web3.Keypair.generate();
const [contributionPda] = await PublicKey.findProgramAddress(
[fundKp.publicKey.toBuffer(), contributorKp.publicKey.toBuffer()],
program.programId
);
​
// Setup: Create fund and contribute
await program.rpc.fundCreate("test", "desc", new anchor.BN(2000), {
accounts: {
fund: fundKp.publicKey,
creator: provider.wallet.publicKey,
systemProgram: SystemProgram.programId,
},
signers: [fundKp],
});
await provider.connection.requestAirdrop(fundKp.publicKey, 2000);
await program.rpc.contribute(new anchor.BN(1000), {
accounts: {
fund: fundKp.publicKey,
contributor: contributorKp.publicKey,
contribution: contributionPda,
systemProgram: SystemProgram.programId,
},
signers: [contributorKp],
});
​
// Simulate original refund with a mid-transaction state check
const tx = new Transaction();
tx.add(
new TransactionInstruction({
keys: [
{ pubkey: fundKp.publicKey, isSigner: false, isWritable: true },
{ pubkey: contributorKp.publicKey, isSigner: true, isWritable: true },
{ pubkey: contributionPda, isSigner: false, isWritable: true },
{ pubkey: SystemProgram.programId, isSigner: false, isWritable: false },
],
programId: program.programId,
data: Buffer.from([0x03]), // Refund instruction with fake ID
}),
// Hypothetical logging instruction to simulate mid-execution observation
SystemProgram.transfer({
fromPubkey: contributorKp.publicKey,
toPubkey: contributorKp.publicKey, // Dummy transfer
lamports: 0,
})
);
​
await provider.sendAndConfirm(tx, [contributorKp]);
​
// Post-refund: Contribution should be 0, but order risks mid-state observation
const contribution = await program.account.contribution.fetch(contributionPda);
const fundBalance = await provider.connection.getBalance(fundKp.publicKey);
const contributorBalance = await provider.connection.getBalance(contributorKp.publicKey);
​
assert.equal(contribution.amount.toNumber(), 0, "State updated last");
assert.equal(fundBalance, 1000, "Funds transferred");
assert.equal(contributorBalance, 1000, "Contributor received funds");
​
// Note: In Solana, mid-tx state can’t be directly observed, but if it could,
// contribution.amount would be 1000 after transfers but before reset.
});
});

Recommended Mitigation:
Restructure to follow CEI:

pub fn refund(ctx: Context<FundRefund>) -> Result<()> {
let fund = &mut ctx.accounts.fund;
let contribution = &mut ctx.accounts.contribution;
let contributor = &mut ctx.accounts.contributor;
​
// Checks
if fund.deadline != 0 && (fund.deadline as i64) > Clock::get()?.unix_timestamp {
return Err(ErrorCode::DeadlineNotReached.into());
}
let amount = contribution.amount;
​
// Effects
contribution.amount = 0;
​
// Interactions
**fund.to_account_info().try_borrow_mut_lamports()? =
fund.to_account_info().lamports()
.checked_sub(amount)
.ok_or(ProgramError::InsufficientFunds)?;
​
**contributor.to_account_info().try_borrow_mut_lamports()? =
contributor.to_account_info().lamports()
.checked_add(amount)
.ok_or(ErrorCode::CalculationOverflow)?;
​
Ok(())
}
Updates

Appeal created

bube Lead Judge 2 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

[Invalid] Reentrancy in refund

The reentrancy risk on Solana is highly eliminated. The `try_borrow_mut_lamports` ensures that only one reference to an account exists at a time. Also, once the fund’s lamports are borrowed mutably, no other transaction can modify them until the borrow is released. This means the function will reset the `amount` before the next call.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.