Lines of code
<https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDYFactory.sol#L126&gt;

Vulnerability details

Impact

Low level calls behave differently than function calls in Solidity. Calls at the EVM level to accounts with no code are successful, this is the expected and normal behavior. It is Solidity that adds checks to prevent accidental calls to accounts with no code while compiling code for normal function calls.

This means that if the target account has no code, then the SourceBridge:multiexcall will be executed successfully and the same for rUSDYFactory:multiexcall . An accidental mistake may go unnoticed and may also cause unexpected loss of funds, as this call may include call value for transferring native currency.

The sponsor has provided us with the following explanation regarding this multiexcall function:

"its a function to allow rescuing any arbitrary operation, mainly ERC20 and ERC721 but open to interacting with other contracts too."

and the function looks like:

• <https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/SourceBridge.sol#L160&gt;

  • @param exCallData Struct consisting of

  •   1) target - contract to call
    

  •   2) data - data to call target with
    

  •   3) value - eth value to call target with
    

  */
  function multiexcall(
  ExCallData[] calldata exCallData
  ) external payable override onlyOwner returns (bytes[] memory results) {
  results = new bytes;
  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;
  }
  }

As written in the solidity documentation, the low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed.

Proof of Concept

I make this PoC to illustrate better the issue.
Follow the following steps:

1. copy and paste this test in forge-tests/SourceBridge.t.sol

   function testCallsToAccountsNoCodeSucceed() public {
   IMulticall.ExCallData[] memory calls = new IMulticall.ExCallData;
   calls[0] = IMulticall.ExCallData(
   makeAddr(“address with no code”),
   abi.encodeWithSignature(“success()”),
   1 ether
   );
   // Target has no code
   assertEq(calls[0].target.code.length, 0);
   // Call succeeds even though the target has no code and no implementation
   usdyBridge.multiexcall{value: calls[0].value }(calls);
   console.log(“Call succeeds even though the target has no code and no implementation”);
   }

2. Run the test with:

forge test --match-test testCallsToAccountsNoCodeSucceed --fork-url $MAINNET_RPC_URL --fork-block-number $FORK_FROM_BLOCK_NUMBER_MAINNET -vv

the output will be:

Running 1 test for forge-tests/bridges/SourceBridge.t.sol:Test_SourceBridge
[PASS] testCallsToAccountsNoCodeSucceed() (gas: 60492)
Logs:
  Call succeeds even though the target has no code and no implementation

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 29.83ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Foundry and Manual review.

Recommended Mitigation Steps

While executing low level calls, the SourceBridge:multiexcall and rUSDYFactory:multiexcall function should check that either the address has code or the return data is greater than zero (which indicates the presence of an implementation). The OpenZeppelin contracts library provides utilities to execute low level calls in a safe way, including the functionCall and functionCallWithValue.

Assessed type

call/delegatecall

---

The text was updated successfully, but these errors were encountered:

All reactions