Lodestar Audit Competition Final Notes

Our second audit competition with Lodestar Finance has ended and we can’t feel more proud of the white hat hacker community and their exceptional work. We extend our appreciation to all the security researchers who participated and demonstrated their commitment to the safety and security of the ecosystem. Like always, hats off to these talented individuals for their invaluable contributions.

About the competition

The competition officially started on March 8th to March 22. We received a total of 48 submissions, of which 20 were valid. Out of all accepted submissions, 18 were considered medium severity and 2 were gas savings.

Below is a summary of eligible vulnerability disclosures:

0xC…Cc2

0x6…f97

0x9…1AE

0x3…fd3

0x6…f97

0x6…f97

0x6…f97

0x6…f97

0x6…f97

0x1…a27

0xB…4Ad

0xB…4Ad

0xB…4Ad

0xB…4Ad

0x9…1AE

0x8…246

0x6…f97

0x1…905

Accepted submissions

Initialize function no initializer modifier: Initialize function needs to be protected by the modifier initializer to make sure the contract can only be initialized once.

A malicious user can take advantage of the lack of an initializer modifier and reinitialize the contract.

First Deposit Bug: The code implementation mentioned here has a flaw that can be exploited by attackers to steal funds from initial depositors of a newly deployed CToken contract if the contract has not been seeded with an initial deposit. This happens because the exchange rate of the CToken depends on the totalSupply of CTokens and the underlying token balance of the CToken contract, which an attacker can manipulate using specific transactions.

No Transfer Ownership Pattern: A better practice is to have 2 step ownership transfer.

Race condition in the Comp.approve approve function may lead to token theft: This is a known issue in the ERC-20 standard and not directly relevant to Dapp. Awarding medium.

Precision lost due to division before multiplication in getGLPPrice(): Multiplication before division results in a loss of precision, ultimately representing changes much smaller than 1 cent.

call() should be used instead of transfer(): The use of the deprecated transfer() function for an address will make a transaction fail in some cases.

Chainlink’s latestRoundData might return stale results: Need to add a check to make sure that the price read by the contract is the most recent price.

Use safeTransfer instead of transfer: The use of the deprecated transfer() function for an address will make the transaction fail in some cases.

borrowBehalf() is used to borrow funds on behalf of another user:

Intended behavior, however, we agree with the submitter that the user should have to give approval before the contract is allowed to use borrowbehalf for them.

Wrong value for maximum swing percentile: The intended behavior was to allow swings of up to 1%, the implementation was only allowing swings of .1% so, therefore, this is a valid report.

Wrong event emitter on PlvGLPOracle contract: The function _updateMaxSwing emits a wrong event: newWindowSize

getSequencerStatus don’t check the grace period: “startedAt: This timestamp indicates when the sequencer changed status. This timestamp returns 0 if a round is invalid. When the sequencer comes back up after an outage, wait for the GRACE_PERIOD_TIME to pass before accepting answers from the price data feed. Subtract startedAt from block.timestamp and revert the request if the result is less than the GRACE_PERIOD_TIME.

_adminTransferAll and _adminTransfer don’t have allowance to transfer tokens: Allowance is not needed in our implementation but we failed to disclose the tokens planned before the competition so awarding a bounty.

Hardcoded addresses: Forgetting one or more of these three addresses could break the deployment/contracts, getting funds stuck in these contracts and other possible errors

Direct usage of ecrecover allows signature malleability: These functions use the ecrecover function directly to verify the given signature. However, the ecrecover EVM opcode allows for malleable (non-unique) signatures and thus is susceptible to replay attacks.

Oracle returns the wrong price with the tokens which have less than 18 decimals when using the ETH-based aggregator: We don’t use ETH-based aggregator tokens but this was not disclosed before the competition so paying a bounty.

Unhandled return values of transfer and transfer from: ERC20 implementations are not always consistent. Some implementations of transfer and transferFrom could return ‘false’ on failure instead of reverting. It is safer to wrap such calls into require() statements to these failures.

