QA Report

[code423n4]

Low-impact Issues

Treasury fees are given to the lender on failure, rather than reverting

        if (!_sendFee(_mapleGlobals(), IMapleGlobalsLike.mapleTreasury.selector, treasuryFee_)) {
            _claimableFunds += treasuryFee_;
        }

https://github.com/maple-labs/loan/blob/4c6fe2cd91d6d16b8434c426fe7eb6d2bc77bc30/contracts/MapleLoanInternals.sol#L321-L323

Inconsistent approve() behavior between ERC20 and RevenueDistributionToken

RevenueDistributionToken considers an approval value of type(uint256).max as 'allow all amounts':

        if (callerAllowance == type(uint256).max) return;

https://github.com/maple-labs/revenue-distribution-token/blob/41a3e40bf8c109ff19b38b80fde300c44fd42a3d/contracts/RevenueDistributionToken.sol#L279

whereas ERC20 considers it as a numerical amount:

        _approve(owner_, msg.sender, allowance[owner_][msg.sender] - amount_);

https://github.com/maple-labs/erc20/blob/10ccf4aa0b2d6914e3c2d32e454e4d106a99a4fd/contracts/ERC20.sol#L110

These inconsistences will likely lead to confusion at some point in the future.

Incorrect revert string in setEndingPrincipal()

        require(endingPrincipal_ <= _principal, "R:DP:ABOVE_CURRENT_PRINCIPAL");

https://github.com/maple-labs/loan/blob/4c6fe2cd91d6d16b8434c426fe7eb6d2bc77bc30/contracts/Refinancer.sol#L43

It should be "R:SEP:ABOVE_CURRENT_PRINCIPAL".

IERC20 should be named IERC20Permit

1. File: erc20-1.0.0-beta.2/contracts/interfaces/IERC20.sol (lines 4-5)
   There may be cases in the future where you may not want EIP-2612 functionality due to deployment costs, and having the name IERC20 taken will cause problems

/// @title Interface of the ERC20 standard as defined in the EIP, including ERC-2612 permit functionality.
interface IERC20 {

IERC20 incorrectly includes PERMIT_TYPEHASH

PERMIT_TYPEHASH is not part of the requirements for EIP-2612, so it shouldn't appear in the interface.

    /**
     *  @dev    Returns the permit type hash.
     *  @return permitTypehash_ The permit type hash.
     */
    function PERMIT_TYPEHASH() external view returns (bytes32 permitTypehash_);

https://github.com/maple-labs/erc20/blob/10ccf4aa0b2d6914e3c2d32e454e4d106a99a4fd/contracts/interfaces/IERC20.sol#L134-L138
OpenZeppelin has it as a private constant: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/52eeebecda140ebaf4ec8752ed119d8288287fac/contracts/token/ERC20/extensions/draft-ERC20Permit.sol#L28

Missing checks for address(0x0) when assigning values to address state variables

1. File: revenue-distribution-token-1.0.0-beta.1/contracts/RevenueDistributionToken.sol (line 73)

        pendingOwner = pendingOwner_;

Open TODOs

There are many open TODOs throughout the various test files, but also some among the code files

./revenue-distribution-token-1.0.0-beta.1/contracts/RevenueDistributionToken.sol:    // TODO: Revisit returns
./revenue-distribution-token-1.0.0-beta.1/contracts/RevenueDistributionToken.sol:        // TODO: investigate whether leave this `require()` in for clarity from error message, or let the safe math check in `callerAllowance - shares_` handle the underflow.

Incorrect Natspec

     *  @dev   Emits an event indicating that one account has set the allowance of another account over their tokens.

https://github.com/maple-labs/erc20/blob/10ccf4aa0b2d6914e3c2d32e454e4d106a99a4fd/contracts/interfaces/IERC20.sol#L12
The natspec doesn't mention that the event is also emitted when transferFrom() is called, even though the natspec for transferFrom() explicitly mentions it.

Non-critical Issues

_processEstablishmentFees() should emit events when fee processing fails

