QA Report - Githubissues

code423n4 commented 2 years ago

Title: Missing commenting Severity: Low Risk

    The following functions are missing commenting as describe below:

    xMPL.sol, scheduleMigration (external), parameters migrator_, newAsset_ not commented

Title: Mult instead div in compares Severity: Low Risk

To improve algorithm precision instead using division in comparison use multiplication in the following scenario:

        Instead a < b / c use a * c < b. 

    MapleLoanInternals.sol, 460, return principal_ <= drawableFunds_ ? uint256(0) : (collateralRequired_ * (principal_ - drawableFunds_)) / principalRequested_;
    MapleLoanInternals.sol, 486, if (raisedRate <= SCALED_ONE) return ((principal_ - endingPrincipal_) / totalPayments_, uint256(0)); 
    RevenueDistributionToken.sol, 285, return (numerator_ / divisor_) + (numerator_ % divisor_ > 0 ? 1 : 0);

Title: Not verified owner Severity: Low Risk

    owner param should be validated to make sure the owner address is not address(0).
    Otherwise if not given the right input all only owner accessible functions will be unaccessible.

    RevenueDistributionToken.sol.maxWithdraw owner_
    Migrator.sol.migrate owner
    RevenueDistributionToken.sol.setPendingOwner pendingOwner_
    RevenueDistributionToken.sol.maxRedeem owner_

Title: Two Steps Verification before Transferring Ownership Severity: Low Risk

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked. It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership. A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

    RevenueDistributionToken.sol

Title: safeApprove of openZeppelin is deprecated Severity: Low Risk

    You use safeApprove of openZeppelin although it's deprecated.
    (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38)
    You should change it to increase/decrease Allowance as OpenZeppilin says
    This appears in the following locations in the code base

Deprecated safeApprove in xMPL.sol line 62: require(ERC20(oldAsset).approve(migrator, oldAssetBalanceBeforeMigration), "xMPL:PM:APPROVAL_FAILED");

Title: Named return issue Severity: Low Risk

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.

    MapleLoan.sol, collateralRequired
    MapleLoan.sol, earlyFeeRate
    MapleLoan.sol, treasuryFee

Title: Does not validate the input fee parameter Severity: Low Risk

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

    MapleLoanInternals._getLateInterest (lateFeeRate_)
    MapleLoanInternals._getPaymentBreakdown (lateFeeRate_)
    MapleLoanInternals._getRefinanceInterestParams (lateFeeRate_)
    Refinancer.setLateFeeRate (lateFeeRate_)
    MapleLoanInternals._setEstablishmentFees (extraDelegateFee_)
    MapleLoanInternals._processEstablishmentFees (delegateFee_)
    Refinancer.setEarlyFeeRate (earlyFeeRate_)
    MapleLoanInternals._setEstablishmentFees (extraTreasuryFee_)
    MapleLoanInternals._processEstablishmentFees (treasuryFee_)

Title: Missing non reentrancy modifier Severity: Low Risk

The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer. Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..

    RevenueDistributionToken.sol, acceptOwnership is missing a reentrancy modifier
    RevenueDistributionToken.sol, updateVestingSchedule is missing a reentrancy modifier
    RevenueDistributionToken.sol, setPendingOwner is missing a reentrancy modifier

Title: Not verified input Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    MapleLoanInternals.sol._initialize borrower_
    MapleLoanInternals.sol._getRefinanceCommitment refinancer_
    MapleLoanInternals.sol._removeCollateral destination_
    RevenueDistributionToken.sol.balanceOfAssets account_

Title: Div by 0 Severity: Low/Medium Risk

Division by 0 can lead to accidentally revert, (An example of a similar issue - https://github.com/code-423n4/2021-10-defiprotocol-findings/issues/84)

    MapleLoanInternals.sol (L351) extraDelegateFee_ might be 0)
    MapleLoanInternals.sol (L637) one_ might be 0)
    MapleLoanInternals.sol (L460) principalRequested_ might be 0)
    RevenueDistributionToken.sol (L285) numerator_ might be 0)
    MapleLoanInternals.sol (L631) one_ might be 0)
    RevenueDistributionToken.sol (L87) vestingPeriod_ might be 0)
    MapleLoanInternals.sol (L488) periodicRate might be 0)
    MapleLoanInternals.sol (L486) totalPayments_ might be 0)
    MapleLoanInternals.sol (L354) extraTreasuryFee_ might be 0)
    MapleLoanInternals.sol (L488) endingPrincipal_ might be 0)

Title: Override function but with different argument location Severity: Low/Med Risk

    xMPL.sol.constructor inherent ERC20.sol.constructor but the parameters does not match
    RevenueDistributionToken.sol.constructor inherent ERC20.sol.constructor but the parameters does not match

Title: Open TODOs Severity: Low Risk

Open TODOs can hint at programming or architectural errors that still need to be fixed. These files has open TODOs:

Open TODO in RevenueDistributionToken.sol line 275 : // TODO: investigate whether leave this require() in for clarity from error message, or let the safe math check in callerAllowance - shares_ handle the underflow.

Open TODO in RevenueDistributionToken.sol line 77 : // TODO: Revisit returns

Title: Anyone can withdraw others Severity: Low Risk

Anyone can withdraw users shares. Although we think that they are sent to the right address, it is still 1) not the desired behavior 2) can be dangerous if the receiver is a smart contract 3) the receiver may not know someone withdraw him

    RevenueDistributionToken.maxWithdraw
    RevenueDistributionToken.withdraw
    RevenueDistributionToken.previewWithdraw