Rigor Protocol contest - simon135's results

Lines of code

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

Vulnerability details

Impact

If builder makes a project,then a day later the builder gets everything done and wants to repay the lender.RepaysLender in that function it calls _reduceDebt.claimInterest.returnToLender() and if noOfDays = 0 your unclaimedInterset =0 it lowers the builders Interest to 0 and you still have _lentAmount left over causing tokens that the builder borrowed arent getting intrest on it.

Proof of Concept

1.attacker calls repayLender which calls _reduceDebt then calls claimInterest then returnToLender note: the diffrent in the timestamp is one day august 6 to the 5 at the same time of the day. block.timestmap= (1659817999- 1659732167) / 86400 = 0 because of percsion error unclaimedInterest = 0 and totalInsterest =5 (lets say 5 )

---

returnToLender function

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

        /// Interest formula = (principal * APR * days) / (365 * 1000)
        // prettier-ignore
        uint256 _unclaimedInterest = 
                _lentAmount *
                _communities[_communityID].projectDetails[_project].apr *
                _noOfDays /
                365000;

        // Old (already rTokens claimed) + new interest
        uint256 _totalInterest = _unclaimedInterest +
            _communityProject.interest;

claimInterest function

• the if statement never gets called because unclaimedInterest = 0 = _interestEarned

      if (_interestEarned > 0) {
            // If any new interest is to be claimed.

            // Increase total interest with new interest to be claimed.
            _communityProject.interest = _interest;


            // Update lastTimestamp to current time.
            _communityProject.lastTimestamp = block.timestamp;

            // Burn _interestEarned amount wrapped token to lender
            _wrappedToken.mint(_lender, _interestEarned);

repayLender function

• attacker has alot more repayAmount (100) then _interest(5)`
• _lentAndInterest = 100 + 5= 115 which is greater then 100
• and then _interest =0 and _lentAmount=15

        uint256 _lentAmount = _communityProject.lentAmount;
        uint256 _interest = _communityProject.interest;

        if (_repayAmount > _interest) {
            // If repayment amount is greater than interest then
            // set lent = lent + interest - repayment.
            // And set interest = 0.
            uint256 _lentAndInterest = _lentAmount + _interest;

            // Revert if repayment amount is greater than sum of lent and interest.
            require(_lentAndInterest >= _repayAmount, "Community::!Liquid");
            _interest = 0;
            _lentAmount = _lentAndInterest - _repayAmount;
        } else {
            // If repayment amount is lesser than interest, then set
            // interest = interest - repayment
            _interest -= _repayAmount;
        }

        // Update community  project details
        _communityProject.lentAmount = _lentAmount;
        _communityProject.interest = _interest;

• attacker/builder at the end gives back the debt tokens but hes not done borrowing funds. This can make the issue worse because there is no pressure or insurance that the community is getting there funds back.

At the end of this brief window of time an builder of the project can still have borrowable assets that arent geting intrested on which can be considerd stealing funds. The reason am rating this as medium is because the attack window is only a day or so and to only have the borrow asset ready to be gived back in a day is not easy.

Tools Used

vim

Recommended Mitigation Steps

• in the returnToLender function add a check to make sure that _noOfDays is > 0;

require(_noOfDays > 0);

block.timestamp can overflow in year 2106

make the variable assigning block.timestapm caluction unchecked to not cause a revert Community.sol:685

if lender dosnt have enough balance to burn _repayAmount then the builder will be owed to pay more intrest

This can happen when a community has too may projects that they are lending to.Which will cause balance[_lender] =0 and the function will revert.

a malious admin can change lenderFee with no maxiaum and no timelock

which will cause a big lenderFee make maxiaum value and a timelock also do this with trustedForwarder

this comment is wrong

because you can make project contract that you input and get what every address out of getTask function then the pass goes through even if your not a role but there is no impact because of how its implemtened in the code.

        // Local instance of variable. For saving gas.
        IProject _projectInstance = IProject(_project);

        // Get task subcontractor
        (, address _sc, ) = _projectInstance.getTask(_taskID);

        // Revert is signer is not builder, contractor or subcontractor.
        bool _result = _projectInstance.builder() == _address ||
            _projectInstance.contractor() == _address ||
            _sc == _address;

Disputes.sol:177

no address zero check

instance includes: HomeFi.sol:114

treasury = _treasury;

no emit of old address of admin or importent variable

emit AdminReplaced(_newAdmin);

instead make

address _oldAdmin=admin;
admin=_newAdmin
emit AdminReplaced(_newAdmin,_oldAdmin);

HomeFi.sol:157,17185,201

make importent states changing functions have a timelock just a best practice

HomeFi.sol:157,17185,201

Using bools for storage incurs overhead

// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.

you can re order storage variables to save 20_000 gas

   address public override contractor;
    /// @inheritdoc IProject
    bool public override contractorConfirmed;
    /// @inheritdoc IProject
    uint256 public override hashChangeNonce;
    /// @inheritdoc IProject
    uint256 public override totalLent;
    /// @inheritdoc IProject
    uint256 public override totalAllocated;
    /// @inheritdoc IProject
    uint256 public override taskCount;
    /// @inheritdoc IProject
    bool public override contractorDelegated;
    /// @inheritdoc IProject
    uint256 public override lastAllocatedTask;
    /// @inheritdoc IProject
    uint256 public override lastAllocatedChangeOrderTask;

you can take the bool under the other bool because a bool only takes up 1 byte other wise it would take up one slot. https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/Project.sol#L70

make i++ into ++i to remove a memory copy

HomeFiProxy.sol:87:        for (uint256 i = 0; i < _length; i++) {
HomeFiProxy.sol:136:        for (uint256 i = 0; i < _length; i++) {
libraries/Tasks.sol:181:        for (uint256 i = 0; i < _length; i++) _alerts[i] = _self.alerts[i];
Project.sol:249:        for (uint256 i = 0; i < _length; i++) {
Project.sol:312:        for (uint256 i = 0; i < _length; i++) {
Project.sol:323:        for (uint256 i = 0; i < _length; i++) {
Project.sol:604:            for (; i < _changeOrderedTask.length; i++) {
Community.sol:625:        for (uint256 i = 0; i < _communities[_communityID].memberCount; i++) {

you can make arrary.length into memory variable to save gas

Project.sol:604:            for (; i < _changeOrderedTask.length; i++) {
Community.sol:625:        for (uint256 i = 0; i < _communities[_communityID].memberCount; i++) {

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) Source Custom Errors in Solidity: Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them. Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

HomeFiProxy.sol:41:        require(_address != address(0), "Proxy::0 address");
HomeFiProxy.sol:81:        require(_length == _implementations.length, "Proxy::Lengths !match");
HomeFiProxy.sol:105:        require(
HomeFiProxy.sol:133:        require(_length == _contractAddresses.length, "Proxy::Lengths !match");
libraries/Tasks.sol:44:        require(_self.state == TaskStatus.Inactive, "Task::active");
libraries/Tasks.sol:50:        require(_self.state == TaskStatus.Active, "Task::!Active");
libraries/Tasks.sol:56:        require(
libraries/Tasks.sol:124:        require(_self.subcontractor == _sc, "Task::!SC");
ProjectFactory.sol:36:        require(_address != address(0), "PF::0 address");
ProjectFactory.sol:64:        require(
ProjectFactory.sol:84:        require(_msgSender() == homeFi, "PF::!HomeFiContract");
Disputes.sol:39:        require(_address != address(0), "Disputes::0 address");
Disputes.sol:46:        require(homeFi.admin() == _msgSender(), "Disputes::!Admin");
Disputes.sol:52:        require(homeFi.isProjectExist(_msgSender()), "Disputes::!Project");
Disputes.sol:61:        require(
Disputes.sol:106:        require(
Disputes.sol:183:        require(_result, "Disputes::!Member");
HomeFi.sol:73:        require(admin == _msgSender(), "HomeFi::!Admin");
HomeFi.sol:78:        require(_address != address(0), "HomeFi::0 address");
HomeFi.sol:84:        require(_oldAddress != _newAddress, "HomeFi::!Change");
HomeFi.sol:142:        require(!addrSet, "HomeFi::Set");
HomeFi.sol:192:        require(lenderFee != _newLenderFee, "HomeFi::!Change");
HomeFi.sol:256:        require(
DebtToken.sol:31:        require(
DebtToken.sol:50:        require(_communityContract != address(0), "DebtToken::0 address");
Project.sol:123:        require(!contractorConfirmed, "Project::GC accepted");
Project.sol:132:        require(_projectAddress == address(this), "Project::!projectAddress");
Project.sol:135:        require(_contractor != address(0), "Project::0 address");
Project.sol:150:        require(_msgSender() == builder, "Project::!B");
Project.sol:153:        require(contractor != address(0), "Project::0 address");
Project.sol:166:        // Check for required signatures
Project.sol:176:        require(_nonce == hashChangeNonce, "Project::!Nonce");
Project.sol:189:        require(
Project.sol:195:        require(_cost > 0, "Project::!value>0");
Project.sol:199:        require(
Project.sol:202:            "Project::value>required"
Project.sol:226:            // Check for required signatures
Project.sol:239:        require(_taskCount == taskCount, "Project::!taskCount");
Project.sol:242:        require(_projectAddress == address(this), "Project::!projectAddress");
Project.sol:246:        require(_length == _taskCosts.length, "Project::Lengths !match");
Project.sol:278:        require(_nonce == hashChangeNonce, "Project::!Nonce");
Project.sol:302:        require(
Project.sol:309:        require(_length == _scList.length, "Project::Lengths !match");
Project.sol:342:        require(_projectAddress == address(this), "Project::!Project");
Project.sol:370:                require(tasks[_taskID].getState() == 3, "Project::!Complete");
Project.sol:402:            // Check for required signatures.
Project.sol:407:        require(_project == address(this), "Project::!projectAddress");
Project.sol:435:                    // As this additional amount may not be required by the project.
Project.sol:512:        require(_project == address(this), "Project::!projectAddress");
Project.sol:516:            require(
Project.sol:522:            require(
Project.sol:531:                require(getAlerts(_task)[2], "Project::!SCConfirmed");
Project.sol:755:        require(_sc != address(0), "Project::0 address");
Project.sol:888:        require(
Project.sol:908:        require(
Community.sol:69:        require(_address != address(0), "Community::0 address");
Community.sol:75:        require(_msgSender() == homeFi.admin(), "Community::!admin");
Community.sol:81:        require(
Community.sol:90:        require(
Community.sol:131:        require(
Community.sol:159:        require(
Community.sol:191:        require(
Community.sol:235:        require(
Community.sol:241:        require(homeFi.isProjectExist(_project), "Community::Project !Exists");
Community.sol:248:        require(_community.isMember[_builder], "Community::!Member");
Community.sol:251:        require(
Community.sol:312:        require(
Community.sol:347:        require(
Community.sol:353:        require(
Community.sol:384:        require(
Community.sol:400:        require(
Community.sol:492:        require(
Community.sol:537:        require(_builder == _projectInstance.builder(), "Community::!Builder");
Community.sol:540:        require(
Community.sol:558:        require(!restrictedToAdmin, "Community::restricted");
Community.sol:569:        require(restrictedToAdmin, "Community::!restricted");
Community.sol:748:        // then this fee will be required to be paid again.
Community.sol:769:        require(_repayAmount > 0, "Community::!repay");
Community.sol:797:            require(_lentAndInterest >= _repayAmount, "Community::!Liquid");
Community.sol:893:        require(
mock/HomeFiMock.sol:42:        require(admin == _msgSender(), "HomeFi::!Admin");
mock/HomeFiMock.sol:47:        require(_address != address(0), "HomeFi::0 address");
mock/HomeFiMock.sol:96:        require(!addrSet, "HomeFi::Set");
mock/HomeFiMock.sol:113:        require(admin != _newAdmin, "HomeFi::!Change");
mock/HomeFiMock.sol:124:        require(treasury != _newTreasury, "HomeFi::!Change");
mock/HomeFiMock.sol:134:        require(lenderFee != _newLenderFee, "HomeFi::!Change");
mock/HomeFiMock.sol:175:        require(
mock/HomeFiV2Mock.sol:19:        require(!addrSet2, "Already set once");
interfaces/IProject.sol:38:     * @notice initialize this contract with required parameters. This is initialized by HomeFi contract.
interfaces/IProject.sol:67:     * @param _signature bytes representing signature on _data by required members.
interfaces/IProject.sol:89:     * @param _signature bytes representing signature on _data by required members.
interfaces/IProject.sol:133:     * @param _signature bytes representing signature on _data by required members.
interfaces/IProject.sol:168:     * @param _signature bytes representing signature on _data by required members.
interfaces/IProject.sol:196:     * @param _signature bytes representing signature on _data by required members.
interfaces/ICommunity.sol:23:        uint256 lendingNeeded; // total lending requirement from the community
interfaces/ICommunity.sol:27:        uint256 publishFee; // fee required to be paid to request funds from community
interfaces/ICommunity.sol:162:     * - _publishFee uint256 - project publish fee. This fee is required to be paid before `toggleLendingNeeded` can be called.
interfaces/ICommunity.sol:186:     * This fee is required to be paid before `toggleLendingNeeded` can be called. Hence before asking for any lending.
interfaces/IHomeFi.sol:28:     * @notice initialize this contract with required parameters.
simon@pop-os:~/solidity/code4/2022-08-rigor/contracts$ grep -Rn "" | grep require
HomeFiProxy.sol:41:        require(_address != address(0), "Proxy::0 address");
HomeFiProxy.sol:81:        require(_length == _implementations.length, "Proxy::Lengths !match");
HomeFiProxy.sol:105:        require(
HomeFiProxy.sol:133:        require(_length == _contractAddresses.length, "Proxy::Lengths !match");
libraries/Tasks.sol:44:        require(_self.state == TaskStatus.Inactive, "Task::active");
libraries/Tasks.sol:50:        require(_self.state == TaskStatus.Active, "Task::!Active");
libraries/Tasks.sol:56:        require(
libraries/Tasks.sol:124:        require(_self.subcontractor == _sc, "Task::!SC");
ProjectFactory.sol:36:        require(_address != address(0), "PF::0 address");
ProjectFactory.sol:64:        require(
ProjectFactory.sol:84:        require(_msgSender() == homeFi, "PF::!HomeFiContract");
Disputes.sol:39:        require(_address != address(0), "Disputes::0 address");
Disputes.sol:46:        require(homeFi.admin() == _msgSender(), "Disputes::!Admin");
Disputes.sol:52:        require(homeFi.isProjectExist(_msgSender()), "Disputes::!Project");
Disputes.sol:61:        require(
Disputes.sol:106:        require(
Disputes.sol:183:        require(_result, "Disputes::!Member");
HomeFi.sol:73:        require(admin == _msgSender(), "HomeFi::!Admin");
HomeFi.sol:78:        require(_address != address(0), "HomeFi::0 address");
HomeFi.sol:84:        require(_oldAddress != _newAddress, "HomeFi::!Change");
HomeFi.sol:142:        require(!addrSet, "HomeFi::Set");
HomeFi.sol:192:        require(lenderFee != _newLenderFee, "HomeFi::!Change");
HomeFi.sol:256:        require(
DebtToken.sol:31:        require(
DebtToken.sol:50:        require(_communityContract != address(0), "DebtToken::0 address");
Project.sol:123:        require(!contractorConfirmed, "Project::GC accepted");
Project.sol:132:        require(_projectAddress == address(this), "Project::!projectAddress");
Project.sol:135:        require(_contractor != address(0), "Project::0 address");
Project.sol:150:        require(_msgSender() == builder, "Project::!B");
Project.sol:153:        require(contractor != address(0), "Project::0 address");
Project.sol:166:        // Check for required signatures
Project.sol:176:        require(_nonce == hashChangeNonce, "Project::!Nonce");
Project.sol:189:        require(
Project.sol:195:        require(_cost > 0, "Project::!value>0");
Project.sol:199:        require(
Project.sol:202:            "Project::value>required"
Project.sol:226:            // Check for required signatures
Project.sol:239:        require(_taskCount == taskCount, "Project::!taskCount");
Project.sol:242:        require(_projectAddress == address(this), "Project::!projectAddress");
Project.sol:246:        require(_length == _taskCosts.length, "Project::Lengths !match");
Project.sol:278:        require(_nonce == hashChangeNonce, "Project::!Nonce");
Project.sol:302:        require(
Project.sol:309:        require(_length == _scList.length, "Project::Lengths !match");
Project.sol:342:        require(_projectAddress == address(this), "Project::!Project");
Project.sol:370:                require(tasks[_taskID].getState() == 3, "Project::!Complete");
Project.sol:402:            // Check for required signatures.
Project.sol:407:        require(_project == address(this), "Project::!projectAddress");
Project.sol:435:                    // As this additional amount may not be required by the project.
Project.sol:512:        require(_project == address(this), "Project::!projectAddress");
Project.sol:516:            require(
Project.sol:522:            require(
Project.sol:531:                require(getAlerts(_task)[2], "Project::!SCConfirmed");
Project.sol:755:        require(_sc != address(0), "Project::0 address");
Project.sol:888:        require(
Project.sol:908:        require(
Community.sol:69:        require(_address != address(0), "Community::0 address");
Community.sol:75:        require(_msgSender() == homeFi.admin(), "Community::!admin");
Community.sol:81:        require(
Community.sol:90:        require(
Community.sol:131:        require(
Community.sol:159:        require(
Community.sol:191:        require(
Community.sol:235:        require(
Community.sol:241:        require(homeFi.isProjectExist(_project), "Community::Project !Exists");
Community.sol:248:        require(_community.isMember[_builder], "Community::!Member");
Community.sol:251:        require(
Community.sol:312:        require(
Community.sol:347:        require(
Community.sol:353:        require(
Community.sol:384:        require(
Community.sol:400:        require(
Community.sol:492:        require(
Community.sol:537:        require(_builder == _projectInstance.builder(), "Community::!Builder");
Community.sol:540:        require(
Community.sol:558:        require(!restrictedToAdmin, "Community::restricted");
Community.sol:569:        require(restrictedToAdmin, "Community::!restricted");
Community.sol:748:        // then this fee will be required to be paid again.
Community.sol:769:        require(_repayAmount > 0, "Community::!repay");
Community.sol:797:            require(_lentAndInterest >= _repayAmount, "Community::!Liquid");
Community.sol:893:        require(
mock/HomeFiMock.sol:42:        require(admin == _msgSender(), "HomeFi::!Admin");
mock/HomeFiMock.sol:47:        require(_address != address(0), "HomeFi::0 address");
mock/HomeFiMock.sol:96:        require(!addrSet, "HomeFi::Set");
mock/HomeFiMock.sol:113:        require(admin != _newAdmin, "HomeFi::!Change");
mock/HomeFiMock.sol:124:        require(treasury != _newTreasury, "HomeFi::!Change");
mock/HomeFiMock.sol:134:        require(lenderFee != _newLenderFee, "HomeFi::!Change");
mock/HomeFiMock.sol:175:        require(
mock/HomeFiV2Mock.sol:19:        require(!addrSet2, "Already set once");

make require statements with uint >0 make it != to save gas

Project.sol:195:        require(_cost > 0, "Project::!value>0");
Community.sol:769:        require(_repayAmount > 0, "Community::!repay");

Unchecking arithmetics operations that can’t underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: Checked or Unchecked Arithmetic. I suggest wrapping with an unchecked block: Same thing with second unchecked because total can't overflow amount cant overflow

• make i into unchecked to save gas only if i is cant get overflowed

HomeFiProxy.sol:87:        for (uint256 i = 0; i < _length; i++) {
HomeFiProxy.sol:136:        for (uint256 i = 0; i < _length; i++) {
libraries/Tasks.sol:181:        for (uint256 i = 0; i < _length; i++) _alerts[i] = _self.alerts[i];
Project.sol:249:        for (uint256 i = 0; i < _length; i++) {
Project.sol:312:        for (uint256 i = 0; i < _length; i++) {
Project.sol:323:        for (uint256 i = 0; i < _length; i++) {
Project.sol:604:            for (; i < _changeOrderedTask.length; i++) {
Community.sol:625:        for (uint256 i = 0; i < _communities[_communityID].memberCount; i++) {

make address(0) and make it into long form to save gas

HomeFiProxy.sol:41:        require(_address != address(0), "Proxy::0 address");
HomeFiProxy.sol:106:            contractAddress[_contractName] == address(0),
libraries/SignatureDecoder.sol:26:            return (address(0));
libraries/SignatureDecoder.sol:36:            return (address(0));
ProjectFactory.sol:35:        // Ensure an address is not the zero address (0x00)
ProjectFactory.sol:36:        require(_address != address(0), "PF::0 address");
Disputes.sol:38:        // Revert if _address zero address (0x00)
Disputes.sol:39:        require(_address != address(0), "Disputes::0 address");
HomeFi.sol:78:        require(_address != address(0), "HomeFi::0 address");
DebtToken.sol:50:        require(_communityContract != address(0), "DebtToken::0 address");
Project.sol:135:        require(_contractor != address(0), "Project::0 address");
Project.sol:153:        require(contractor != address(0), "Project::0 address");
Project.sol:479:            if (_newSC != address(0)) {
Project.sol:486:                tasks[_taskID].subcontractor = address(0);
Project.sol:754:        // Revert if sc to invite is address 0
Project.sol:755:        require(_sc != address(0), "Project::0 address");
Project.sol:800:        if (contractor == address(0)) {
Project.sol:844:        if (contractor == address(0)) {

make admin only function payable to save gas

because payable dosnt check msg.value == 0 HomeFi.sol:157,17185,201