Rigor Protocol contest - TrungOre's results

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Community.sol#L684-L694

Vulnerability details

Impact

Community's owner lose amount of interest from project (up to half of total interest)

Proof of concept

When builder repay any loan amount by function repayLender() (or community call function lendToProject()), function claimInterest() in contract Community will be called and then calculate the interest in function returnToLender() It will calculate number of days difference current and last timestamp

        uint256 _noOfDays = (block.timestamp -
            _communityProject.lastTimestamp) / 86400; // 24*60*60

Here, the calculation skipped (block.timestamp - _communityProject.lastTimestamp) % 86400 seconds. The more number of calls repayLender() and lendToProject(), the more time was skipped. After that:

uint256 _unclaimedInterest = 
                _lentAmount *
                _communities[_communityID].projectDetails[_project].apr *
                _noOfDays /
                365000;

When _unclaimedInterest > 0 (means _noOfDays > 0), variable lastTimestamp will be updated in function claimInterst(). Then amount of redundant seconds will be accrued (can up to a half of total time).

Example scenerio:

Builder will call repayLender() (any amount > 0, such as 1 wei) continuously with number of seconds difference is smaller and close to 86400 * 2. After 1 year (365 days), the total interest was calculated by about 183 days, then community's owner has lost almost half of total interest which should be claimed from this project.

Tools Used

Manual review

Recommended Mitigation Steps

Should calculate the interest based unit seconds instead of days.

uint256 _differentTime = block.timestamp - _communityProject.lastTimestamp;

uint256 _unclaimedInterest = 
                _lentAmount *
                _communities[_communityID].projectDetails[_project].apr *
                differentTime /
                (365000 * 86400);

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L87-L89 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L216-L230

Vulnerability details

Impact

Lose control of deployed proxy

Proof of concept

HomeFi Proxy is responsible for initializing all the modules contract in the correct sequential order and generate upgradable proxy for them. But after generating proxy for each module, it didn't call initialize for that contract. It can lead to scenario that attacker/miner can front-run to initialize() the contract first and take control of that contract.
In some cases, because of the carelessness of deployer, they doesn't check the transaction initialize() that they call reverted or not can lead to some bad scenarios (because the module contracts is controlled by someone else).

Tools Used

Manual review

Recommended Mitigation Steps

call initialize() after generateProxy() in the same transaction

[2022-08-rigor] QA report

tags: c4, 2022-08-rigor, QA

Unused variable

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L33-L35

tokenCurrency1, tokenCurrency2, tokenCurrency3 is not used anywhere in the contract except setting it equal to homeFi.tokenCurrency in initialize() function. Consider to remove these variable

function mintNFT() no need to return

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/HomeFi.sol#L284-L297 projectCount is global variable so it can be used instead of return variable from mintNFT(). And the return variable of mintNFT() has been never used. https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/HomeFi.sol#L225