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

Multiple calculations and parameter usage errors in the lend function for swivel #43

Closed
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 duplicate This issue or pull request already exists

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L294-L295

Vulnerability details

Impact

Multiple calculations and parameter usage errors in the lend function for swivel

  1. transferFrom should use the accumulated value of a instead of lent
  2. initiate should use a - fee instead of a
  3. if the yeild function is swapping principalToken for illuminateToken, the last argument should be msg.sender
  4. returned variable calculation should avoid rounding

Proof of Concept

https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L294-L295

Tools Used

None

Recommended Mitigation Steps

Rewrite the lend function for swivel as follows

    function lend(
        uint8 p,
        address u,
        uint256 m,
        uint256[] calldata a,
        address y,
        Swivel.Order[] calldata o,
        Swivel.Components[] calldata s
    ) public unpaused(p) returns (uint256) {

        uint256 sum;
        // returned represents the number of underlying tokens to lend to yield
        uint256 returned;

        {
            uint256 totalFee;
            uint256[o.length] new_a;
            // iterate through each order a calculate the total lent and returned
            for (uint256 i = 0; i < o.length; ) {
                Swivel.Order memory order = o[i];
                // Require the Swivel order provided matches the underlying and maturity market provided
                if (order.underlying != u) {
                    revert NotEqual('underlying');
                } else if (order.maturity > m) {
                    revert NotEqual('maturity');
                }
                // Determine the fee
                uint256 fee = calculateFee(a[i]);
                // Track accumulated fees
                totalFee += fee;
                // Sum the total amount lent to Swivel (amount of ERC5095 tokens to mint) minus fees
                sum += a[i];
                new_a[i] = a[i] - fee;
                // Sum the total amount of premium paid from Swivel (amount of underlying to lend to yield)
                returned += (a[i] - fee) * 1e18 / order.principal * order.premium / 1e18;

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

            // transfer underlying tokens from user to illuminate
            Safe.transferFrom(IERC20(u), msg.sender, address(this), sum);
            // fill the orders on swivel protocol
            ISwivel(swivelAddr).initiate(o, new_a, s);
            uint256 lent = yield(u, y, returned, msg.sender);
        }

        emit Lend(p, u, m, lent);
        return lent;
    }
@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
@sourabhmarathe
Copy link
Collaborator

Duplicate of #201.

@sourabhmarathe sourabhmarathe added the duplicate This issue or pull request already exists label Jun 30, 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 duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

3 participants