coral-xyz / anchor

⚓ Solana Sealevel Framework
https://anchor-lang.com
Apache License 2.0
3.68k stars 1.35k forks source link

Negative number from client flipped to positive if expecting unsigned int #1592

Open jibbers42 opened 2 years ago

jibbers42 commented 2 years ago

If I have a u64 on the rust side and pass a negative (new anchor.BN(-1)) from the client then the program will receive that number as a positive. I would expect an error to be returned or at least for the number to be changed to 0 (which isn't good either really).

anchor version:

$ anchor --version
anchor-cli 0.22.1

program:

use anchor_lang::prelude::*;

declare_id!("Ces8PtnCfymcGeFKNLWBhRUYd9LHf4N7ZEDQd8P9pwHc");

#[program]
pub mod anchor_negative_number {
    use super::*;

    pub fn initialize(ctx: Context<Initialize>, amount: u64) -> Result<()> {
        // It seems like anchor would return an error before reaching this point
        // or, at the very least, amount should be 0.
        msg!(&amount.to_string());
        if amount != 0 {
            return Err(ProgramError::InvalidArgument.into());
        }
        Ok(())
    }
}

#[derive(Accounts)]
pub struct Initialize {}

test:

import * as anchor from "@project-serum/anchor";
import { Program } from "@project-serum/anchor";
import { AnchorNegativeNumber } from "../target/types/anchor_negative_number";

describe("anchor-negative-number", () => {
  // Configure the client to use the local cluster.
  anchor.setProvider(anchor.Provider.env());

  const program = anchor.workspace.AnchorNegativeNumber as Program<AnchorNegativeNumber>;

  it("Is initialized!", async () => {
    // Add your test here.
    const tx = await program.rpc.initialize(new anchor.BN(-1), {});
    console.log("Your transaction signature", tx);
  });
});

output:

  anchor-negative-number
Transaction simulation failed: Error processing Instruction 0: invalid program argument 
    Program Ces8PtnCfymcGeFKNLWBhRUYd9LHf4N7ZEDQd8P9pwHc invoke [1]
    Program log: Instruction: Initialize
    Program log: 1
    Program log: ProgramError occurred. Error Code: InvalidArgument. Error Number: 8589934592. Error Message: The arguments provided to a program instruction where invalid.
    Program Ces8PtnCfymcGeFKNLWBhRUYd9LHf4N7ZEDQd8P9pwHc consumed 3674 of 200000 compute units
    Program Ces8PtnCfymcGeFKNLWBhRUYd9LHf4N7ZEDQd8P9pwHc failed: invalid program argument
    1) Is initialized!

  0 passing (136ms)
  1 failing

  1) anchor-negative-number
       Is initialized!:
     Error: failed to send transaction: Transaction simulation failed: Error processing Instruction 0: invalid program argument
      at Connection.sendEncodedTransaction (node_modules/@solana/web3.js/src/connection.ts:3964:13)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
      at Connection.sendRawTransaction (node_modules/@solana/web3.js/src/connection.ts:3921:20)
      at sendAndConfirmRawTransaction (node_modules/@solana/web3.js/src/util/send-and-confirm-raw-transaction.ts:27:21)
      at Provider.send (node_modules/@project-serum/anchor/src/provider.ts:118:18)
      at Object.rpc [as initialize] (node_modules/@project-serum/anchor/src/program/namespace/rpc.ts:25:23)
paul-schaaf commented 2 years ago

the solution is to make the client check the type in the IDL and then return an error if the type is a uSomething but the BN is < 0

jibbers42 commented 2 years ago

Interesting... I guess I was thinking of this like normal client/server stuff, but maybe the design of Anchor implies the client would always be the same, and the design of Solana means there is a cost to do an operation. Still, the cost is cheap and using Anchor on the client isn't required (I assume), so a bad actor might still cause problems... or less paranoid, just a bug.

Before reading your solution, I changed the type to a signed int and return an error if it's < 0. Unless I'm missing something, I feel like it would be better for Anchor to catch this case for me... if that's possible.

paul-schaaf commented 2 years ago

The anchor client should catch this case for you yes. To be clear, I was suggesting a way for us to build this into Anchor. Not a solution that you should implement in your own code.

PhongVu07 commented 1 year ago

Any update on this?