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

implicit underflows #6

Open
code423n4 opened this issue Jul 5, 2021 · 1 comment
Open

implicit underflows #6

code423n4 opened this issue Jul 5, 2021 · 1 comment
Labels

Comments

@code423n4
Copy link
Contributor

Handle

gpersoon

Vulnerability details

Impact

There are a few underflows that are converted via a typecast afterwards to the expected value. If solidity 0.8.x would be used, then the code would revert.
int256(a-b) where a and b are uint, For example if a=1 and b=2 then the intermediate result would be uint(-1) == 2256-1
int256(-x) where x is a uint. For example if x=1 then the intermediate result would be uint(-1) == 2
256-1
Its better not to have underflows by using the appropriate typecasts.
This is especially relevant when moving to solidity 0.8.x

Proof of Concept

// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/insurance/Exposure.sol#L178
function sortVaultsByDelta(..)
..
for (uint256 i = 0; i < N_COINS; i++) {
// Get difference between vault current assets and vault target
int256 delta = int256(unifiedAssets[i] - unifiedTotalAssets.mul(targetPercents[i]).div(PERCENTAGE_DECIMAL_FACTOR)); // underflow in intermediate result

//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/pnl/PnL.sol#L112
function decreaseGTokenLastAmount(bool pwrd, uint256 dollarAmount, uint256 bonus)...
..
emit LogNewGtokenChange(pwrd, int256(-dollarAmount)); // underflow in intermediate result

// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/pools/oracle/Buoy3Pool.sol#L87
function safetyCheck() external view override returns (bool) {
...
_ratio = abs(int256(_ratio - lastRatio[i])); // underflow in intermediate result

Tools Used

Recommended Mitigation Steps

replace int256(a-b) with int256(a)-int256(b)
replace int256(-x) with -int256(x)

@code423n4 code423n4 added 1 (Low Risk) bug Something isn't working labels Jul 5, 2021
code423n4 added a commit that referenced this issue Jul 5, 2021
@kitty-the-kat kitty-the-kat added sponsor confirmed duplicate This issue or pull request already exists labels Jul 14, 2021
@flabble-gro flabble-gro removed the duplicate This issue or pull request already exists label Jul 14, 2021
@ghoul-sol
Copy link
Collaborator

Majority of overflow listed above seems low risk with one exception of safetyCheck. Underflow is a real risk here.safetyCheck is run every time a deposit is made. Ratios can change and the change does not need to be substantial for it to overflow. For that reason it's a high risk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants