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

Swivel lend method doesn't pull protocol fee from user #201

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

Swivel lend method doesn't pull protocol fee from user #201

code423n4 opened this issue Jun 26, 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 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/main/lender/Lender.sol#L297

Vulnerability details

The Swivel lend method adds to fees[u] the order fee, but does not pull that fee from the user. It only pulls the order-post-fee amount.

Impact

withdrawFee will fail, as it tries to transfer more tokens than are in the contract.

Proof of Concept

The Swivel lend method sums up the fees to totalFee, and the amount to send to Swivel in lent:

                    totalFee += fee;
                    // Amount lent for this order
                    uint256 amountLent = amount - fee;
                    // Sum the total amount lent to Swivel (amount of ERC5095 tokens to mint) minus fees
                    lent += amountLent;

It then increments fees[u] by totalFee, but only pulls from the user lent:

            fees[u] += totalFee;
            // transfer underlying tokens from user to illuminate
            Safe.transferFrom(IERC20(u), msg.sender, address(this), lent);

Therefore, totalFee has not been pulled from the user.
The fees variable now includes tokens which are not in the contract, and withdrawFee will fail as it tries to transfer fees[u].

Recommended Mitigation Steps

Pull lent + totalFee from the user.

@sourabhmarathe
Copy link
Collaborator

There were several issues that marked this ticket as a high-severity issue. Because the current code does not put user funds at risk (that otherwise would not be spent), we believe this issue is a Med Risk severity issue.

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 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

2 participants