Out-of-flow balance transfer to NominationAgent may cause inconsistencies in the protocol

Github username: -- Twitter username: -- Submission hash (on-chain): 0x7359e604db8f48d3c6c31d628ec08ebf65f9ef0d8d98d1a1127f026a1f3df9fe Severity: medium

Description: Description\ The NominationAgent contract holds information about the amount of staked tokens in its storage (self.staked). The NominationAgent contract calls runtime to stake the native tokens sent by the user via the Vault contract.

The compound function is used to claim the staking rewards from the native runtime staking and then submit them for staking. The code snippet portraying the rewards fetching is as follows:

(...)

if Self::env().caller() != vault {
    return Err(RuntimeError::Unauthorized);
}

// Claim available AZERO
self.env()
    .call_runtime(&RuntimeCall::NominationPools(
        NominationCall::ClaimPayout {}
    ))?;

let balance = Self::env().balance();

// Gracefully return when nomination pool had nothing to claim
if balance == 0 {
    return Ok((0, 0));
}`

(...)

The aforementioned code does not consider that the NominationAgent's balance can be non-zero before claiming the payout from native staking. Although only the new constructor and deposit functions are payable, it must be noted that the ink!-based contracts, once deployed, are identified by the same type of AccountId as user-controlled accounts used in the Substrate-based chains. Hence, it is possible to use the native pallet_balances module to transfer native tokens manually to the contract.

Attack Scenario\ A malicious user might manually send some additional tokens to the NominationAgent contract to cause subsequent inconsistencies in the protocol. Although those funds sent by the attacker will be staked, the NominationAgent will consider that it was the protocol, i.e., its other components, used to stake assets. As the NominationAgent exposes the self.staked value via the get_staked_value function, it will impact any subsequent calculations.

Attachments

Proof of Concept (PoC) File

#[test]
fn send_assets_to_nomination_agent() {
    let mut ctx = setup().unwrap();
    let nominator_new = ctx
        .sess
        .deploy(
            helpers::bytes_nominator(),
            "new",
            &[ctx.vault.to_string(), false.to_string()],
            vec![3],
            None,
            &helpers::transcoder_nominator().unwrap(),
        )
        .unwrap();

    let balance_to_transfer = 1000;
    let balance_transfer_call = RuntimeCall::Balances(pallet_balances::Call::transfer {
        dest: nominator_new.clone(),
        value: balance_to_transfer,
    });

    let balance_before = ctx.sess.chain_api().balance(&nominator_new);

    ctx.sess
        .chain_api()
        .runtime_call(balance_transfer_call, RuntimeOrigin::signed(ctx.alice)).unwrap();

    let balance_after = ctx.sess.chain_api().balance(&nominator_new);

    assert_eq!(balance_after, balance_before + balance_to_transfer);
    // NOTE: The nominator used in tests is actually the Mock Nominator
    // which is out-of-scope, but the same principle applies to any contract
}

Revised Code File (Optional)
```
(...)
```

if Self::env().caller() != vault { return Err(RuntimeError::Unauthorized); }

// getting the balance before the claim let balance_before = Self::env().balance();

// Claim available AZERO self.env() .call_runtime(&RuntimeCall::NominationPools( NominationCall::ClaimPayout {} ))?;

// the balance variable only contains the tokens claimed from the NominationPool let balance = Self::env().balance() - balance_before;

// Gracefully return when nomination pool had nothing to claim if balance == 0 { return Ok((0, 0)); }`

(...)

hats-finance / Kintsu-0x7d70f9442af3a9a0a734fa6a1b4857f25518e9d2

Out-of-flow balance transfer to NominationAgent may cause inconsistencies in the protocol #41