Platform: Code4rena
Start Date: 08/04/2021
Pot Size: $100,000 USDC
Total HM: 3
Participants: 10
Period: 14 days
Judge: Nick Johnson
Total Solo HM: 3
Id: 4
League: ETH
Rank: 1/10
Findings: 2
Award: $33,406.11
🌟 Selected for report: 7
🚀 Solo Findings: 1
🌟 Selected for report: cmichel
11135.3712 USDC - $11,135.37
@cmichelio
When the protocol suffers a default, the BPT stakers are the first line of defence and the protocol trades the BPT pool tokens for the single-sided liquidity asset of the Balancer LIQUIDITY <> MPT pool. (PoolLib.handleDefault)
Note that a pool token to single-asset trade is the same as burning the LP tokens to receive an equal amount of all underlying tokens, and then trading all other tokens received for the single asset.
It's the reverse of this: "Depositing a single asset A to a shared pool is equivalent to depositing all pool assets proportionally and then selling more of asset A to get back all the other tokens deposited." Balancer
This means on each default MPT tokens are sold for the liquidity asset. As the default is potentially a huge amount that happens at once, this creates a huge arbitrage opportunity.
As the default suffered can be a huge amount and the "repayment" happens at once, this creates a huge arbitrage opportunity. The MPT token price goes down. The borrow could also be incentivised to not repay the loan and take advantage of the arbitrage opportunity, either competing themselves on-chain or through shorts/bets on the MPT price.
Hard to completely mitigate. Pool delegates should be especially careful when giving out high-value loans and demand high collateral lockup.
#0 - lucas-manuel
2021-04-23T19:53:54Z
This is a valid concern, but not something that we are going to mitigate before launch. We are going to plan for PDs to atomically liquidate and burn.
#1 - Arachnid
2021-04-27T02:08:35Z
I think this a valid finding; whether or not it's intended to be mitigated pre-launch, the Sponsor acknowledges it's a valid concern, and not something that's declared as part of the protocol's intrinsic assumptions. These sort of findings are exactly what audits are intended to uncover and bring to the attention of users as caveats when using the system. I concur with the Warden's assessment of Medium.
🌟 Selected for report: cmichel
3711.7904 USDC - $3,711.79
@cmichelio
There is no check if the return value indicates stale data. This could lead to stale prices according to the Chainlink documentation:
The price oracle might return unreliable price data which can lead to a variety of different issues in the protocol, for example, for liquidating more staker & lender tokens than required at fair market price.
Add missing checks for stale data. See example here.
#0 - lucas-manuel
2021-04-22T21:32:59Z
We will add this check, but disagree that this is a high severity bug.
#1 - lucas-manuel
2021-04-22T21:33:28Z
Especially since we will be using BTC and ETH oracles to start, it is very rare that there will be stale data.
#2 - Arachnid
2021-04-27T01:30:43Z
Since the contract only asks for latest data, incomplete rounds should be impossible, so we can discount them. Stale data is possible; I would rate this as Likelihood=LOW (it'll be difficult to make ChainLink oracles go stale) and Impact=Medium (this could only be used to create arb opportunities on loan collateral or liquidations, which will be limited to the price change during the stale period), resulting in a Severity=Low.
🌟 Selected for report: cmichel
3711.7904 USDC - $3,711.79
@cmichelio
The response from the price oracle always assumes 8 decimals (see PoolLib.convertFromUsd) but it's never checked if the oracle response has 8 decimals using ChainLink's .decimals() function.
At some point, the governor might set up a USD price feed oracle that contains more than 8 decimals leading to inflated prices everywhere.
Consider checking _aggregator.decimals() == 8 in ChainlinkOracle constructor and changeAggregator.
#0 - lucas-manuel
2021-04-22T21:34:37Z
We were going to do this manually, we were aware of this issue, but it is a good idea to just add a check. Disagree with severity.
#1 - Arachnid
2021-04-27T01:33:41Z
Impact would be High if this happened, but the Likelihood is very low. Agree with Severity=Low.
#2 - lucas-manuel
2021-04-27T19:49:19Z
Ended up not addressing this, will just make sure to check this during asset onboarding.
🌟 Selected for report: cmichel
3711.7904 USDC - $3,711.79
@cmichelio
The ChainlinkOracle.setManualPrice function specifies that it can only be called "if manualOverride == true".
This is not the case.
Assume an oracle failure happened, and the oracle needs to be manually set to prevent losses.
The setManualPrice function succeeds and the owner might think that the oracle price is overwritten as the function would fail when manualOverride is not true according to specification.
The protocol would still use the broken chainlink price feed and suffer losses.
Add the missing require(manualOverride == true, "manual override not set") check.
#0 - lucas-manuel
2021-04-22T21:38:40Z
Not really a bug, but we will address this.
#1 - Arachnid
2021-04-27T01:37:12Z
Agree with Non-critical as reported.
I would add, though, that the current configuration is unsafe. If setManualPrice is changed to require that manualOverride is true, there will be an interval between calling setManualOverride and setManualPrice during which an old price is used. Instead, a single function that enables the manual override and sets the price should probably be used.
#2 - Arachnid
2021-04-27T01:38:22Z
On further thought, upgrading to Low based on Warden's reasoning - the success of the call to setManualPrice may lead the submitter to believe that the issue is resolved, when it only sets a value that is not referenced.
🌟 Selected for report: cmichel
3711.7904 USDC - $3,711.79
@cmichelio
Anyone can send the USDC interest to the balancer pool by calling withdrawFundsOnBehalf(balancerPool).
An attacker can abuse this to capture part of this interest by doing the following steps in a single transaction:
withdrawFundsOnBehalf(bPool), call gulp.USDC interest that was supposed to go to MPT balancer pool stakers is stolen by attackers. Funds might be locked forever.
This is hard to prevent completely because you're sending free money to the pool. One way to reduce the risk is to only allow claiming interest by the governor / trusted parties. This would disallow attacker to perform this in a risk-free way in a single transaction, but the same attack would still be possible for miners.
Consider alternative ways of distributing the interest of balancer pools like transferring it to all MPT holders instead of liquidity providers, because:
withdrawFundsOnBehalf was called will benefit, regardless of how long they have been providing this liquidity.#0 - lucas-manuel
2021-04-23T19:32:43Z
Not a bug, distributeToHolders and withdrawFundsOnBehalfOf will always be called atomically by the governor.
#1 - Arachnid
2021-04-27T01:56:10Z
Without code to ensure that withdrawFundsOnBehalfOf is called immediately after distributeToHolders`, this bug can still occur. I'm considering this Likelihood=Low,Impact=Medium => Severity=Low.
🌟 Selected for report: cmichel
3711.7904 USDC - $3,711.79
@cmichelio
The MapleGlobals.setPriceOracle should check that the oracle address is not zero.
A wrong call to this function might set the oracle address to the zero address and break core oracle functionality.
Add a require(oracle != 0) statement.
#0 - lucas-manuel
2021-04-22T21:42:56Z
Will address
#1 - lucas-manuel
2021-04-26T16:36:24Z
We actually are not going to address this, we do not think this is a bug. The governor will manually verify non-zero addresses.
#2 - Arachnid
2021-04-27T01:44:08Z
I think this warrants Likelihood=Low,Impact=Medium => Severity=Low. Unlike deployment misconfigurations, this mistake, while unlikely, would impact the running system, and it's easily defended against.
🌟 Selected for report: cmichel
3711.7904 USDC - $3,711.79
@cmichelio
Anyone can withdraw USDC interest of another address by calling withdrawFundsOnBehalf(addr).
Imagine a smart contract that has a specific function for withdrawing the USDC contract to their contract:
function withdrawAndTransfer() { mpt.withdraw(); usdc.transfer(usdc.balanceOf(address(this), owner); }
If the contract has no skim function to transfer out the assets, they can get stuck forever.
USDC interest can get stuck forever.
Disallow withdrawals on behalf of other users.
#0 - lucas-manuel
2021-04-23T19:38:30Z
Not a bug, this was intended. Projects taht integrate with MPL will have to take this functionality into account.
#1 - Arachnid
2021-04-27T02:00:10Z
Many systems integrate support for generic ERC-20 token contracts without being able to handle per-token special cases. For example, I believe this issue would affect any Uniswap/Balancer/etc liquidity pool between MPL and any non-USDC token. Rating this as Low, per submitter, though I believe an argument could be made for making this higher severity, since it limits the ability to use MPL tokens in generic systems without losing out on dividends.