Intended interaction with a contract will fail, leading to potential loss of funds

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/78779c30bebfd46e6f416b03066c55d587e8b30b/contracts/ousg/rOUSGFactory.sol#L121-L132 https://github.com/code-423n4/2024-03-ondo-finance/blob/78779c30bebfd46e6f416b03066c55d587e8b30b/contracts/ousg/ousgInstantManager.sol#L794-L811

Vulnerability details

Impact

the intended interaction with a contract will fail, leading to a potential loss of funds or a failed operation, as the value is transferred to an EOA without executing the intended logic. Here is how it works: Low-level calls return success if there is no code present at the specified address. If the guardian intends to call a contract with some data and some value but mistakenly provides an address that is not a contract (i.e., an EOA), the following will happen with the call:

Call Execution: The low-level call will be executed with the provided data and value.

Return Success: Since EOAs do not have associated code to execute, the call will not execute any code but will return true, indicating that the call itself did not revert. The value sent will be transferred to the EOA.

Data Ignored: The data sent in the call will be ignored because there is no contract code to interpret or act upon the data.

Potential Risks: This situation is not ideal because the intended contract interaction will not occur. The value will be transferred, but the expected effects of the data payload will not be realized, potentially leading to a loss of funds.

Proof of Concept

function multiexcall(
    ExCallData[] calldata exCallData
  ) external payable override onlyGuardian returns (bytes[] memory results) {
    results = new bytes[](exCallData.length);
    for (uint256 i = 0; i < exCallData.length; ++i) {
      (bool success, bytes memory ret) = address(exCallData[i].target).call{
        value: exCallData[i].value
      }(exCallData[i].data);
      require(success, "Call Failed");
      results[i] = ret;
    }
  }

https://github.com/code-423n4/2024-03-ondo-finance/blob/78779c30bebfd46e6f416b03066c55d587e8b30b/contracts/ousg/rOUSGFactory.sol#L121-L132

Tools Used

Manual Review

Recommended Mitigation Steps

To handle both scenarios in the multiexcall function—transferring value to an EOA and calling a contract with data—you can introduce a conditional check that only performs the isContract verification when data is being sent. Here's an example of how you could modify the multiexcall function to accommodate this:

function multiexcall(
    ExCallData[] calldata exCallData
) external payable onlyGuardian returns (bytes[] memory results) {
    results = new bytes[](exCallData.length);
    for (uint256 i = 0; i < exCallData.length; ++i) {
        // Check if the target is a contract only if data is provided
        if (exCallData[i].data.length > 0) {
            require(AddressUpgradeable.isContract(exCallData[i].target), "Target must be a contract when data is provided");
        }

        (bool success, bytes memory ret) = exCallData[i].target.call{value: exCallData[i].value}(exCallData[i].data);
        require(success, "Call failed");
        results[i] = ret;
    }
}

Assessed type

Other

[0xRobocop]

Admin mistake and the loss of funds of the sender do not poses an impact on the protocol itself.

[c4-pre-sort]

0xRobocop marked the issue as insufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #260

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid