Platform: Code4rena
Start Date: 10/05/2024
Pot Size: $300,500 USDC
Total HM: 4
Participants: 27
Period: 17 days
Judge: Picodes
Total Solo HM: 1
Id: 375
League: ETH
Rank: 17/27
Findings: 1
Award: $0.00
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: Sathish9098
Also found by: Audinarey, Dup1337, K42, KupiaSec, LessDupes, Rhaydden, SpicyMeatball, Takarez, ZanyBonzy, bronze_pickaxe, carlitox477, dontonka, forgebyola, fyamf, guhu95, hihen, josephdara, ladboy233, slvDev, twcctop, xuwinnie, zanderbyte
0 USDC - $0.00
Links to affected code *
EdgeChallengeManager.sol is inended to be upgradeable as can be noted from the prescence of an Initializer and also the comment made in the interface. It however does not derive from UUPS contract that handles Openzepplin's UUPSUpgradeable contract. Therefore, it is missing the authoriseUpgrade method, and the contract cannot be upgraded incase it needs to be upgraded at later point.
contract EdgeChallengeManager is IEdgeChallengeManager, Initializable {
/// @dev The contract is paused, so cannot be paused error AlreadyPaused(); /// @dev The contract is unpaused, so cannot be unpaused error AlreadyUnpaused(); /// @dev The contract is paused error Paused();
error EdgeTypeNotBlock(uint8 level);
error EdgeClaimMismatch(bytes32 edgeId, bytes32 claimingEdgeId);
error EdgeNotAncestor( //@ qa unused errors bytes32 edgeId, bytes32 lowerChildId, bytes32 upperChildId, bytes32 ancestorEdgeId, bytes32 claimId );
Links to affected code *
function depositIntoPool(uint256 amount) external { if (amount == 0) { revert ZeroAmount(); } depositBalance[msg.sender] += amount; IERC20(stakeToken).safeTransferFrom(msg.sender, address(this), amount); emit StakeDeposited(msg.sender, amount); }
Links to affected code *
function withdrawFromPool(uint256 amount) public { if (amount == 0) { revert ZeroAmount(); } uint256 balance = depositBalance[msg.sender]; if (amount > balance) { revert AmountExceedsBalance(msg.sender, amount, balance); } depositBalance[msg.sender] = balance - amount; IERC20(stakeToken).safeTransfer(msg.sender, amount); emit StakeWithdrawn(msg.sender, amount); }
postUpgradeInit can be removed, since it is also performed in _setBufferConfigLinks to affected code *
function postUpgradeInit(BufferConfig memory bufferConfig_) external onlyDelegated onlyProxyOwner { if (!isDelayBufferable) revert NotDelayBufferable(); // ... _setBufferConfig(bufferConfig_); }
and in _setBufferConfig, there's the same check.
function _setBufferConfig(BufferConfig memory bufferConfig_) internal { if (!isDelayBufferable) revert NotDelayBufferable(); ... }
Links to affected code *
Users passing in 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE will have ERC20 bridges created for them instead.
BridgeContracts memory frame = _createBridge( adminProxy, nativeToken == address(0) ? ethBasedTemplates : erc20BasedTemplates, isDelayBufferable ); // init contracts if (nativeToken == address(0)) { IEthBridge(address(frame.bridge)).initialize(IOwnable(rollup)); } else { IERC20Bridge(address(frame.bridge)).initialize(IOwnable(rollup), nativeToken); }
And will sequencerInbox can be successfully initializated with the bridge depending on if they select using fee token in the constructor.
function initialize( IBridge bridge_, ISequencerInbox.MaxTimeVariation calldata maxTimeVariation_, BufferConfig memory bufferConfig_ ) external onlyDelegated { if (bridge != IBridge(address(0))) revert AlreadyInit(); if (bridge_ == IBridge(address(0))) revert HadZeroInit(); // Make sure logic contract was created by proper value for 'isUsingFeeToken'. // Bridge in ETH based chains doesn't implement nativeToken(). In future it might implement it and return address(0) bool actualIsUsingFeeToken = false; try IERC20Bridge(address(bridge_)).nativeToken() returns (address feeToken) { if (feeToken != address(0)) { actualIsUsingFeeToken = true; } } catch {} if (isUsingFeeToken != actualIsUsingFeeToken) { revert NativeTokenMismatch(); }
Links to affected code *
When bridge is created with address(0) as native token, EthBridge is automatically created.
BridgeContracts memory frame = _createBridge( adminProxy, nativeToken == address(0) ? ethBasedTemplates : erc20BasedTemplates, isDelayBufferable ); // init contracts if (nativeToken == address(0)) { IEthBridge(address(frame.bridge)).initialize(IOwnable(rollup)); } else { IERC20Bridge(address(frame.bridge)).initialize(IOwnable(rollup), nativeToken); }
Upon SequencerInbox initialization, a check is the made to see if nativeToken from ERC20Bridge is address(0), which as established will not be.
function initialize( IBridge bridge_, ISequencerInbox.MaxTimeVariation calldata maxTimeVariation_, BufferConfig memory bufferConfig_ ) external onlyDelegated { if (bridge != IBridge(address(0))) revert AlreadyInit(); if (bridge_ == IBridge(address(0))) revert HadZeroInit(); // Make sure logic contract was created by proper value for 'isUsingFeeToken'. // Bridge in ETH based chains doesn't implement nativeToken(). In future it might implement it and return address(0) bool actualIsUsingFeeToken = false; try IERC20Bridge(address(bridge_)).nativeToken() returns (address feeToken) { if (feeToken != address(0)) { actualIsUsingFeeToken = true; } } catch {} if (isUsingFeeToken != actualIsUsingFeeToken) { revert NativeTokenMismatch(); }
Links to affected code *
The function is intended to be settable only once and holds a check for just that. The check is however not effective if the children are set to 0.
function setChildren(ChallengeEdge storage edge, bytes32 lowerChildId, bytes32 upperChildId) internal { if (edge.lowerChildId != 0 || edge.upperChildId != 0) { revert ChildrenAlreadySet(ChallengeEdgeLib.id(edge), edge.lowerChildId, edge.upperChildId); } edge.lowerChildId = lowerChildId; edge.upperChildId = upperChildId; }
mostSignificantBit should wrapped in unchecked blocks as overflow wrapping behaviour is desiredLinks to affected code *
mostSignificantBit in UintUtilsLib.sol appears to be a fork of uniswapV3 bitmath contract. And it can be observed that its being compiled with a solidity version less than 0.8.0, which allows for overflow wrapping. In the 0.8 version of the contract, the unchecked blocks can be noticed indicating that the overflow wrapping behaviour is quite important in bitmath calculations. Consider doing the same in UintUtilsLib.sol
function mostSignificantBit(uint256 x) internal pure returns (uint256 msb) { require(x != 0, "Zero has no significant bits"); // x >= 2 ** 128 if (x >= 0x100000000000000000000000000000000) { x >>= 128; msb += 128; } // x >= 2 ** 64 if (x >= 0x10000000000000000) { x >>= 64; msb += 64; } // x >= 2 ** 32 if (x >= 0x100000000) { x >>= 32; msb += 32; } // x >= 2 ** 16 if (x >= 0x10000) { x >>= 16; msb += 16; } // x >= 2 ** 8 if (x >= 0x100) { x >>= 8; msb += 8; } // x >= 2 ** 4 if (x >= 0x10) { x >>= 4; msb += 4; } // x >= 2 ** 2 if (x >= 0x4) { x >>= 2; msb += 2; } // x >= 2 ** 1 if (x >= 0x2) msb += 1; } }
Links to affected code *
The contracts are to be deployed on Ethereum and Arbitrum which have different units of time measurements per block. Consider setting the value in the constructor instead.
uint256 public constant VALIDATOR_AFK_BLOCKS = 201600;
Links to affected code *
RollupCreator.sol holds a receive function which makes it able to receive ETH.
receive() external payable {}
Upon factory deployment, the cost is calculated, and for refunds, the entire contract balance is sent to the caller. Considering that the function that deploys the factories is payable, users can call the function sending ETH too. They should be refunded the difference between the amount sent and the cost of factory deployment, not the entire contract balance. A user monitoring the mempool can frontrun another user's deployment transaction if the first user had sent tokens to the contract first, before calling the createRollup function.
if (_nativeToken == address(0)) { // we need to fund 4 retryable tickets uint256 cost = l2FactoriesDeployer.getDeploymentTotalCost( IInboxBase(_inbox), _maxFeePerGas ); // do it l2FactoriesDeployer.perform{value: cost}(_inbox, _nativeToken, _maxFeePerGas); // refund the caller // solhint-disable-next-line avoid-low-level-calls (bool sent, ) = msg.sender.call{value: address(this).balance}(""); require(sent, "Refund failed"); } else {
Links to affected code *
// A little over 15 minutes minimumAssertionPeriod = 75; challengeGracePeriodBlocks = config.challengeGracePeriodBlocks;
Links to affected code *
Stake amount is current denoted as a definite amount of the stake token in use (not in terms of usd or oracle pricing). This cost of creating edges fluctuates depending the current value of the stake token. Depending on the direction of the staketoken price, creating assertions and edges can either be too expensive (even for pools), potentially allowing incorrect assertions to slip through, or can be extremely cheap, allowing excessive edge and assertion creation leading to spam.
Consider using a stake token price instead, or introducing a function to change the stake amount.
getPool might be vulnerable to address collisionsLinks to affected code *
https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/assertionStakingPool/AssertionStakingPoolCreator.sol#L30 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/assertionStakingPool/EdgeStakingPoolCreator.sol#L30 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/assertionStakingPool/StakingPoolCreatorUtils.sol#L15
The CREATE2's computeAddress function employs deterministic address computation to verify the authenticity of the deployed assertion and edge staking pools. This approach is based on the assumption that the address derived from the contract's creation bytecode and the creator's address is unique. However, this method is susceptible to address collision risks, where a different contract, under specific circumstances, might share the same computed address, leading to erroneous validation and possible draining of the pool's funds.
bytes32 bytecodeHash = keccak256(abi.encodePacked(creationCode, args)); address pool = Create2.computeAddress(0, bytecodeHash, address(this));
#0 - c4-judge
2024-06-05T11:43:57Z
Picodes marked the issue as grade-b
#1 - Picodes
2024-06-05T11:44:01Z
1 - my understanding is that transparent proxies are used