close() in account.rs: unwrap() on checked_add should return a graceful error

[Mctursh]

Hello,

I was reading through the close function in anchor-lang and spotted an unwrap() on a checked_add that I think should be replaced with a proper error propagation. Here's the line in question:

pub fn close<'info>(info: AccountInfo<'info>, sol_destination: AccountInfo<'info>) -> Result<()> {
    let dest_starting_lamports = sol_destination.lamports();
    **sol_destination.lamports.borrow_mut() =
        dest_starting_lamports.checked_add(info.lamports()).unwrap(); // <-- this one
    **info.lamports.borrow_mut() = 0;

    info.assign(&system_program::ID);
    info.resize(0).map_err(Into::into)
}

Is this exploitable?

Honestly, not really, at least not today. Current total supply is around ~567M SOL (~567 * 10^15 lamports), which is nowhere near u64::MAX (~18.4 * 10^18). And unlike tokens with a hard cap, SOL has no max supply — it's inflationary by design. That said, even with inflation, we're still orders of magnitude away from overflow territory for the foreseeable future.

But I still think it's worth fixing, and here's why:

The real issue: CPI composability

This is the one that matters. close() gets called by basically every Anchor program that closes accounts. If another program calls into a close instruction via CPI and this somehow panics, the entire transaction dies. There's no way for the calling program to catch a panic from invoke / invoke_signed — it just kills everything.

If this returned a proper error instead, the caller would get back a ProgramError they can actually match on and handle gracefully. Right now, any program composed with an Anchor program's close instruction has a (theoretical) unrecoverable failure path baked in.

Given that close() lives in the framework itself and gets inherited across the ecosystem, the surface area here is pretty wide. On top of that, a panic just gives you a cryptic Program failed to complete in explorer logs — a proper error code is something integrators can actually work with. The SPL token program already handles this exact same operation gracefully in their close account path:

// From solana-program/token processor.rs — CloseAccount
let destination_starting_lamports = destination_account_info.lamports();
**destination_account_info.lamports.borrow_mut() = destination_starting_lamports
    .checked_add([source](https://github.com/solana-program/token/blob/main/program/src/processor.rs)_account_info.lamports())
    .ok_or(TokenError::Overflow)?;

(source)

Same operation, same context, but they propagate the error instead of panicking. I think Anchor should hold itself to the same standard.

Suggested fix

One-liner:

**sol_destination.lamports.borrow_mut() = dest_starting_lamports
    .checked_add(info.lamports())
    .ok_or(ProgramError::ArithmeticOverflow)?;

Zero risk of regression, function already returns Result<()>, and now it composes cleanly.

Happy to open a PR for this if you think it's worth merging. Let me know!

[acheroncrypto]

The real issue: CPI composability

This is the one that matters. close() gets called by basically every Anchor program that closes accounts. If another program calls into a close instruction via CPI and this somehow panics, the entire transaction dies. There's no way for the calling program to catch a panic from invoke / invoke_signed — it just kills everything.

If this returned a proper error instead, the caller would get back a ProgramError they can actually match on and handle gracefully. Right now, any program composed with an Anchor program's close instruction has a (theoretical) unrecoverable failure path baked in.

There is nothing "real" about this issue because:

• The mentioned unwrap can never panic in the real world
• CPI failures halt the transaction independent of whether the instruction returns an error or panic.

Suggested fix

One-liner:

**sol_destination.lamports.borrow_mut() = dest_starting_lamports
    .checked_add(info.lamports())
    .ok_or(ProgramError::ArithmeticOverflow)?;

Zero risk of regression, function already returns Result<()>, and now it composes cleanly.

Happy to open a PR for this if you think it's worth merging. Let me know!

A simpler would way would be to use Lamports::add_lamport:

sol_destination.add_lamports(info.lamports())?;

which would not only handle the overflow case, but also the potential panic of borrow_mut (which can actually happen in the real world, unlike the lamports overflow).

In general, it's fine, or even preferred, to use unwrap and not propagate errors when you are sure it can't panic. However, in this case, I wouldn't mind this change because:

• It's more readable and idiomatic
• Negative performance impact is negligable
• AI-based tools are likely to report non-existent issues like this one.

[Mctursh]

Thanks for the detailed breakdown @acheroncrypto . I did consider the current supply being well under u64::MAX, though with no hard cap on issuance, I flagged it anyway. But you're right that it's effectively unreachable in practice.

Happy to submit a PR using the add_lamportsapproach if that works.

[Mctursh]

@acheroncrypto, kindly check the PR I made for this
#4227

Thank you