Phoenix-Protocol-Group / phoenix-contracts

Source code of the smart contracts of the Phoenix DeFi hub DEX protocol
GNU General Public License v3.0
10 stars 6 forks source link

PHOAM-023: Confusing variable naming #356

Closed gangov closed 2 months ago

gangov commented 2 months ago

Location

./contracts/stake/src/distribution.rs:53

Description The shares_per_point variable in each distribution actually tracks the amount of points a single share is worth, contrary to what the name suggests. Coinspect consultants reviewed all references to this variable to detect any confusion with its units and found that the calculations were correct, thereby rating this issue as informational. However, the current naming makes maintenance more prone to errors in handling shares/points units.

The snippet below is from the distribute_rewards function. This code computes the amount of points each share is worth (points_per_share variable) and adds this result to the distribution.shares_per_point variable:

let ppw = distribution.shares_per_point;
let stakes: i128 = get_stakes(env, owner).total_stake;
// Decimal::one() represents the standard multiplier per token
// 1_000 represents the constant token per power. TODO: make it
configurable
let points = calc_power(config, stakes as i128, Decimal::one(),
TOKEN_PER_POWER);
let points = (ppw * points as u128) as i128;

A similar scenario occurs in the withdrawable_rewards function, where the variable names are also inverted. Note that calc_power returns the power/shares based on the user's total stake.

Recommendation Rename the variable to points_per_share and review the naming of variables from the functions mentioned above.