unused-ret.md

Unused return values of non-void function

Configuration

• detector id: unused-ret
• severity: low

Description

The return value of functions should be used. Otherwise, some information may be lost or unchecked.

Sample code

// https://github.com/ref-finance/ref-contracts/blob/117b044280053661fda217057560c8e35111856f/ref-exchange/src/lib.rs#L98
#[near_bindgen]
#[derive(BorshSerialize, BorshDeserialize, PanicOnDefault)]
pub struct Contract {
    // ...
    guardians: UnorderedSet<AccountId>,
    // ...
}

// https://github.com/ref-finance/ref-contracts/blob/536a60c842e018a535b478c874c747bde82390dd/ref-exchange/src/owner.rs#L65
#[near_bindgen]
impl Contract {
    #[payable]
    pub fn remove_guardians(&mut self, guardians: Vec<ValidAccountId>) {
        assert_one_yocto();
        self.assert_owner();
        for guardian in guardians {
            // pub fn remove(&mut self, element: &T) -> bool
            // Removes a value from the set. Returns whether the value was present in the set.
            self.guardians.remove(guardian.as_ref());
        }
    }
}

In this sample, the contract doesn't check the return value of remove to make sure the guardian exists in self.guardians. If the guardian doesn't exist, the program will not panic, which may bring an unexpected impact.

A possible fixed version is as follows:

#[payable]
pub fn remove_guardians(&mut self, guardians: Vec<ValidAccountId>) -> Vec<ValidAccountId> {
    assert_one_yocto();
    self.assert_owner();
    let mut guardians_left = vec![];
    for guardian in guardians {
        if !self.guardians.remove(guardian.as_ref()) {
            guardians_left.push(guardian);
        }
    }
    guardians_left
}

In this version, remove_guardians will return a vector of all the guardians which are not in the self.guardians.