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

Lend() for swivel will run out of funds when filling orders #14

Closed
code423n4 opened this issue Jun 23, 2022 · 1 comment
Closed

Lend() for swivel will run out of funds when filling orders #14

code423n4 opened this issue Jun 23, 2022 · 1 comment
Labels
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 disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) duplicate This issue or pull request already exists 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.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L247-L305

Vulnerability details

Impact

lend will always fail if there is feenominator > 0 and there are not enough fees already in the contract to cover amount short

Proof of Concept

L297 transfers the "lent" amount calculated in L283

https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L297

https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L283

L299 makes the call to swivel with the full value of specified in the input a

https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L299

Let feenominator be set to 100 (1% fee). Let a simply be [100]. Calculated in L283 lent = 100 - 1 = 99. L297 will only transfer 99 underlying token into the contract. It will then use 100 tokens in the subsequent call in L299 as specified by a. If there are not already enough tokens in the contract then the call in L299 will fail.

Tools Used

Recommended Mitigation Steps

L299 should transfer in the sum of a rather than the sum of (a[i] - fee)

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Jun 23, 2022
code423n4 added a commit that referenced this issue Jun 23, 2022
@KenzoAgada
Copy link

Duplicate of #201. I believe warden got confused in the line number in the mitigation - by the rest of the description he refers to the line that transfers funds from sender to lender, not L299.

@sourabhmarathe sourabhmarathe added duplicate This issue or pull request already exists disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") labels Jun 29, 2022
@gzeoneth gzeoneth added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Jul 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) duplicate This issue or pull request already exists 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

4 participants