Incorrect Authorization in setPendingRedemptionBalance function can lead receiving more collateral

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/CashManager.sol#L851-L876 https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/CashManager.sol#L855

Vulnerability details

Impact

An attacker to exploit the setPendingRedemptionBalance function if they are able to gain the MANAGER_ADMIN role. In the provided code, the function allows the MANAGER_ADMIN to set the pending redemption balance of a user for a specific epoch. If an attacker is able to gain this role, they could potentially set the pending redemption balance of a user to an arbitrary value, allowing them to steal collateral.

Proof of Concept

The setPendingRedemptionBalance function allows any account with the MANAGER_ADMIN role to set the redemption balance of any user for any epoch. This means that an attacker who gains access to the MANAGER_ADMIN role could potentially use this function to set the redemption balance of any user to an arbitrary value, potentially allowing them to redeem more collateral than they are entitled to.

A proof of concept for this vulnerability would involve an attacker gaining access to the MANAGER_ADMIN role, and then using the setPendingRedemptionBalance function to set their own redemption balance to a large value. They could then call the completeRedemptions function to redeem more collateral than they are entitled to.

For example, let's say that Bob is an attacker who has gained access to the MANAGER_ADMIN role. He wants to redeem more collateral than he is entitled to. He can use the setPendingRedemptionBalance function to set his own redemption balance to a large value, let's say 100 ether. He can then call the completeRedemptions function to redeem this amount of collateral. However, the correct amount that Bob is entitled to redeem is only 10 ether. So, Bob will be able to steal 90 ether of collateral from the contract.

Recommended Mitigation Steps

To fix the issue of incorrect authorization in the setPendingRedemptionBalance function, the first step would be to ensure that only the intended parties are able to call the function by adding proper access controls. One way to do this would be to add a require statement that checks if the calling address has the MANAGER_ADMIN role before allowing the function to proceed.

function setPendingRedemptionBalance(
    address user,
    uint256 epoch,
    uint256 balance
  ) external updateEpoch {
    // Ensure that only the `MANAGER_ADMIN` is able to call this function
    require(hasRole(msg.sender, MANAGER_ADMIN), "Caller is not authorized as MANAGER_ADMIN");
    if (epoch > currentEpoch) {
      revert CannotServiceFutureEpoch();
    }
    uint256 previousBalance = redemptionInfoPerEpoch[epoch].addressToBurnAmt[user];
    // Increment or decrement total burned for the epoch based on whether we
    // are increasing or decreasing the balance.
    if (balance < previousBalance) {
      redemptionInfoPerEpoch[epoch].totalBurned -= previousBalance - balance;
    } else if (balance > previousBalance) {
      redemptionInfoPerEpoch[epoch].totalBurned += balance - previousBalance;
    }
    redemptionInfoPerEpoch[epoch].addressToBurnAmt[user] = balance;
    emit PendingRedemptionBalanceSet(
      user,
      epoch,
      balance,
      redemptionInfoPerEpoch[epoch].totalBurned
    );
}

The onlyRole(MANAGER_ADMIN) modifier checks that the current msg.sender is in the MANAGER_ADMIN role. If the msg.sender is not in the role, the transaction reverts. However, if an attacker is able to gain control of the MANAGER_ADMIN account, then they would be able to call functions with the onlyRole(MANAGER_ADMIN) modifier and execute malicious actions, such as stealing collateral in the case of the setPendingRedemptionBalance function. This is why in the corrected version of the code, additional security measures such as the use of a whitelist and the use of a nonce are implemented to further secure the function and prevent unauthorized calls.

[c4-judge]

trust1995 marked the issue as unsatisfactory: Out of scope

[tom2o17]

If an attacker is able to gain access to the managerAdmin role they can also mint infinite cash tokens to themselves through the multiexCall function.

Unless you are able to show how one can gain access to this role. This finding is uninteresting.

[c4-sponsor]

tom2o17 marked the issue as sponsor disputed