Ethos Reserve contest - chaduke's results

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L359-L412

Vulnerability details

Impact

Detailed description of the impact of this finding. When a user calls _withdraw() (via withdraw() or redeem()) to withdraw assets, the user might lose funds due to the adjustment of value at L400-L402.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

We show how a user Bob might lose funds when he tries to withdraw assets from a ReaperVaultERC4626 vault.

1. Suppose Bob likes to withdraw 1,000,000 underlying assets, which corresponds to 1,000,000 shares (1/1)

2. Bob calls _withdraw(1,000,1000 Bob, Bob) via withdraw() or redeem().

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L359-L412\

3. First, the 1,000,000 shares get burned.

        _burn(_owner, _shares);

4. Suppose token.balanceOf(address(this) = 800,000. Then, we are 200,000 short. The if-part gets executed, which tries to withdraw more assets from the underlying list of strategies. Since there is a loss when such withdrawal occurs, totalLoss is used to keep track of the total loss of these withdrawals .

5. After making the efforts of withdrawing more assets from strategies, the asset check is performed again. Suppose now token.balanceOf(address(this) = 900,000, that is, we are still 100,000 short. Then, in the following, the smaller of the two values, the asset balance and the original value, is chosen as the real withdrawal amount - value is updated.

token.balanceOf(address(this)
vaultBalance = token.balanceOf(address(this));
            if (value > vaultBalance) {
                value = vaultBalance;
            }

6. There is a following slippage control, unfortunately, it only controls the allowed loss for withdrawals from the strategies, not for the discrepancy between the original value and the final real withdrawal amount. Suppose this totalLoss control check passes.

require(
                totalLoss <= ((value + totalLoss) * withdrawMaxLoss) / PERCENT_DIVISOR,
                "Withdraw loss exceeds slippage"
            );

7. Finally, only 900,000 underlying asset tokens are sent back to Bob.

 token.safeTransfer(_receiver, value);

8. Bob loses 100,000 underlying tokens in this withdrawal transaction.

Tools Used

VScode

Recommended Mitigation Steps

• Revert when the final underlying asset balance is not sufficient for withdrawing value amount of underlying assets.
• A better implementation of the maxWithdaw() function to reflect both the token.balanceOf(address(this)) and the accommodation of the underlying strategies.

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/TroveManager.sol#L1500-L1507

Vulnerability details

Impact

Detailed description of the impact of this finding. lastFeeOperationTime is not modified correctly in function _updateLastFeeOpTime(). As a result, decayBaseRateFromBorrowing() will decay the base rate more slowly than expected (worst case half slower).

Since borrowing base rate is so fundamental to the protocol, I would rate this finding as H.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

We show how decayBaseRateFromBorrowing() will decay the base rate more slowly than expected because of the wrong modification of lastFeeOperationTime in _updateLastFeeOpTime():

1. decayBaseRateFromBorrowing() calls _calcDecayedBaseRate() to calculate the decayed base rate based how many minutes elapsed since last recorded lastFeeOperationTime.

function decayBaseRateFromBorrowing() external override {
        _requireCallerIsBorrowerOperations();

        uint decayedBaseRate = _calcDecayedBaseRate();
        assert(decayedBaseRate <= DECIMAL_PRECISION);  // The baseRate can decay to 0

        baseRate = decayedBaseRate;
        emit BaseRateUpdated(decayedBaseRate);

        _updateLastFeeOpTime();
    }

   function _calcDecayedBaseRate() internal view returns (uint) {
        uint minutesPassed = _minutesPassedSinceLastFeeOp();
        uint decayFactor = LiquityMath._decPow(MINUTE_DECAY_FACTOR, minutesPassed);

        return baseRate.mul(decayFactor).div(DECIMAL_PRECISION);
    }

 function _minutesPassedSinceLastFeeOp() internal view returns (uint) {
        return (block.timestamp.sub(lastFeeOperationTime)).div(SECONDS_IN_ONE_MINUTE);
 }

2. decayBaseRateFromBorrowing() then calls _updateLastFeeOpTime() to set lastFeeOperationTime to the current time if at least 1 minute pass.

 function _updateLastFeeOpTime() internal {
        uint timePassed = block.timestamp.sub(lastFeeOperationTime);

        if (timePassed >= SECONDS_IN_ONE_MINUTE) {
            lastFeeOperationTime = block.timestamp;
            emit LastFeeOpTimeUpdated(block.timestamp);
        }
    }

3. The problem with such an update of lastFeeOperationTime is, if 1.999 minutes had passed, the base rate will only decay for 1 minute, at the same time, 1.999 minutes will be added onlastFeeOperationTime. In other words, in a worst scenario, for every 1.999 minutes, the base rate will only decay for 1 minute. Therefore, the base rate will decay more slowly then expected.

4. The borrowing base rate is very fundamental to the whole protocol. Any small deviation is accumulative. In the worse case, the decay speed will slow down by half; on average, it will be 0.75 slower.

Tools Used

VSCode

Recommended Mitigation Steps

Using the effective elapsed time that is consumed by the model so far to revise lastFeeOperationTime.

 function _updateLastFeeOpTime() internal {
        uint timePassed = block.timestamp.sub(lastFeeOperationTime);

        if (timePassed >= SECONDS_IN_ONE_MINUTE) {
-            lastFeeOperationTime = block.timestamp;
+            lastFeeOperationTime += _minutesPassedSinceLastFeeOp()*SECONDS_IN_ONE_MINUTE;
            emit LastFeeOpTimeUpdated(block.timestamp);
        }
    }

QA1. The updateDistributionPeriod() function has no range check for the input _newDistributionPeriod. As a result, it might cause an overflow for L112 of function fund():

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LQTY/CommunityIssuance.sol#L101-L117

In addition, as _newDistributionPeriod is an important configuration parameter, a timelock should be introduced help a smooth transition.

Mitigation: 1) introduce a range check for _newDistributionPeriod; 2) introduce a timelock delay;

QA2. issueOath() and fund() have a division-followed-by-multiplication rounding error issue, because the fund() performs a division first to calculate rewardPerSecond and then issueOath() uses a multiplication to calculate the amount issuance to issue. For example, there will be around $871 lost for distributing 1M over the given 14 days period using this division-followed-by-multiplication approach.

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LQTY/CommunityIssuance.sol#L84-L110

Mitigation: we multiply rewardPerSecond by WAD and then divide issuance by WAD afterwards, where WAD = 1e18:

 function issueOath() external override returns (uint issuance) {
        _requireCallerIsStabilityPool();
        if (lastIssuanceTimestamp < lastDistributionTime) {
            uint256 endTimestamp = block.timestamp > lastDistributionTime ? lastDistributionTime : block.timestamp;
            uint256 timePassed = endTimestamp.sub(lastIssuanceTimestamp);
-            issuance = timePassed.mul(rewardPerSecond);
+            issuance = timePassed.mul(rewardPerSecond)/WAD;
            totalOATHIssued = totalOATHIssued.add(issuance);
        }

        lastIssuanceTimestamp = block.timestamp;
        emit TotalOATHIssuedUpdated(totalOATHIssued);
    }

    /*
      @dev funds the contract and updates the distribution
      @param amount: amount of $OATH to send to the contract
    */
    function fund(uint amount) external onlyOwner {
        require(amount != 0, "cannot fund 0");
        OathToken.transferFrom(msg.sender, address(this), amount);

        // roll any unissued OATH into new distribution
        if (lastIssuanceTimestamp < lastDistributionTime) {
            uint timeLeft = lastDistributionTime.sub(lastIssuanceTimestamp);
            uint notIssued = timeLeft.mul(rewardPerSecond);
            amount = amount.add(notIssued);
        }


-        rewardPerSecond = amount.div(distributionPeriod);
+       rewardPerSecond = amount.div(distributionPeriod)*WAD;
        lastDistributionTime = block.timestamp.add(distributionPeriod);
        lastIssuanceTimestamp = block.timestamp;

        emit LogRewardPerSecond(rewardPerSecond);
}

QA3. The permit() function fails to check v ∈ {27, 28}, so it might be subject to signature malleability.

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LUSDToken.sol#L262-L290

Mitigation: Use Zeppelin's ECDSA: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol

QA4. The BorrowerOperations contract might not work for fees-on-transfer collateral tokens.

For example, the function _moveTokensAndCollateralFromAdjust calls safeTransferFrom()at L 497 to try to move_collchangeamount of collateral tokens to theBorrowerOperationscontract, however, the contract might receive less amount due to fees-on-transfer, as a result, the next statement that pull_collChange, the same amount, to the _activePool`` will fail due to insufficient balance of collateral tokens.

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L476-L502

Mitigation: always measure the balance before and after transfer to decide the amount that has been transferred for fees-on-transfer tokens.

QA5: _computeNominalCR() assumes the debt token has a 18 decimals. Need to make this explicit or adjust the debit tokens to 18 decimals as well.

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/Dependencies/LiquityMath.sol#L97-L106

QA6. Nothing concrete regarding lqtystaking is implemented now in TroveManager.sol, so state variable or event can be deleted regarding lqtystaking or complete the implementation if necessary.

    address public lqtyStakingAddress;

QA7. updateGovernance() and updateGuardian() use only one step to change address.

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LUSDToken.sol#L146-L158

Changing adminS in one step is risky: if the new address is a wrong input by mistake, we will lose all the privileges of the owner.

Recommendation: Use OpenZeppelin's Ownable2Step. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable2Step.sol

QA8. There is no way to revoke a permit of approval. It the approval permit was given to the wrong person or with the wrong amount, a revocation is necessary.

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LUSDToken.sol#L262-L290

Mitigation: One can revoke a permit by increasing the nonce number.

QA9. permit() will bypass signature check when owner = 0x0, allowing allowance approval from the zero address and possible other future exploits (such as transfer LUSD from zero address to the attacker's address)

For example, Bob can call permit(address(0), Bob, balanceOf(adress(0)), deadline, v, r, s) to get an allowance from zero address with the amount of the LUSD balance of the zero address.

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LUSDToken.sol#L262-L294

He will pick a value s such that it will pass the check

if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) {
            revert('LUSD: Invalid s value');
        }

Pick a future deadline so that it will pass

require(deadline >= now, 'LUSD: expired deadline');

Pick v = 27, and the value of r does not matter.

Then recoveredAddress = 0 since it will be an invalid signature.

However, the following two lines will pass and as a result, Bob gets the needed allowance from the Zero address.

      require(recoveredAddress == owner, 'LUSD: invalid signature');
        _approve(owner, spender, amount);

Mitigation: 1) add a check that recoveredAddress != 0 and 2) use Zeppelin's ECDSA.

