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

Deny of service in AccountantDelegate.sweepInterest #28

Open
code423n4 opened this issue Jun 29, 2022 · 1 comment
Open

Deny of service in AccountantDelegate.sweepInterest #28

code423n4 opened this issue Jun 29, 2022 · 1 comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working 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

Lines of code

https://github.dev/Plex-Engineer/lending-market-v2/blob/2646a7676b721db8a7754bf5503dcd712eab2f8a/contracts/Accountant/AccountantDelegate.sol#L101

Vulnerability details

Impact

The sweepInterest method is susceptible to denial of service.

Proof of Concept

The logic of the sweepInterest method relative to the treasury is as follows:

		bool success = cnote.transfer(treasury, amtToSweep);
		if (!success) { revert  SweepError(treasury , amtToSweep); }
		TreasuryInterface Treas = TreasuryInterface(treasury);
		Treas.redeem(address(cnote),amtToSweep);
		require(cnote.balanceOf(treasury) == 0, "AccountantDelegate::sweepInterestError");

As you can see, amtToSweep is passed to it and redeem that amount. Later it is checked that the balance of cnote in the treasury address must be 0. However, all calculations related to amtToSweep come out of the balance of address(this) so if a third party sends a single token cnote to the address of treasury the method will be denied.

Recommended Mitigation Steps

  • Check that the balance is the same after and before the bool success = cnote.transfer(treasury, amtToSweep);
@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Jun 29, 2022
code423n4 added a commit that referenced this issue Jun 29, 2022
@nivasan1 nivasan1 added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Jul 4, 2022
@GalloDaSballo
Copy link
Collaborator

GalloDaSballo commented Aug 16, 2022

The warden has shown how, due to an incorrect invariant (treasury having zero cNote), any griefer can permanently brick the sweepInterest function.

The finding shows how a loss of yield can be achieved, so Medium Severity would be in order.

However, because:

  • an invariant was broken
  • the tokens cannot be withdrawn via an alternative method

I believe High Severity to be more appropriate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working 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

3 participants