# Throughout code-base: Homogenize how access control is done. Low/Info · Three Sigma · Institutional lending · 10th April, 2024 Finding 3S-MAPLE-N11 of the Maple Finance security review. - Protocol: https://maple.finance/ - Report: /reports/maple-finance-iii-2024 - Source: https://cdn.sanity.io/files/qoqld077/production/36dbe5ca76da3d2392bcee581548067705b8bd36.pdf --- ### Description There are some discrepancies in the way that checks for access control/requires are done throughout the code base. Sometimes a modifier is used, sometimes an internal function is used, and sometimes the require is just hard-coded. Some examples below: ● on open-term/LoanManager.sol ○ isLoan and notPaused are implemented as modifiers ○ require is used to check if factory or if PD (Pool Delegate) or PM (Pool Manager) ● on fixed-term/LoanManager.sol ○ internal function (_requireCallerIsPoolDelegate ) is used to check if PD (different from open-term above) ○ internal function (_requireProtocolNotPaused) is used to check if protocol is not paused (different from open-term above) ● on open-term/MapleLoan.sol and on fixed-term/MapleLoan.sol ○ whenNotPaused is used as a modifier ○ require is used to check if factory, if borrower, if lender We understand that requires might be used to return custom string error messages that specify, in addition to the contract, in which function it reverted (which nevertheless is not done in fixed-term LoanManager since internal functions are used here, where only the contract it reverts is specified in the error message); however, there is still a discrepancy on how the access control is done on the open-term and fixed-term for the LoanManager functions (internal functions vs requires). ### Recommendation We suggest using the same approach in both contracts. Also, there are three different implementations to check if a protocol is not paused (modifier notPaused, modifier whenNotPaused, internal function _requireProtocolNotPaused), we also suggest using the same approach across all contracts. Additionally when a modifier is added to a function, the entire code of the modifier gets copied in all the functions. It is more gas efficient (on deployment) to implement the logic of the modifier in another function and call it from inside the modifier. ### Status Addressed in the following PRs: maple-labs/fixed-term-loan-private#283 maple-labs/pool-v2-private#271 maple-labs/globals-v2-private#63 maple-labs/open-term-loan-private#55 maple-labs/open-term-loan-manager-private#53 maple-labs/fixed-term-loan-manager-private#31 --- Related findings: - [deTokenize() is missing access control, anyone can burn other people's nfts](https://0xsimao.com/findings/metazero-i-tokenize-access-control-burn): MetaZero Omnichain NFTs - [SpiritVestingFactory::setTreasury() could be 2 factor, as well as the AccessControl OZ contract used in the codebase](https://0xsimao.com/findings/spirit-protocol-vesting-well-access-control): Spirit Protocol - [Throughout code base, implement using SafeERC20 for IERC20 for better readability](https://0xsimao.com/findings/fuji-finance-erc20-ierc20-better-readability): Fuji Finance - [Admin checks in KeyringCoreV2Base could be placed in a modifier to increase readability and reduce code duplication](https://0xsimao.com/findings/keyring-ii-placed-readability-reduce-duplication): Keyring Credentials