Security Audit
December 15, 2023
Version 1.0.0
Presented by 0xMacro
This document includes the results of the security audit for Maple Finance's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from November 27, 2023 to December 11, 2023.
The purpose of this audit is to review the source code of certain Maple Finance Solidity contracts, and provide feedback on the design, architecture, and quality of the source code with an emphasis on validating the correctness and security of the software in its entirety.
Disclaimer: While Macro’s review is comprehensive and has surfaced some changes that should be made to the source code, this audit should not solely be relied upon for security, as no single audit is guaranteed to catch all possible bugs.
The following is an aggregation of issues found by the Macro Audit team:
Severity | Count | Acknowledged | Won't Do | Addressed |
---|---|---|---|---|
Medium | 2 | 1 | - | 1 |
Low | 1 | - | - | 1 |
Code Quality | 12 | 6 | 2 | 4 |
Informational | 3 | - | - | - |
Maple Finance was quick to respond to these issues.
Our understanding of the specification was based on the following sources:
The following source code was reviewed during the audit:
a2cf31c51e3efa44ae17ef5ab4bfc1ea2581c112
Specifically, we audited the following contracts.
Contract | SHA256 |
---|---|
modules/globals/contracts/MapleGlobals.sol |
|
modules/fixed-term-loan/contracts/MapleLoan.sol |
|
modules/fixed-term-loan/contracts/MapleLoanFactory.sol |
|
modules/fixed-term-loan/contracts/MapleLoanV502Migrator.sol |
|
modules/withdrawal-manager-cyclical/contracts/MapleWithdrawalManager.sol |
|
modules/withdrawal-manager-cyclical/contracts/MapleWithdrawalManagerInitializer.sol |
|
modules/pool/contracts/MaplePoolManager.sol |
|
modules/pool/contracts/MaplePoolDeployer.sol |
|
modules/pool/contracts/proxy/MaplePoolManagerInitializer.sol |
|
modules/pool/contracts/proxy/MaplePoolManagerMigrator.sol |
|
modules/pool/contracts/proxy/MaplePoolManagerStorage.sol |
|
modules/pool/contracts/proxy/MaplePoolManagerWMMigrator.sol |
|
modules/pool-permission-manager/contracts/MaplePoolPermissionManager.sol |
|
modules/pool-permission-manager/contracts/proxy/MaplePoolPermissionManagerInitializer.sol |
|
modules/pool-permission-manager/contracts/proxy/MaplePoolPermissionManagerStorage.sol |
|
modules/withdrawal-manager-queue/contracts/proxy/MapleWithdrawalManagerFactory.sol |
|
modules/withdrawal-manager-queue/contracts/proxy/MapleWithdrawalManagerInitializer.sol |
|
modules/withdrawal-manager-queue/contracts/proxy/MapleWithdrawalManagerStorage.sol |
|
modules/withdrawal-manager-queue/contracts/MapleWithdrawalManager.sol |
|
Note: This document contains an audit solely of the Solidity contracts listed above. Specifically, the audit pertains only to the contracts themselves, and does not pertain to any other programs or scripts, including deployment scripts.
Click on an issue to jump to it, or scroll down to see them all.
hasPermissions()
will return true if there are no lenders passed
We quantify issues in three parts:
This third part – the severity level – is a summary of how much consideration the client should give to fixing the issue. We assign severity according to the table of guidelines below:
Severity | Description |
---|---|
(C-x) Critical |
We recommend the client must fix the issue, no matter what, because not fixing would mean significant funds/assets WILL be lost. |
(H-x) High |
We recommend the client must address the issue, no matter what, because not fixing would be very bad, or some funds/assets will be lost, or the code’s behavior is against the provided spec. |
(M-x) Medium |
We recommend the client to seriously consider fixing the issue, as the implications of not fixing the issue are severe enough to impact the project significantly, albiet not in an existential manner. |
(L-x) Low |
The risk is small, unlikely, or may not relevant to the project in a meaningful way. Whether or not the project wants to develop a fix is up to the goals and needs of the project. |
(Q-x) Code Quality |
The issue identified does not pose any obvious risk, but fixing could improve overall code quality, on-chain composability, developer ergonomics, or even certain aspects of protocol design. |
(I-x) Informational |
Warnings and things to keep in mind when operating the protocol. No immediate action required. |
(G-x) Gas Optimizations |
The presented optimization suggestion would save an amount of gas significant enough, in our opinion, to be worth the development cost of implementing it. |
Actors for which manual redeem mode is enabled may go against the purpose of the withdrawal queue mechanism by performing step 1 of the redeem flow (getting their request processed) but delaying step 2, exchanging shares for the available assets indefinitely until an appropriate time.
As a result, they may have an advantage in certain market conditions when an unrealized loss is about to increase, which they may front-run as they can perform exchange instantly while others would need to go through the withdrawal manager queue.
We will consider this for the version 2 of the withdrawal mechanism design.
Currently, we do not believe this to be a concern, as the manual flow can only be enabled for LPs by the Pool Delegate or Protocol Admins. This will be done only for over-collateralized pools, such as our cash management product where the incentive for parking funds is lower.
This flow is also designed for smart contract integrators, so we will ensure that we review the smart contracts ahead of time to see if this would be an issue. As an added precaution, we will ask our Pool Delegates and Protocol Admins to use private RPCs like Flashbots to send any transactions that would reduce the share token exchange rate, to further discourage this issue.
If an LP is seen abusing this, we can also revoke their permissions to interact with the share token or not process any future redemption requests.
In the withdrawal-manager-cyclical module, MapleWithdrawalManagerInitializer._initialize()
does not perform safe downcasting of provided inputs, such as startTime_
, cycleDuration_
, and windowDuration_
. As a result, the provided system invariant may be broken.
The system is designed to reject any attempts to set a start time that falls in the past.
If provided startTime_
, cycleDuration_
, and windowDuration_
are greater than uint64, their values will pass initial checks such as startTime_ >= block.timestamp
, but these values will get truncated on assignment, and incorrect values will be set in cycleConfigs[0]. Allowing even startTime to be 0.
function _initialize(address pool_, uint256 startTime_, uint256 cycleDuration_, uint256 windowDuration_) internal {
require(pool_ != address(0), "WMI:ZERO_POOL");
require(startTime_ >= block.timestamp, "WMI:INVALID_START");
require(windowDuration_ != 0, "WMI:ZERO_WINDOW");
require(windowDuration_ <= cycleDuration_, "WMI:WINDOW_OOB");
pool = pool_;
poolManager = IPoolLike(pool_).manager();
cycleConfigs[0] = CycleConfig({
initialCycleId: 1,
initialCycleTime: uint64(startTime_),
cycleDuration: uint64(cycleDuration_),
windowDuration: uint64(windowDuration_)
});
emit Initialized(pool_, cycleDuration_, windowDuration_);
emit ConfigurationUpdated({
configId_: 0,
initialCycleId_: 1,
initialCycleTime_: uint64(startTime_),
cycleDuration_: uint64(cycleDuration_),
windowDuration_: uint64(windowDuration_)
});
}
Proof of concept:
function test_createInstance_success() external {
// @audit - startTime set to large value
bytes memory calldata_ = abi.encode(address(pool), uint256(type(uint64).max) + 1, 7 days, 2 days);
MapleWithdrawalManager withdrawalManager_ = MapleWithdrawalManager(factory.createInstance(calldata_, "SALT"));
(
uint64 initialCycleId_,
uint64 initialCycleTime_,
uint64 cycleDuration_,
uint64 windowDuration_
) = withdrawalManager_.cycleConfigs(0);
assertEq(withdrawalManager_.pool(), address(pool));
assertEq(withdrawalManager_.latestConfigId(), 0);
assertEq(initialCycleId_, 1);
// @audit - initialCycleTime should never be smaller than block.timestamp
// but because of truncation of values larger than type(uint64).max it ends up being 0
assertEq(initialCycleTime_, 0);
assertEq(cycleDuration_, 7 days);
assertEq(windowDuration_, 2 days);
}
Remediations to consider:
hasPermissions()
will return true if there are no lenders passed
MaplePoolPermissionManager
's hasPermission
function accepts an arbitrary length array for lenders_
and loops through all addresses passed to check the corresponding pool permission level and bitmap. However, if no lenders are passed to this function, the returned value will be true
, potentially allowing access to gated functions without permission.
It is worth mentioning that this is not possible in the current codebase, as it’s impossible to provide arbitrary values to this function outside of external off-chain calls to the view function.
Consider returning false
if no lenders are provided. Alternatively, document this behavior to avoid pitfalls.
Consider removing onlyGovernor
modifier from the MaplePoolPermissionManager
contract since it is not used.
For the following events, consider using an indexed attribute for one or more parameters:
Additional changes in https://github.com/maple-labs/withdrawal-manager-queue-private/commit/9822b7551a11aab5f97be761df34d08f69973fdd.
In both withdrawal-manager-cyclical and withdrawal-manager-queue modules, the implementation of MapleWithdrawalManagerFactory.createInstance()
function contains redundant and unnecessary assignment.
isInstance[instance_ = super.createInstance(arguments_, salt_)] = true;
Consider removing the mentioned assignment since super.createInstance()
performs the same in the parent function implementation.
isInstance[instance_] = true;
We won’t be redeploying the cyclical withdrawal manager factory. For ease of maintainability we’ll keep the same assignment in the new queue withdrawal manager factory.
Across the two withdrawal manager contract types (queue and cyclical) there are logic inconsistencies in implementing pausable functionality.
The whenProtocolNotPaused
modifier checks for isFunctionPaused
in the queue withdrawal manager.
modifier whenProtocolNotPaused() {
require(!IGlobalsLike(globals()).isFunctionPaused(msg.sig), "WM:PAUSED");
_;
}
However, the whenProtocolNotPaused
modifier in the cyclical manager checks protocolPaused
which is less flexible and does not allow fine-grained control.
modifier whenProtocolNotPaused() {
require(!IGlobalsLike(globals()).protocolPaused(), "WM:PROTOCOL_PAUSED");
_;
}
Consider updating implementation to provide consistent behavior between Withdrawal Managers of different types to reduce maintenance impact and prevent operational issues.
Currently the protocol pause is only used for one function setExitCycleConfig() in the cyclical withdrawal manager. We will consider update in the future release.
In the MapleWithdrawalManager
, _processManualExit()
has the following implementation.
function _processManualExit( uint256 shares_, address owner_)
internal returns ( uint256 redeemableShares_, uint256 resultingAssets_)
{
require(shares_ > 0, "WM:PE:NO_SHARES");
require(shares_ <= manualSharesAvailable[owner_], "WM:PE:TOO_MANY_SHARES");
( redeemableShares_ , resultingAssets_ ) = _calculateRedemption(shares_);
// @audit - case when shares_ < redeemableShares_ is not reachable
require(shares_ <= redeemableShares_, "WM:PE:NOT_ENOUGH_LIQUIDITY");
...
}
The value of redeemableShares_
returned from the _calculateRedemption()
is always represented by the following relationship redeemableShares_ <= shares_
. Therefore, shares_
cannot ever be smaller than redeemableShares_
.
The same logic applies to the similar guard condition in the processRedemptions()
function.
require(maxSharesToProcess_ <= redeemableShares_, "WM:PR:LOW_LIQUIDITY");
Consider changing conditions in the implementation above to the following to indicate that only full redemption would be possible.
In _processManualExit()
require(shares_ == redeemableShares_, "WM:PE:NOT_ENOUGH_LIQUIDITY");
and in processRedemptions()
require(maxSharesToProcess_ == redeemableShares_, "WM:PR:LOW_LIQUIDITY");
Additional update in https://github.com/maple-labs/withdrawal-manager-queue-private/pull/27/commits/f4c8e3b6e2fcbcb2d51e628de9606d354abcfd40.
Maple Finance now features two types of withdrawal managers: Cyclical and Queue withdrawal managers. Both of these withdrawal managers feature upgrade functionality, which is access-controlled. However, access control is inconsistent.
Upgrading the cyclical withdrawal manager is allowed for the pool delegate or governor, while in the case of the queue withdrawal manager, upgrades are allowed to be performed by the pool delegate or security admin.
require(msg.sender == poolDelegate_ || msg.sender == governor(), "WM:U:NOT_AUTHORIZED");
require(msg.sender == poolDelegate_ || msg.sender == securityAdmin(), "WM:U:NOT_AUTHORIZED");
Consider updating access control in the Cyclical withdrawal manager to allow securityAdmin()
instead of the governor()
.
Fixed to allow security admin to upgrade.
For the withdrawal-manager-cyclical
module, MapleWithdrawalManager
proxy functions do not feature pausable behavior since whenProtocolNotPaused
modifier is missing. This contrasts the implementation of other similar contracts across modules in the Maple Finance codebase.
Due to the above, fine-grained control over paused functionality on a specific contract instance is unavailable. However, a fallback is a pauseable functionality on the factory level. Functions missing whenProtocolNotPaused
modifier:
The same functions have the corresponding modifier in withdrawal-manager-queue:MapleWithdrawalManager
.
We will make this update when introducing function level granularity for pausing in this contract in a future iteration.
MapleWithdrawalManager.processExit()
emits WithdrawalCanceled
event in addition to WithdrawalProcessed
when all lockedShares are processed, which is slightly misleading and could potentially cause errors in off-chain tracking and monitoring tools.
Consider not emitting this event when all lockedShares
are processed.
This is a requirement from our team to help with the subgraph data modeling.
In the fixed-term-loan
module MapleLoan.skim()
is accessible to anyone, while in the open-term-loan
module MapleLoan.skim()
is accessible only to the borrower and governor.
Consider updating access control for the MapleLoan.skim()
in the fixed-term-loan
module to prevent external parties from profiting from the mistakes of the protocol users.
We don’t see this as an issue as there are inherit differences between open term and fixed term loans. For one, the open term loans don’t directly hold funds to be drawn down like the fixed term loan. We don’t expect the same behaviour across both loan types but this is something we will keep in mind in future iterations.
In the fixed-term-loan
module MapleLoan.skim()
has no zero address check for the destination address, while in the open-term-loan
module MapleLoan.skim()
corresponding check is present.
Consider adding a zero address check to the MapleLoan.skim()
in the fixed-term-loan
module to prevent assets from being inadvertently locked.
As we are not updating the fixed term loan in this release we will defer this to a future update.
In the open-term-loan module, MapleRefinancer
contains unused import of IERC20.
Consider removing this unnecessary import.
We will update the MapleRefinancer in a future release when we decide to re-deploy the MapleRefinancer contract.
In the withdrawal-manager-cyclical module, the MapleWithdrawalManager
contract implements pool manager access control directly in functions addShares()
, removeShares()
, and processExit()
.
require(msg.sender == poolManager, "WM:AS:NOT_POOL_MANAGER");
However, in the Queue withdrawal manager this access control is implemented using onlyPoolManager()
modifier.
Consider replacing repetitive access controls with the onlyPoolManager()
modifier in the cyclical withdrawal manager.
We will update in a future release when we update the cyclical withdrawal manager.
In the pool-permission-manager module, the MaplePoolPermissionManager
contract implements several modes for access control. Two modes that allow function level control and higher level but less fine-grained pool level control are represented by FUNCTION_LEVEL
and POOL_LEVEL
constant values.
These modes require corresponding bitmaps to be set in poolBitmaps
mapping storage variable for each pool. When values in specific bitmap match bitmaps of lenders from respective lenderBitmaps
mapping entries, access is allowed; otherwise is denied.
However, the default when corresponding bitmaps in these two modes are not set is to allow anyone access. This default comes with a high risk of misconfiguration, and actors responsible for configuring the permission manager (pool delegate and protocol admins) must take necessary precautions.
While this design choice is intentional, and it is meant to allow transfers to be allowed by default, Maple Finance will need to ensure that pool delegates are aware of misconfiguration risks.
This is already in the natspec to ensure no operator error. However, we will highlight this in the docs too.
In the cyclical withdrawal manager, the delay period when the new configuration starts to apply is +3 from the current cycle. This allows all users who have already locked their shares not to be affected during withdrawal (in case of enough liquidity during withdrawal).
When the pool delegate changes the configuration, it will take effect only on the start of the third cycle.
This way all users that have already locked their shares will not have their withdrawal time affected.
C1 C2 C3 C4
|===--.--|===-----|===-----|==========----------|
^ ^
configuration change new configuration kicks in
Users that request a withdrawal during C1 will withdraw during WW3 using the old configuration.
Users that lock their shares during and after C2 will withdraw in windows that use the new configuration.
However, if a delay is meant to be used as a Timelock mechanism to allow those unhappy with the new configuration to exit, the defined delay may be too short. Depending at which moment the new config is set in the current cycle, those users who haven’t locked their shares yet may have time from 0 < time-to-decide < cycleDuration
to decide on their withdrawal. This, in extreme cases, is a too short time period for the user to react.
Consider increasing the delay to +4, so that users who haven’t yet locked their shares have at least one full cycle to decide on their withdrawal.
We don’t see this as an issue as the user will still be able to request to withdraw in C2 and be able to exit in C4 as the window duration will still be at the start of C4. But we will keep this in mind for future designs.
As suggested by Chainlink documentation, Optimistic L2 oracle implementations should check their corresponding L2 Sequencer uptime feed to ensure that the sequencer is live before trusting the data feed returned by the oracle. Even with the Oracle staleness price check, if the sequencer is down, there could be multiple price changes provided by the Oracle that won’t go through and will get queued and applied once the sequencer is up; if price changes are high, it could cause unfair conditions for users and borrowers.
Maple Base Oracles are currently not being used by the protocol; if enabled checks should be added to verify that the sequencer is live and provide a grace period to react to potential high price changes (See suggested example in Chainlink docs).
This is something we will keep in mind if we decide to use oracles on the BASE/L2 deployments
Macro makes no warranties, either express, implied, statutory, or otherwise, with respect to the services or deliverables provided in this report, and Macro specifically disclaims all implied warranties of merchantability, fitness for a particular purpose, noninfringement and those arising from a course of dealing, usage or trade with respect thereto, and all such warranties are hereby excluded to the fullest extent permitted by law.
Macro will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, or costs of procurement of substitute goods or services or for any claim or demand by any other party. In no event will Macro be liable for consequential, incidental, special, indirect, or exemplary damages arising out of this agreement or any work statement, however caused and (to the fullest extent permitted by law) under any theory of liability (including negligence), even if Macro has been advised of the possibility of such damages.
The scope of this report and review is limited to a review of only the code presented by the Maple Finance team and only the source code Macro notes as being within the scope of Macro’s review within this report. This report does not include an audit of the deployment scripts used to deploy the Solidity contracts in the repository corresponding to this audit. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. In this report you may through hypertext or other computer links, gain access to websites operated by persons other than Macro. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such websites’ owners. You agree that Macro is not responsible for the content or operation of such websites, and that Macro shall have no liability to your or any other person or entity for the use of third party websites. Macro assumes no responsibility for the use of third party software and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.