search

code-423n4 / 2023-05-asymmetry-mitigation-findings

2 stars 2 forks source link

Mitigation of M-10: Issue not mitigated #54

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

MITIGATION IS NOT CONFIRMED

MITIGATION IS NOT CONFIRMED

Mitigation of M-10: Issue not mitigated

Link to Issue: https://github.com/code-423n4/2023-03-asymmetry-findings/issues/363

Comments

Even though the protocol team applied the warden's recommendation in M-10, the feature to enable/disable derivatives added as a mitigation for H-03/M-02/M-05 potentially reintroduces the issue.

Technical details

The following pull request https://github.com/asymmetryfinance/smart-contracts/pull/264/files adds a feature to enable or disable derivatives. As a result of this change, it is now possible to have at least one derivative, such that derivativeCount > 0, but have that derivative disabled.

Given this scenario, the added check in https://github.com/asymmetryfinance/smart-contracts/pull/208/files#diff-badfabc2bc0d1b9ef5dbef737cd03dc2f570f6fd2074aea9514da9db2fff6e4eR67 will succeed, while the deposit in the for-loop will be skipped as the derivative is disabled here https://github.com/asymmetryfinance/smart-contracts/pull/264/files#diff-badfabc2bc0d1b9ef5dbef737cd03dc2f570f6fd2074aea9514da9db2fff6e4eR86

This effectively reintroduces the conditions for the original issue described in M-10.

Recommendation

The check should only consider enabled derivatives. Alternatively the check could be done using the totalWeight variable, as this variable considers only enabled derivatives, and will be zero if all derivatives are disabled.

elmutt commented 1 year ago

we consider this edge case acceptable in order to keep code changes to a minimum

d3e4 commented 1 year ago

The suggested check using totalWeight has indeed been implemented. See #38.

romeroadrian commented 1 year ago

The suggested check using totalWeight has indeed been implemented. See #38.

I think that change should be out of scope as it is not part of the pull requests in scope. Need to double check though.

romeroadrian commented 1 year ago

Yes, that change is part of the following commit https://github.com/asymmetryfinance/smart-contracts/commit/75c4a6f5abe2ee6ae434fba6dc24845588b6ca02, which isn't part of PR #208 or #264

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory

c4-judge commented 1 year ago

Picodes marked the issue as primary issue