Leverage should be > 100000: The documentation says with desired leverage amount y where 10000 < y <= 3000 so _leverage should be > 10000 but the implementation contract lets you set _leverage = 10000

Ploopy accidentally inherits Ownable instead of Owner which removes an important safeguard without sponsor knowledge: Change so the safeguard is in place on ownership transfer

Can’t swap to new token because of safeTransferFrom #2: Not possible in our implementation but failed to disclose tokens ahead of time so awarding bounty.

Disclaimer: The language was slightly cleaned by the Hats team. Any changes in meaning are not intended.

Gas Savings Winners

First Place 0x6…f97

Second Place0 x3…40A

The gas savings category was based on submissions with the most gas savings. The winning submissions provided many upgrades across the entire codebase. The top two submissions received ⅔ and ⅓ of the gas savings prize respectively.

Low and Disqualified submissions

Manipulation of lToken when totalSupply is zero can lead to an implicit minimum deposit amount and loss of user funds due to rounding errors: Repeat submission of the first deposit bug.

Division before Multiplication can lead to 0 return price and a not-accurate price: A repeat of precision lost due to division before multiplication in getGLPPrice()

No slippage protection will lead to loss of funds intended or unintended: There is no slippage on minting glp

Reentrancy can be possible to avoid liquidation:The function alleged to be compromised has re-entrancy guard.

Incorrect price updating mechanism of plvGLPOracle: The function is working as intended, the security researcher who submitted the error misunderstood the intent of the function

Sushi Oracle is vulnerable to use as a price feed: This was disclosed in the known issues before the competition. This function is not used for pricing anything in the app. It is exclusively used to display the price of $LODE in the front end for user convenience.

Unindexed events:Not a bug, indexed materials are unneeded.

No input validation can result in loss of ownership and re-deployment: Not a bug, this issue describes the consequences of deploying the contract incorrectly. A correctly deployed contract has no issue.

No input validation can lead to a loss of all the funds in the contract: Similar to the previous issue, not a bug, described the consequences of deploying the contract incorrectly. A correctly deployed contract has no issue.

Non-Input validation can lead to no-owner contracts and having to re-deploy: Similar to the previous issue, not a bug, described the consequences of deploying the contract incorrectly. A correctly deployed contract has no issue.

Unchecked return value transferFrom: This is a repeat submission of Use safeTransfer instead of transfer

castVoteBySig() can cause signature replay attack: Same as Direct usage of ecrecover allows signature malleability:

CastVote can be done even proposal is in a pending state: As shown in the state function in the Governor contract, the only possible states for a proposal to be in are: canceled, pending, active, defeated, successful, executed, expired, and queued. The only one of these states where voting should be allowed to take place is during the active state, and the check linked here explicitly requires the proposed state to be active to accomplish this. The change suggested here would allow for voting in all states except for pending, which is not the intended behavior. As for your second point, I think you’ve gotten some things turned around here. A user cannot make a new proposal if their latest proposal is either active or pending. The two are independent of one another.

Admin can transfer all tokens out using function _adminTransferAll and _adminTransfer: Intended behavior, the solution offered of using a multisig is exactly what we do.

No Transfer Ownership Pattern: This contract was out of scope.

Allow borrowCap to be filled fully: This is intended behavior.

Wrong function selector makes native token revert: The Ether market mint function does not take an input parameter because it is dealing in native Ether. As such, msg.value is passed with the transaction as the mint amount. Regarding interacting with the Ether market externally, it is true that using a standard cToken interface mint function will not work, you would need to have a separate interface specifically for the Ether market that handles this correctly.

An unbounded loop on array can lead to DoS: While it is true that there is no upper bound on the size of the array, in practice the array will never grow large enough to exceed the block gas limit. As such, this is seen as a non-issue and not planned at this time. Additionally, it is by design that markets can not be removed from the markets array after they’ve been listed. This is to prevent anyone from having the ability to de-list markets.

