computeTotalDebt() from the Penrose.sol uses the "&&" operator instead of the "||" operator in its require statement.

Summary

computeTotalDebt() from the Penrose.sol uses the "&&" operator instead of the "||" operator in its require statement. Therefore it fails to accept individual calls from either the registered Markets, owner or Penrose contract address itself.

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/Penrose.sol#L517-L532

Vulnerability Detail

According to the Natspec describing the computeTotalDebt() function, The require statement should only check for only one of registered market address, the owner of the contract address or the penrose contract address itself to proceed with fulfilling the purpose of the function, but instead the require statement mandates that all three parameters be met before total debts could be computed. Because of this the contract fails to deliver expected function and reverts if the msg.sender doesn't passes all three requirements.

Impact

Contract fails to deliver promised returns, but doesn't lose value

Vulnerability

Medium

Code Snippet

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/Penrose.sol#L517-L532

the below function takes the "&&" operator instead of the "|| " in its require statement

    function computeTotalDebt() public notPaused returns (uint256 totalUsdoDebt) {
        // allow other registered Markets, owner or Penrose to call it
        if (!isMarketRegistered[msg.sender] && msg.sender != owner() && msg.sender != address(this)) {
            revert NotAuthorized();
        }

        //accrue to the latest point in time
        _reAccrueMarkets(true);

        // compute debt
        totalUsdoDebt = viewTotalDebt();

        emit TotalUsdoDebt(totalUsdoDebt);
    }

Tool used

Vs code, Manual Review

POC

Register a singularity market address which is not a prior owner of the the penrose contract.
Call the computeTotalDebt() to get the total debt amount
the function reverts because the computeTotalDebt() function requires a caller to also be the owner of the contract.

Recommendation

change the "&&" operator in the computeTotalDebt() to "||" to allow access for either of the three authorized addresses.

    if (!isMarketRegistered[msg.sender] || msg.sender != owner() || msg.sender != address(this)) {
            revert NotAuthorized();
        }

cryptotechmaker commented 4 months ago

Invalid; the condition works as you think now; In case of || the revert would happen in any of the 3 cases

sherlock-admin2 commented 4 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

it made sure they are all met before reverting

nevillehuang commented 3 months ago

Invalid, check is correct

sherlock-audit / 2024-02-tapioca-judging

aycozynfada - computeTotalDebt() from the Penrose.sol uses the "&&" operator instead of the "||" operator in its require statement. #131