Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA Report #115

Open
code423n4 opened this issue Feb 23, 2022 · 1 comment
Open

QA Report #115

code423n4 opened this issue Feb 23, 2022 · 1 comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Hubble QA Report

Unless otherwise noted, manual auditing and testing were done using Visual Studio Code and Remix. The audit was done from February 17-23, 2022 by ye0lde through code4rena.

Overall, I found the code to be clear to follow and read. I'd recommend the team improve the supporting documentation to give a better overall understanding of the protocol.

Findings

L-1 - No validation of parameter price in setStablePrice (Oracle.sol)

Impact

The setStablePrice function does not do any validation of the price parameter before setting stablePrice[underlying] = price. While this is a governance function, once stablePrice[underlying] is set to any non-zero value this overrides any aggregator calls made to access the current price by function getUnderlyingPrice and getUnderlyingTwapPrice. underlying is checked but not the price itself.

Proof of Concept

setStablePrice is here:
https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/Oracle.sol#L169-L172

function setStablePrice(address underlying, int256 price) external onlyGovernance {
    requireNonEmptyAddress(underlying);
    stablePrice[underlying] = price;
}

Recommended Mitigation Steps

Consider adding a check for price > 0


L-2 - Unsafe type cast in getTwapPrice (AMM.sol)

Impact

getTwapPrice performs an unsafe type cast to uint128 without checking if the value actually fits into 128 bits. This typecast doesn't seem to directly lead to an exploit but safe typecasts should still be implemented for additional security.

Proof of Concept

The type cast is here:
https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/AMM.sol#L392

Recommended Mitigation Steps

I suggest the following change:

function getTwapPrice(uint256 _intervalInSeconds) public view returns (int256) {
    return (_calcTwap(_intervalInSeconds).toInt256()); 
}

NC-1 - Typo in getUnderlyingTwapPrice (Oracle.sol)

Impact

Code clarity

Proof of Concept

The typo is here:
https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/Oracle.sol#L53

Recommended Mitigation Steps

Change form to from.


@code423n4 code423n4 added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax bug Something isn't working labels Feb 23, 2022
code423n4 added a commit that referenced this issue Feb 23, 2022
@atvanguard atvanguard added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Feb 26, 2022
@atvanguard
Copy link
Collaborator

Good minor find.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

2 participants