Not checking current totalSupplyUnderlying in _setMarketSupplyCaps: This is intended behavior. There may be a reason such as decreased on-chain liquidity available for liquidations that may require parameters to be tightened regardless of the current amount borrowed, overtime new restrictions will bring the market into compliance with lower caps.

Tokens should be minted until it is equals to supplyCap: Intended behavior

DECIMAL_DIFFERENCE is not the standard: This submission misunderstands what this function is doing, this is normalizing the decimals in price of glp.

One can manipulate the price fetched through the SushiOracle and steal funds: This was disclosed in the known issues before the competition. This function is not used for pricing anything in the app. It is exclusively used to display the price of $LODE in the front end for user convenience.

An incorrect return value of function computeAverageIndex(): This is only true during the initialization period of the contract, once initialized this is no longer true.

Incorrect comparison when calculating the averageIndex: This is only true during the initialization period of the contract, once initialized this is no longer true.

The underlying tokens with more than 18 decimals are unable to get price: We don’t use tokens with more than 18 decimals, tokens used are disclosed in docs ahead of the competition.

Supply caps of CToken can be set to lower than the current supply by mistake: This is intended behavior. There may be a reason such as decreased on-chain liquidity available for liquidations that may require parameters to be tightened regardless of the current amount borrowed, overtime new restrictions will bring the market into compliance with lower caps.

Borrow caps of CToken can be set to lower than the current total borrows by mistake: This is intended behavior. There may be a reason such as decreased on-chain liquidity available for liquidations that may require parameters to be tightened regardless of the current amount borrowed, overtime new restrictions will bring the market into compliance with lower caps.

Disclaimer: The language was slightly cleaned by the Hats team. Any changes in meaning are not intended.

Final Remarks from the Lodestar Finance Team

Big thank you to the Hats team for organizing this competition. We had some great submissions and identified several medium-level bugs that were missed in our first audit. Having many skilled researchers look over the code has proven to be a successful endeavor. The recommendation from the Lodestar team is to simplify the submission process and organize incoming submissions with a repository of submissions instead of needing to catalog them into a spreadsheet.

Final Remarks from the Hats Finance Team:

As we continue to receive an increasing demand for our Hats audit competition product, we are committed to ongoing development by ensuring the implementation of new features, and addressing existing issues.

Issue 1:

• Submitting GitHub issues on other platforms — We are developing a feature that allows submitters to submit issues from the Hats dApp, along with the Hats on-chain report. We estimate that this feature will be implemented in our next competition.

Issue 2:

• Classification process — The Hats security team will review the winner list and classification process to ensure that winner selection is made according to the rules and guidelines. This has already been implemented.

Issue 3:

Downgrade severity rewards:

• We recognize the potential future challenges with severity classification and are working on short-term, and long-term solutions.

Short-term: Implemented

• Reward cap for high severity — This ensures attractive bounties while maintaining a cap that the protocol is willing to pay.
• Dispute resolution: Alongside the winner announcement post, submitters can send disputes to the committee team and request clarification. They can also involve the Hats security team in the process. The aim is to facilitate honest and professional debate regarding disputed submissions.

Long-term:

• A hard-coded arbitrator contract will be deployed, granting the arbitrator decision-making power on disputes.
• This will involve a collaboration with Kleros Courts, but final details will be ready next week.
• Security researchers interested in reviewing and collaborating with Hats and Kleros can join this group to receive more information about the only decentralized dispute mechanism in Web3.

In closing, we welcome criticism and feedback to help us improve. Our focus will always be to deliver a comprehensive product that considers the entire security needs of a DeFi project, with independent security researchers as our partners.

Our goal is to provide every protocol, at any stage, with the tools it needs to be safer and therefore create a safer web3 ecosystem — combining audit firms that have skin in the game, audit competitions, on-chain bug bounties, detection integration, and more features to come.

Once again, thank you to Lodestar for trusting us with their security needs, and security researchers for embarking on this journey with us. Stay on the lookout for a new audit competition coming to Hats Finance soon!

Until then — stay safe!