shaflow01 - Peers that have been set cannot be modified or deleted.

[sherlock-admin3]

shaflow01

Medium

Peers that have been set cannot be modified or deleted.

Summary

A target chain corresponds to a Peer account, which stores the target chain address for cross-chain message sending. However, the program does not provide any method to modify the target chain message sending address, nor can it delete any unsupported target chains.

Root Cause

The administrator can only initialize a Peer account for a target chain once, and the program lacks the functionality to modify or delete an already enabled Peer.

https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract/blob/02396b61f6e77008d8d24c8b84f65644b20f445e/solana-vault/packages/solana/contracts/programs/solana-vault/src/instructions/oapp_instr/set_peer.rs#L12

#[derive(Accounts)]
#[instruction(params: SetPeerParams)]
pub struct SetPeer<'info> {
    #[account(mut)]
    pub admin: Signer<'info>,
    #[account(
@>        init,
        payer = admin,
        space = 8 + Peer::INIT_SPACE,
        seeds = [PEER_SEED, &oapp_config.key().to_bytes(), &params.dst_eid.to_be_bytes()],
        bump
    )]
    pub peer: Account<'info, Peer>,
    #[account(
        seeds = [OAPP_SEED],
        bump = oapp_config.bump,
        has_one = admin @OAppError::Unauthorized
    )]
    pub oapp_config: Account<'info, OAppConfig>,
    pub system_program: Program<'info, System>,
}

Internal pre-conditions

None

External pre-conditions

The target chain contract address needs to be migrated for various reasons.

Attack Path

None

Impact

The administrator cannot adjust the target chain address of the Peer account, and when the target chain is no longer in use, the Peer address cannot be deleted to prevent users from sending cross-chain messages.

PoC

No response

Mitigation

It is recommended to allow the administrator to reset the target chain address, while also adding an "allowed" parameter to the Peer account to enable and disable cross-chain messages for the target chain

#[account]
#[derive(InitSpace)]
pub struct Peer {
    pub address: [u8; 32],
    pub rate_limiter: Option<RateLimiter>,
    pub bump: u8,
+   pub allowed: bool,
}

#[derive(Accounts)]
#[instruction(params: SetPeerParams)]
pub struct SetPeer<'info> {
    #[account(mut)]
    pub admin: Signer<'info>,
    #[account(
-       init,
+       init_if_needed,
        payer = admin,
        space = 8 + Peer::INIT_SPACE,
        seeds = [PEER_SEED, &oapp_config.key().to_bytes(), &params.dst_eid.to_be_bytes()],
        bump
    )]
    pub peer: Account<'info, Peer>,
    #[account(
        seeds = [OAPP_SEED],
        bump = oapp_config.bump,
        has_one = admin @OAppError::Unauthorized
    )]
    pub oapp_config: Account<'info, OAppConfig>,
    pub system_program: Program<'info, System>,
}

impl SetPeer<'_> {
    pub fn apply(ctx: &mut Context<SetPeer>, params: &SetPeerParams) -> Result<()> {
        ctx.accounts.peer.address = params.peer;
        ctx.accounts.peer.bump = ctx.bumps.peer;
+       ctx.accounts.peer.allowed = params.allowed;
        Ok(())
    }
}
#[derive(Clone, AnchorSerialize, AnchorDeserialize)]
pub struct SetPeerParams {
    pub dst_eid: u32,
    pub peer: [u8; 32],
+   pub peer: bool,
}

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/OrderlyNetwork/solana-vault/pull/4/commits/b5399989090de61f5d1b77fa1576751abca6b825

[gjaldon]

This line makes the peer modifiable and fixes this issue.