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

MochiVault.flashFee() May Truncate Result #171

Open
code423n4 opened this issue Oct 28, 2021 · 2 comments
Open

MochiVault.flashFee() May Truncate Result #171

code423n4 opened this issue Oct 28, 2021 · 2 comments
Labels
1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working

Comments

@code423n4
Copy link
Contributor

Handle

leastwood

Vulnerability details

Impact

The flashFee() function differs to the implementation found in usdm.flashFee() due to a missing SCALE variable which ensures the result is not truncated. The end result for MochiVault.flashFee() is that users could end up paying a slightly lower fee than intended if _amount is somewhat small.

This could abused by calling flashLoan() with a small _amount input and continually reentering the contract before using the actual funds. This enables a user to pay marginally less in fees as compared to a typical user.

If the asset being held in the vault is configured with a low decimals value, the degree of truncation could be significant, potentially resulting in little to no fees paid back into MochiVault.sol.

Proof of Concept

https://github.com/code-423n4/2021-10-mochi/blob/main/projects/mochi-core/contracts/vault/MochiVault.sol#L345-L354

Tools Used

Manual code review

Recommended Mitigation Steps

Consider updating return (_amount * 1337) / 1000000; in MochiVault.flashLoan() to return (_amount * ((1337 * SCALE) / 1000000)) / SCALE; where SCALE = 1e18.

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Oct 28, 2021
code423n4 added a commit that referenced this issue Oct 28, 2021
@r2moon r2moon added resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") invalid This doesn't seem right and removed resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") labels Oct 29, 2021
@r2moon
Copy link
Collaborator

r2moon commented Oct 29, 2021

return (_amount * 1337) / 1000000; and return (_amount * ((1337 * SCALE) / 1000000)) / SCALE; could return same amount i think.

@ghoul-sol
Copy link
Collaborator

I see a risk of rounding errors but I think this is low risk since small amount flash loans are inpractical

@ghoul-sol ghoul-sol added 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments and removed invalid This doesn't seem right 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants