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

Function lend is not transfering in protocol fees #291

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

Function lend is not transfering in protocol fees #291

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 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/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L641-L654

Vulnerability details

The function lend is calculating the total sum of tokens lent to swivel, as well as summing up the fees totalFee

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L278-L297

                    // Track accumulated fees
                    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;
                    // Sum the total amount of premium paid from Swivel (amount of underlying to lend to yield)
                    returned += amountLent * order.premium / order.principal;
                }

                unchecked {
                    i++;
                }
            }
      
            // Track accumulated fee
            fees[u] += totalFee;

            // transfer underlying tokens from user to illuminate
            Safe.transferFrom(IERC20(u), msg.sender, address(this), lent);

However, when it comes to transferring in the underlying, only lent is transferred in.

Meaning that the contract will account for fees it didn't receive, breaking fees accounting for that token as well as unfairly making minting of the principal cheaper via this function call

Mitigation steps

Transfer in lent + totalFee

Change

Safe.transferFrom(IERC20(u), msg.sender, address(this), lent);

To

Safe.transferFrom(IERC20(u), msg.sender, address(this), lent + totalFee);
@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 Jun 26, 2022
code423n4 added a commit that referenced this issue Jun 26, 2022
@KenzoAgada
Copy link

Duplicate of #201

@sourabhmarathe sourabhmarathe added 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") labels Jun 29, 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 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