QA10. The _deposit() might suffer from a divide-by-zero error.

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L334

This will happen when _freeFunds() = 0. Then the following line will have a divide-by-zero revert, nobody can deposit anymore

  shares = (_amount * totalSupply()) / freeFunds; // use "freeFunds" instead of "pool"

Mitigation: keep a minimum reserve so that users cannot withdraw. In this way, we never have _freeFunds() = 0 when totalSupply() > 0. Locked profit does not help since it is not part of the _freeFunds() = 0.

QA 11. fund() should call issueOath() first in the beginning. Otherwise, it might move past issuance to the future.

function fund(uint amount) external onlyOwner {
        require(amount != 0, "cannot fund 0");
+     issueOath();     
       OathToken.transferFrom(msg.sender, address(this), amount);

        // roll any unissued OATH into new distribution
        if (lastIssuanceTimestamp < lastDistributionTime) {
            uint timeLeft = lastDistributionTime.sub(lastIssuanceTimestamp);
            uint notIssued = timeLeft.mul(rewardPerSecond);
            amount = amount.add(notIssued);
        }

        rewardPerSecond = amount.div(distributionPeriod);
        lastDistributionTime = block.timestamp.add(distributionPeriod);
        lastIssuanceTimestamp = block.timestamp;

        emit LogRewardPerSecond(rewardPerSecond);
    }