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

getInterestOverdue reverts rather than returning 0 when there is no overdue interest #74

Open
code423n4 opened this issue Dec 14, 2021 · 2 comments
Labels
0 (Non-critical) Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas optimisation bug Something isn't working sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@code423n4
Copy link
Contributor

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Potentially unexpected reverts when being called by external code

Proof of Concept

Here we revert if there's no extension (and therefore no overdue interest)

https://github.com/code-423n4/2021-12-sublime/blob/9df1b7c4247f8631647c7627a8da9bdc16db8b11/contracts/Pool/Repayments.sol#L300

I would expect in this case to receive a return value of zero as it's a truthful representation of the current state of the pool.

Otherwise if I were to call this from a contract I would have to wrap this function call as we do in _repayExtension which will increase costs as I need to make two calls to this contract.

https://github.com/code-423n4/2021-12-sublime/blob/9df1b7c4247f8631647c7627a8da9bdc16db8b11/contracts/Pool/Repayments.sol#L325-L335

Low risk as it may cause unexpected reverts in integrators' code but unlikely to cause any loss of funds. At the very least this can be a gas optimisation by removing the if statement in _repayExtension and replacing it with an early return for zero interest.

Recommended Mitigation Steps

Remove require statement and replace it with returning zero if there's no extension.

@code423n4 code423n4 added 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working labels Dec 14, 2021
code423n4 added a commit that referenced this issue Dec 14, 2021
@ritik99 ritik99 added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Dec 23, 2021
@ritik99
Copy link
Collaborator

ritik99 commented Dec 23, 2021

Once an extension is repaid, isLoanExtensionActive turns false (here) because of which we do return 0 in the corresponding else block.

Whether getInterestOverdue reverts/returns 0 is honestly a matter of choice, either way does not seem to be an issue

@0xean
Copy link
Collaborator

0xean commented Jan 21, 2022

Moving to non-critical

@0xean 0xean added 0 (Non-critical) Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas optimisation and removed 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments labels Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 (Non-critical) Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas optimisation bug Something isn't working sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

3 participants