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

Builder can lock in a temporarily low lender fee for all future projects #15

Open
code423n4 opened this issue Aug 2, 2022 · 1 comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons valid

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L102

Vulnerability details

Impact

The admin has the capability to adjust the lender fee by calling the replaceLenderFee() function in the HomeFi.sol contract.

However, users have the ability to lock in future projects at the current 0.5% rate. If the rate is ever temporarily lowered or set to zero, they will also be able to lock in at that lower rate.

This happens because the lender fee is saved in the project initialization (when the createProject() function is called), and not pulled dynamically from the HomeFi.sol contract at the time of the loan. Since users can create unlimited projects at any time without any specific commitment, they can create an arbitrary number of contracts with the lower rate locked in for future use.

Proof of Concept

When createProject() is called, it calls out to the project factory's createProject() function.

address _project = projectFactoryInstance.createProject(
    _currency,
    _sender
);

This function clones and initializes a new instance of Project.sol:

// Create clone of Project implementation
_clone = ClonesUpgradeable.clone(underlying);

// Initialize project
Project(_clone).initialize(_currency, _sender, homeFi);

Finally, this initialize() function sets the project's parameters, including calling out to HomeFi.sol to fetch the current lender fee:

function initialize(address _currency, address _sender, address _homeFiAddress) external override initializer {
    // Initialize 
    homeFi = IHomeFi(_homeFiAddress);
    disputes = homeFi.disputesContract();
    lenderFee = homeFi.lenderFee();
    builder = _sender; 
    currency = IDebtToken(_currency);
}

This permanently sets the lender fee for the project to homeFi.lenderFee() at the time it is created, with no mechanism to update or override this fee.

As a result, when _lenderFee is calculated and sent to the treasury in the lendToProject() function of Community.sol, it is fixed at the original rate.

// Calculate lenderFee
uint256 _lenderFee = (_lendingAmount * _projectInstance.lenderFee()) / (_projectInstance.lenderFee() + 1000);

Tools Used

VS Code, vim, hardhat

Recommended Mitigation Steps

Community.sol can call directly to the HomeFi contract to get the current lenderFee() at the time the lendToProject() function is called.

uint256 _lenderFee = (_lendingAmount * homeFi.lenderFee()) / (homeFi.lenderFee() + 1000);

This will allow the protocol to dynamically update the lender fee and have the result of updating all projects going forward, as well as stopping malicious users from locking in low lender fees.

@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 Aug 2, 2022
code423n4 added a commit that referenced this issue Aug 2, 2022
@zgorizzo69
Copy link
Collaborator

zgorizzo69 commented Aug 8, 2022

lenderfee is meant to be fixed once the project is created

@zgorizzo69 zgorizzo69 added disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue labels Aug 8, 2022
@parv3213 parv3213 added sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons and removed disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue labels Aug 18, 2022
@jack-the-pug jack-the-pug added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax valid and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons valid
Projects
None yet
Development

No branches or pull requests

4 participants