search

code-423n4 / 2024-04-renzo-validation

2 stars 2 forks source link

HARDCODE EXTRAARGS may revert due to gas limit #364

Closed c4-bot-8 closed 6 months ago

c4-bot-8 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/L1/xRenzoBridge.sol#L225

Vulnerability details

Chainlink CCIP to transfer tokens and arbitrary data between smart contracts on different blockchains

To construct a CCIP-compatible message the EVM2AnyMessage struct is used. The arguments of struct include an extraArgs that is hardcoded in the contract.

According to the documentation, for production deployments, make sure that extraArgs is mutable. This allows you to build it offchain and pass it in a call to a function or store it in a variable that you can update on-demand. This makes extraArgs compatible with future CCIP upgrades.

Impact

Hardcoded gas fee may be insufficient due to future CCIP upgrades, causing the contract to revert on gasLimits.

Tools Used

Manual Review

Recommended Mitigation Steps

Allow admin to set gas price via a mutator method.

Assessed type

Context

DadeKuma commented 6 months ago

It's not an issue for now, and in case the logic is changed in the future the contract can be upgraded

DadeKuma commented 6 months ago

@howlbot reject

goheesheng commented 5 months ago

It wasn't a known and acceptable risk stated by the development team. You are right that the contract is upgradable, but there isn't a need to without much logic implementation. It would be a waste of fees to upgrade again where you can include a mutator method to fix the problem clearly stated by Chainlink to NOT hardcode gas. It's pretty unfair that we know this is problem and not documented in README invalidates the report. This at least have to be a borderline high severity.

alcueca commented 5 months ago

This is a vulnerability depending on a future upgrade by an integrator. In general, the upgradability of the contracts in scope is not to be taken into account, but in this case I think it is fair to accept it since for the vulnerability to materialize an upgrade is also needed in the integrator.

As such, this would be valid QA. In no case this would ever be a High. As such, it is invalidated due to overinflated severity.

goheesheng commented 5 months ago

This is a vulnerability depending on a future upgrade by an integrator. In general, the upgradability of the contracts in scope is not to be taken into account, but in this case I think it is fair to accept it since for the vulnerability to materialize an upgrade is also needed in the integrator.

As such, this would be valid QA. In no case this would ever be a High. As such, it is invalidated due to overinflated severity.

Thanks @alcueca, I think that this should at most be Medium because as you stated, in order to fix the issue an upgrade is require. Also, it will cause a denial of service, which according to C4 Severity:

Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.