No check when updating `rdpxBurnPercentage` or `rdpxFeePercentage`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L175-L199 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L652-L666

Vulnerability details

Impact

When updating the values of the variables rdpxBurnPercentage and rdpxFeePercentage, their respective setter function does not check that the sum of those to variables is equal to 1e10 after the new values are set, this can result in bad RPDX balance accounting if the sum is less than 1e10 and if it is greater it will lead to either burning too much RPDX fund or sending to much RPDX fees than intended.

Proof of Concept

The values of rdpxBurnPercentage and rdpxFeePercentage are used to handle the RPDX token amount trasnferred from the user for bonding, as both variables represent a % of the amount their sum should be equal 100% or in our case 1e10.

/// @notice The % of rdpx to burn while bonding
uint256 public rdpxBurnPercentage = 50 * DEFAULT_PRECISION;

/// @notice The % of rdpx sent to fee distributor while bonding
uint256 public rdpxFeePercentage = 50 * DEFAULT_PRECISION;

The issue is that in both the setter functions for this variables there is no check to ensure that their sum will be equal to 1e10 :

/**
    * @notice Sets the rdpx burn percentage
    * @dev    Can only be called by admin
    * @param  _rdpxBurnPercentage the burn percentage to set in 1e8 precision
    **/
function setRdpxBurnPercentage(
    uint256 _rdpxBurnPercentage
) external onlyRole(DEFAULT_ADMIN_ROLE) {
    _validate(_rdpxBurnPercentage > 0, 3);
    rdpxBurnPercentage = _rdpxBurnPercentage;
    emit LogSetRdpxBurnPercentage(_rdpxBurnPercentage);
}

/**
    * @notice Sets the rdpx fee percentage
    * @dev    Can only be called by admin
    * @param  _rdpxFeePercentage the fee percentage to set in 1e8 precision
    **/
function setRdpxFeePercentage(
    uint256 _rdpxFeePercentage
) external onlyRole(DEFAULT_ADMIN_ROLE) {
    _validate(_rdpxFeePercentage > 0, 3);
    rdpxFeePercentage = _rdpxFeePercentage;
    emit LogSetRdpxFeePercentage(_rdpxFeePercentage);
}

This can cause problems as if one of the variables is changed without changing the other as well or if when changing them their sum is different from 1e10, some RPDX funds transferred by the user may not accounted for or it could lead to either burning too much RPDX fund or sending a greater amount as fees than what was intended.

This happens because of the following lines of code :

// Transfer rDPX and ETH token from user
IERC20WithBurn(reserveAsset[reservesIndex["RDPX"]].tokenAddress)
    .safeTransferFrom(msg.sender, address(this), _rdpxAmount);

// burn the rdpx
IERC20WithBurn(reserveAsset[reservesIndex["RDPX"]].tokenAddress)
    .burn((_rdpxAmount * rdpxBurnPercentage) / 1e10);

// transfer the rdpx to the fee distributor
IERC20WithBurn(reserveAsset[reservesIndex["RDPX"]].tokenAddress)
    .safeTransfer(
        addresses.feeDistributor,
        (_rdpxAmount * rdpxFeePercentage) / 1e10
    );

As you can see when _rdpxAmount is pulled from the user a rdpxBurnPercentage % of it is burned (currently 50%) and rdpxFeePercentage % is send as fees.

But if at some point the sum of rdpxBurnPercentage and rdpxFeePercentage is not equal to 1e10 (100%) then the contract will either not use all the RPDX funds send by the user (if the sum is less than 1e10) which will stay in the contract but will not added to the variables tracking the RPDX token balance, or if the sum is greater than 1e10 the contract will have to pull tokens from its internal RPDX balance to handle additional RPDX amount to be burned or transferred as fees, because in this case the user amount will not be sufficient to cover both the burning operation and fees transfer.

In both cases this issue will lead to having a bad accounting of the RPDX tokens balance inside the contract and even a loss of RPDX tokens.

Tools Used

Manual review

Recommended Mitigation Steps

To avoid this issue, i recommend to create a single setter function for updating both rdpxBurnPercentage and rdpxFeePercentage variables and add a check inside it to verify that their sum is always equal to 1e10.

Assessed type

Context

[c4-pre-sort]

bytes032 marked the issue as duplicate of #1196

[c4-pre-sort]

bytes032 marked the issue as duplicate of #747

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)