QA Report

[code423n4]

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 - code-423n4/2021-10-defiprotocol-findings#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

[lucas-manuel]

Title: Missing commenting Severity: Low Risk

    The following functions are missing commenting as describe below:
    
    xMPL.sol, scheduleMigration (external), parameters migrator_, newAsset_ not commented

All comments are contained in inherited interface with Natspec

[lucas-manuel]

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);

• Example 1 is impossible as we would still have to divide either way
• I don't think Example 2 is an example of this
• I don't think Example 3 is an example of this

[lucas-manuel]

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_

• maxWithdraw is a view function so no validation needed
• migrate does a transferFrom the owner, so it would revert unless there was an approval performed from the zero address which is impossible
• setPendingOwner does not need a zero check since acceptOwnership must be called by the pendingOwner. When acceptOwnership is called by a valid owner pendingOwner is set to address(0) anyways.
• maxRedeen is a view function so no validation needed

[lucas-manuel]

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

setPendingOwner and acceptOwnership is the two step process used to transfer admin ownership

[lucas-manuel]

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");

We are not concerned with the approval front running attack during migration.

[lucas-manuel]

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

Hmm I'm not sure I understand what this means actually.

[lucas-manuel]

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_)

We can look into this further but I don't think this is needed. The getters are pure functions, the refinance functions intentionally do not validate since they have to be agreed on by two parties so they should have flexibiltiy, and the establishment fee values are not rates so they cannot be validated

[lucas-manuel]

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

• acceptOwnership does not make an external call
• updateVestingSchedule does not make an external call that is not a view function
• setPendingOwner does not make an external call

[lucas-manuel]

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_

We're not going to add any of these

[lucas-manuel]

Title: Div by 0 Severity: Low/Medium Risk

Division by 0 can lead to accidentally revert, (An example of a similar issue - code-423n4/2021-10-defiprotocol-findings#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)

1. extraDelegateFee is added, not divided
2. SCALED_ONE is a constant which is 10 ** 18
3. princaiplRequested_ cannot be zero as it is validated on line 112
4. This could be zero, but we would want the function to revert in this case anyways
5. It wouldn't matter, its the numerator
6. totalPayments_ would never be zero because the loan would be finished
7. extraDelegateFee is added, not divided
8. endingPrincipal is in the numerator

[lucas-manuel]

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

The constructors aren't supposed to match

[lucas-manuel]

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

Will remove these, this is not an issue though

[lucas-manuel]

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

This is intended functionality

[lucas-manuel]

Disagree with all of these findings