Hubble contest - 0x1f8b's results

Lines of code

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/Oracle.sol#L115

Vulnerability details

Vulnerability

On Oracle.sol#L115, we are using latestRoundData, but there are no validations that the data is not stale.

The current code is:

 (uint80 round, int256 latestPrice, , uint256 latestTimestamp, ) = _aggregator.latestRoundData();
        finalPrice = uint256(latestPrice);
        if (latestPrice < 0) {
            requireEnoughHistory(round);
            (round, finalPrice, latestTimestamp) = getRoundData(_aggregator, round - 1);
        }
        return (round, finalPrice, latestTimestamp);

But is missing the checks to validate the data is stale

 (uint80 round, int256 latestPrice, , uint256 latestTimestamp, answeredInRound) = _aggregator.latestRoundData();
require(latestPrice > 0, "Chainlink price <= 0");
require(latestTimestamp != 0, "Incomplete round");
require(answeredInRound >= round, "Stale price");

This could affect in all the logic, including funds.

Lines of code

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/VUSD.sol#L11

Vulnerability details

Impact

The contract use two governance model, one looks hidden.

Proof of Concept

The VUSD contract uses VanillaGovernable but inherits from ERC20PresetMinterPauserUpgradeable and this contract uses roles to use some administrative methods like pause or mint.

This two-governance model does not seem necessary and can hide or raise suspicion about a rogue pool, thus damaging the user's trust.

Recommended Mitigation Steps

Unify governance in only one, VanillaGovernable or role based.

LOW

1. Open TODO it seems that the logic is not finished and the logic might not be as expected.

• AMM.sol#L142
• AMM.sol#L555
• MarginAccount.sol#L277
• ClearingHouse.sol#L172

2. It seems that use an outdated version of openzeppelin

"@openzeppelin/contracts": "4.2.0", "@openzeppelin/contracts-upgradeable": "4.3.2",

3. The modification process of an owner is a delicate process, since the governance of our contract and therefore of the project may be at risk, for this reason it is recommended to adjust the owner’s modification logic, to a logic that allows to verify that the new owner is in fact valid and does exist. It's mandatory to create a logic of the owner’s modification where a new owner is proposed first, the owner accepts the proposal and, in this way, we make sure that there are no errors when writing the address of the new owner.

• Governable.sol#L16-L20

4. The following contracts use Governable and can be paused, so a denial of service could happend if the keys are compromised or the issue number 3 happens.

• HubbleBase.sol#L15
• VUSD.sol#L11

5. There are a lack of input checks around the contracts:

• Registry.sol#L19-L23
• Never should be 0 in VUSD.sol#L74
• 1 could be abused in Oracle.sol#L170-L171
• Is not checked that the input was already added in ClearingHouse.sol#L340-L341

6. The current lock logic does not contemplate ERC20 tokens with fee during the safeTransferFrom, therefore, the amount received by VUSD will be less than the expected. Some tokens may implement a fee during transfers, this is the case of USDT, even though the project has currently set it to 0. So, the transferFrom function would return 'true' despite receiving less than expected.

• VUSD.sol#L44-L45
• VUSD.sol#L62-L63

7. Wrong logic around the method processWithdrawals

The logic is while (i < withdrawals.length && (i - start) <= maxWithdrawalProcesses) {

If maxWithdrawalProcesses=0 and start=0 then i=0. So (0-0)<=0 == true But the expected maxWithdrawalProcesses is zero.

1. Change the incremental logic from i++ to ++i in order to save some opcodes:

• MarginAccount.sol#L331
• MarginAccount.sol#L331
• MarginAccount.sol#L331
• MarginAccount.sol#L331
• MarginAccount.sol#L331
• MarginAccount.sol#L331
• MarginAccount.sol#L373
• MarginAccount.sol#L521
• MarginAccount.sol#L552
• HubbleViewer.sol#L52
• HubbleViewer.sol#L65
• HubbleViewer.sol#L78
• HubbleViewer.sol#L95
• HubbleViewer.sol#L120
• HubbleViewer.sol#L137
• HubbleViewer.sol#L367
• ClearingHouse.sol#L122
• ClearingHouse.sol#L130
• ClearingHouse.sol#L170
• ClearingHouse.sol#L194
• ClearingHouse.sol#L251
• ClearingHouse.sol#L263
• ClearingHouse.sol#L277

2. It's possible to avoid storage access a save gas using immutable keyword for the following variables:

• marginAccount and vusd in MarginAccountHelper.sol#L15-L16

3. It's possible to save gas using unchecked cast in PRECISION.toInt256() because PRECISION is a constant

• ClearingHouse.sol#L332