Unchecked Arithmetic in NominationAgent Contract

Github username: @neuraldevx Twitter username: -- Submission hash (on-chain): 0x1a4ebcf2024f7424bda536fa0fc82169a0ad0793f8bcc96e50e1d79bf5116a0a Severity: high

Description: Description

The NominationAgent contract contains several instances of unchecked arithmetic operations. Specifically, the deposit and start_unbond functions perform addition and subtraction on the staked variable without checking for overflow or underflow. This can lead to unexpected behavior and potential loss of funds.

Attack Scenario

An attacker could exploit this vulnerability by causing an overflow or underflow in the staked variable. For example, if the staked value is close to the maximum value of u128, a large deposit could cause an overflow, resulting in an incorrect staked value. Similarly, if the staked value is small, a large unbonding request could cause an underflow.

Attachments

Proof of Concept (PoC) File

#[cfg(test)]
mod tests {
use super::*;

#[ink::test]
fn test_overflow() {
    let mut agent = NominationAgent::new(AccountId::default(), AccountId::default(), 1);
    agent.staked = u128::MAX;
    assert!(agent.deposit().is_err());
}

#[ink::test]
fn test_underflow() {
    let mut agent = NominationAgent::new(AccountId::default(), AccountId::default(), 1);
    agent.staked = 0;
    assert!(agent.start_unbond(1).is_err());
}
}

Revised Code File (Optional)

impl NominationAgent {
#[ink(message, payable, selector = 1)]
pub fn deposit(&mut self) -> Result<(), RuntimeError> {
    let deposit_amount = Self::env().transferred_value();

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

    if self.staked == 0 {
        // Join nomination pool
        self.env()
            .call_runtime(&RuntimeCall::NominationPools(
                NominationCall::Join {
                    amount: deposit_amount,
                    pool_id: self.pool_id,
                }
            ))?;
    } else {
        // Bond extra AZERO to nomination pool
        self.env()
            .call_runtime(&RuntimeCall::NominationPools(
                NominationCall::BondExtra {
                    extra: BondExtra::FreeBalance {
                        balance: deposit_amount,
                    }
                }
            ))?;
    }

    self.staked = self.staked.checked_add(deposit_amount).ok_or(RuntimeError::CallRuntimeFailed)?;

    Ok(())
}

#[ink(message, selector = 2)]
pub fn start_unbond(&mut self, amount: u128) -> Result<(), RuntimeError> {
    // Restricted to vault
    if Self::env().caller() != self.vault {
        return Err(RuntimeError::Unauthorized);
    }

    self.staked = self.staked.checked_sub(amount).ok_or(RuntimeError::CallRuntimeFailed)?;

    // Trigger un-bonding process
    self.env()
        .call_runtime(&RuntimeCall::NominationPools(
            NominationCall::Unbond {
                member_account: MultiAddress::Id(Self::env().account_id()),
                unbonding_points: amount,
            }
        ))?;

    Ok(())
}
}

Recommendation:

Use checked arithmetic methods like checked_add and checked_sub to prevent overflow and underflow. This ensures that any arithmetic operation that exceeds the limits of u128 will result in an error, preventing unexpected behavior and potential loss of funds.

hats-finance / Kintsu-0x7d70f9442af3a9a0a734fa6a1b4857f25518e9d2

Unchecked Arithmetic in NominationAgent Contract #38