Brisk Felt Lark - The reinit_oapp did not update the delegate when setting a new admin.

[sherlock-admin2]

Brisk Felt Lark

Low/Info

The reinit_oapp did not update the delegate when setting a new admin.

Summary

The reinit_oapp did not update the delegate when setting a new admin. This causes the old admin address to still be able to interact directly with the endpoint and influence cross-chain messages, while the new admin does not have this privilege.

Root Cause

When the oapp_config is initialized for the first time, the admin is set as the delegate. However, during reinitialization (reinit), when a new admin is set, the delegate address is not updated. This results in the new admin not being granted the necessary permissions, while the old admin's permissions are not revoked.

https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract/blob/main/solana-vault/packages/solana/contracts/programs/solana-vault/src/instructions/oapp_instr/reinit_oapp.rs#L33
impl ReinitOApp<'_> {
    pub fn apply(
        ctx: &mut Context<ReinitOApp>,
        reset_oapp_params: &ReinitOAppParams,
    ) -> Result<()> {
        let oapp_config = &mut ctx.accounts.oapp_config;
 @>      oapp_config.admin = reset_oapp_params.admin;
        oapp_config.endpoint_program =
            if let Some(endpoint_program) = reset_oapp_params.endpoint_program {
                endpoint_program
            } else {
                ENDPOINT_ID
            };
        oapp_config.usdc_hash = reset_oapp_params.usdc_hash;
        oapp_config.usdc_mint = reset_oapp_params.usdc_mint;
        oapp_config.bump = ctx.bumps.oapp_config;
        Ok(())
    }
}

Additionally, in the transferAdmin function, the project team added a TODO tag when updating the delegate. This indicates that the team anticipated updating the delegate when the admin changes.However, the above situation was not taken into account.

impl TransferAdmin<'_> {
    pub fn apply(ctx: &mut Context<TransferAdmin>, params: &TransferAdminParams) -> Result<()> {
        ctx.accounts.oapp_config.admin = params.admin;
@>        // TODO:call endpoint to update delegate
        Ok(())
    }
}

Internal pre-conditions

None

External pre-conditions

When calling reinit_oapp, the new admin address was switched.

Attack Path

None

Impact

Since the delegate address can configure oappConfig through the endpoint and influence the execution of cross-chain information, the removed admin role still retains these permissions, while the new admin cannot obtain them.

PoC

No response

Mitigation

impl ReinitOApp<'_> {
    pub fn apply(
        ctx: &mut Context<ReinitOApp>,
        reset_oapp_params: &ReinitOAppParams,
    ) -> Result<()> {
        let oapp_config = &mut ctx.accounts.oapp_config;
        oapp_config.admin = reset_oapp_params.admin;
        oapp_config.endpoint_program =
            if let Some(endpoint_program) = reset_oapp_params.endpoint_program {
                endpoint_program
            } else {
                ENDPOINT_ID
            };
        oapp_config.usdc_hash = reset_oapp_params.usdc_hash;
        oapp_config.usdc_mint = reset_oapp_params.usdc_mint;
        oapp_config.bump = ctx.bumps.oapp_config;

+       let seeds: &[&[u8]] =
+            &[OAPP_SEED, &[ctx.accounts.oft_store.bump]];
+       let _ = oapp::endpoint_cpi::set_delegate(
+            ctx.accounts.oapp_config.endpoint_program,
+            ctx.accounts.oapp_config.key(),
+            &ctx.remaining_accounts,
+            seeds,
+            SetDelegateParams { admin },
+       )?;
        Ok(())
    }
}