    function _processEstablishmentFees(uint256 delegateFee_, uint256 treasuryFee_) internal {
        if (!_sendFee(_lender, ILenderLike.poolDelegate.selector, delegateFee_)) {
            _claimableFunds += delegateFee_;
        }

        if (!_sendFee(_mapleGlobals(), IMapleGlobalsLike.mapleTreasury.selector, treasuryFee_)) {
            _claimableFunds += treasuryFee_;
        }
    }

https://github.com/maple-labs/loan/blob/4c6fe2cd91d6d16b8434c426fe7eb6d2bc77bc30/contracts/MapleLoanInternals.sol#L316-L324

Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate

1. File: erc20-1.0.0-beta.2/contracts/ERC20.sol (lines 32-34)

    mapping(address => uint256) public override balanceOf;

    mapping(address => mapping(address => uint256)) public override allowance;

Use scientific notation (e.g. 10e18) rather than exponentiation (e.g. 10**18)

1. File: loan-3.0.0-beta.1/contracts/MapleLoanInternals.sol (line 14)

    uint256 private constant SCALED_ONE = uint256(10 ** 18);

public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

1. File: loan-3.0.0-beta.1/contracts/MapleLoanFactory.sol (lines 16-18)

    function createInstance(bytes calldata arguments_, bytes32 salt_)
        override(IMapleProxyFactory, MapleProxyFactory) public returns (
            address instance_

Use a more recent version of solidity

Use a solidity version of at least 0.8.12 to get string.concat() to be used instead of abi.encodePacked(<str>,<str>)

1. File: erc20-1.0.0-beta.2/contracts/ERC20.sol (line 2)

pragma solidity ^0.8.7;

Typos

owner => owner_
https://github.com/maple-labs/erc20/blob/10ccf4aa0b2d6914e3c2d32e454e4d106a99a4fd/contracts/interfaces/IERC20.sol#L129
https://github.com/maple-labs/erc20/blob/10ccf4aa0b2d6914e3c2d32e454e4d106a99a4fd/contracts/interfaces/IERC20.sol#L132
https://github.com/maple-labs/mpl-migration/blob/a99549d96ed12cd4589a02bccf70747dbaebeb5b/contracts/Migrator.sol#L24
https://github.com/maple-labs/mpl-migration/blob/a99549d96ed12cd4589a02bccf70747dbaebeb5b/contracts/Migrator.sol#L26
https://github.com/maple-labs/mpl-migration/blob/a99549d96ed12cd4589a02bccf70747dbaebeb5b/contracts/Migrator.sol#L27
https://github.com/maple-labs/loan/blob/4c6fe2cd91d6d16b8434c426fe7eb6d2bc77bc30/contracts/interfaces/IOwnable.sol#L17

account => account_
https://github.com/maple-labs/loan/blob/4c6fe2cd91d6d16b8434c426fe7eb6d2bc77bc30/contracts/interfaces/IOwnable.sol#L11

Emits an event => Emitted when
https://github.com/maple-labs/erc20/blob/10ccf4aa0b2d6914e3c2d32e454e4d106a99a4fd/contracts/interfaces/IERC20.sol#L12
https://github.com/maple-labs/erc20/blob/10ccf4aa0b2d6914e3c2d32e454e4d106a99a4fd/contracts/interfaces/IERC20.sol#L20

ERC-2612 => EIP-2612
https://github.com/maple-labs/erc20/blob/10ccf4aa0b2d6914e3c2d32e454e4d106a99a4fd/contracts/interfaces/IERC20.sol#L4

Grammar

Throughout the various interfaces, most of the comments have fragments that end with periods. They should either be converted to actual sentences with both a noun phrase and a verb phrase, or the periods should be removed.

[lucas-manuel]

1. Intentional
2. We can address, infomational
3. We can address, informational
4. We are always going to want permit, dismissed
5. We would like to keep this public.
6. pendingOwner does not need a zero check as it is a two step process
7. TODOs is duplicate
8. Incorrect, Natspec only mentions events emitted in functions
9. This will be monitored in tenderly, no event needed
10. Won't implement this
11. Won't implmement this
12. Has to match visibility of overriden function
13. Won't implement
14. Will implement typo changes

All issues are informational.