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

Collected fees are never transferred out of Pool contract #60

Open
c4-bot-7 opened this issue Apr 16, 2024 · 8 comments
Open

Collected fees are never transferred out of Pool contract #60

c4-bot-7 opened this issue Apr 16, 2024 · 8 comments
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 downgraded by judge Judge downgraded the risk level of this issue M-05 primary issue Highest quality submission among a set of duplicates satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@c4-bot-7
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-04-gondi/blob/b9863d73c08fcdd2337dc80a8b5e0917e18b036c/src/lib/pools/Pool.sol#L444

Vulnerability details

Impact

Lenders in the Gondi protocol could be EOA or Gondi Pool. The Gondi Pool, an ERC4626, allows anyone to deposit funds and earn yield from lending on Gondi. When a loan is repaid or liquidated, the pool deducts a fee from the received amount before adding the rest to the pool balance. As shown in the loanRepayment() function, the fees are calculated by calling processFees() and then added to getCollectedFees. After that, the accounting function _loanTermination() is called with the amount being received - fees.

However, this fee is credited to getCollectedFees but never transferred out of the pool. As a result, these funds remain locked in the contract indefinitely.

function loanRepayment(
    uint256 _loanId,
    uint256 _principalAmount,
    uint256 _apr,
    uint256,
    uint256 _protocolFee,
    uint256 _startTime
) external override onlyAcceptedCallers {
    uint256 netApr = _netApr(_apr, _protocolFee);
    uint256 interestEarned = _principalAmount.getInterest(netApr, block.timestamp - _startTime);
    uint256 received = _principalAmount + interestEarned;
    uint256 fees = IFeeManager(getFeeManager).processFees(_principalAmount, interestEarned);
    getCollectedFees += fees; // @audit getCollectedFees is never transfer out
    _loanTermination(msg.sender, _loanId, _principalAmount, netApr, interestEarned, received - fees);
}

Proof of Concept

The processFees() function only calculates the fee but doesn't transfer anything.

function processFees(uint256 _principal, uint256 _interest) external view returns (uint256) {
    /// @dev cached
    Fees memory __fees = _fees;
    return _principal.mulDivDown(__fees.managementFee, PRECISION)
        + _interest.mulDivDown(__fees.performanceFee, PRECISION);
}

Then after getCollectedFees is credited for fees, we can see this getCollectedFees is never transferred out of the pool.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a function to collect the credited fees getCollectedFees from the pool in the FeeManager contract.

Assessed type

Other

@c4-bot-7 c4-bot-7 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Apr 16, 2024
c4-bot-7 added a commit that referenced this issue Apr 16, 2024

Verified

This commit was signed with the committer’s verified signature.
@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Apr 17, 2024
@c4-judge
Copy link
Contributor

0xA5DF marked the issue as primary issue

@0xA5DF
Copy link

0xA5DF commented Apr 17, 2024

TODO: severity

@0xend 0xend added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Apr 19, 2024
@0xend
Copy link

0xend commented Apr 19, 2024

Not sure if it's high - tend to think as high as those that would compromise user's assets. Definitely an issue though.

@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Apr 20, 2024
@c4-judge
Copy link
Contributor

0xA5DF changed the severity to 2 (Med Risk)

@0xA5DF
Copy link

0xA5DF commented Apr 20, 2024

Marking as med as fees falls under the definition 'leak of value'

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Apr 20, 2024
@c4-judge
Copy link
Contributor

0xA5DF marked the issue as selected for report

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Apr 20, 2024
@c4-judge
Copy link
Contributor

0xA5DF marked the issue as satisfactory

@0xend
Copy link

0xend commented Apr 20, 2024

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 downgraded by judge Judge downgraded the risk level of this issue M-05 primary issue Highest quality submission among a set of duplicates satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report 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

5 participants