HackMDQuant Protocol - Halborn Report Response
(HAL-01) Vulnerable TimelockController Contract
As recommended by Halborn, we updated the TimelockController contract to use the revised timelock contract from version 3.4.2-solc-0.7 of the OpenZepellin contracts, which included a fix to the vulnerability reported by Halborn.
The changes were added at the following commit: 1550fd9.
(HAL-02) Misuse of an Oracle
We acknowledge and understand the recommendation given by Halborn to check if the latest price data returned by a Chainlink oracle is recent. Still, since that method is only a utility to be used by our app's UI, we decided not to add the suggested checks because we want the UI always to get some price data to display, even if it's a bit stale.
In conclusion, the reported issue doesn't affect any critical contract functionality.
(HAL-03) Lack of Integer Overflow Protection
As pointed out by the auditors, we added overflow and underflow protection to all the arithmetic operations missing that.
We added those changes in this commit: 6e70b46.
(HAL-04) Lack of Maximum Amount Check on the Exercise Flow
Suppose a user calls the exercise action on the controller with an amount argument greater than his QToken balance. In that case, we want that call to revert instead of succeeding by exercising the whole user's balance.
But when a user calls exercise with 0 as the amount, the controller exercises the total user's balance for the given QToken. We added that as a feature, so users don't need to get their balance before exercising.
(HAL-05) Weak Referral Code Generation
We didn't intend for referral codes to use cryptographically secure random numbers. We just wanted them to be the first 32 bytes of a valid string. The validateCode function only checks if a given string is less than 32 characters long, non-empty, doesn't start with 0x, and contains only digits(0-9) and uppercase and lowercase characters from A-Z or a-z.
(HAL-06) Unchecked Transfer
We fixed this issue on the following commit: 96fd197.
(HAL-07) Oracle is not activated during the first installation
Even though it would save the oracle manager some gas, as indicated by the auditors, we decided to keep the oracle addition and activation on the registry as two separate methods and oracle management steps. In other words, we want newly added oracles to be inactive by default.
(HAL-08) Front-runnable Initializers
As per the OpenZeppelin upgradable contracts package, upgradable contracts can only have a single method with the initializer modifier (usually called initialize) and not a constructor. The initializer modifier checks and guarantees that the initialize method is only called once during deployment, and the msg.sender in the context of that call is the deployer. Thus, the initialize method in our upgradeable contracts can not be front-run.
(HAL-09) Floating Pragma
All contracts are now fixed to a 0.7.6 version pragma per the following commit: 594ecaf.
The only files left unchanged were the interfaces for possible integration and interoperability reasons.
(HAL-10) Lack of Zero Address Check
Missing zero address checks were added in the commit with hash 737169d.
(HAL-11) Ignored return values
When minting spreads, we now check that the external call to collateralToken.createCollateralToken succeeds and returns the correct id: b045abb.
(HAL-12) Use of block.timestamp
After further analysis, we determined that there's no risk of MEV attacks regarding the places where we used block.timestamp. Even if a miner were to manipulate the block timestamp to an earlier time, there would still need to be a price at the option expiration in the Chainlink oracle before an attacker could exercise the option.
(HAL-13) Use of inline assembly
We recognize where issues with inline assembly could arise. Still, in the case of our protocol contracts, we're only using it for manipulating bytes in memory where there are no critical protocol operations involved and no risk of malicious manipulation by users.
(HAL-14) Experimental features enabled (ABIEncoderV2)
We checked and validated that the solidity version we're using (0.7.6) already includes fixes to the bug mentioned. The relevant fixes were added in Solidity v0.5.7: https://github.com/ethereum/solidity/releases/tag/v0.5.7.
(HAL-15) External calls within a loop
The worse that could happen is attackers running out of gas when trying to execute too many actions through the Controller. But for regular users and expected calls to the operate method, the loop over specified actions and their external calls shouldn't be a problem.
(HAL-16) Uninitialized function variable
Fixed in 5272331.
(HAL-17) For-loop over dynamic array
The functions in the ConfigTimelockController that loop over user-provided dynamic arrays can only be called by protocol multisig proposers and executors. So, in this case, there's no room for external and malicious DoS attempts.
