-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
onlyCentrifugeChainOrigin() can't require msg.sender equal axelarGateway #537
Comments
raymondfam marked the issue as sufficient quality report |
raymondfam marked the issue as primary issue |
raymondfam marked the issue as high quality report |
hieronx (sponsor) confirmed |
This seems High risk to me since the Axelar bridge is a centerpiece of this protocol, and when deployed in a certain way where the AxelarRouter is the only ward, it might causes user deposits to be stuck forever. |
gzeon-c4 changed the severity to 3 (High Risk) |
gzeon-c4 marked the issue as satisfactory |
Reconsidering severity to Med here since the expected setup would have DelayedAdmin able to unstuck the system |
gzeon-c4 changed the severity to 2 (Med Risk) |
gzeon-c4 marked the issue as selected for report |
Hi @gzeon-c4 i think this issue should be high. Because if this issue will not fix than protocol will suffer %100 percent after launch and without deploy new contract this issue will not fix. Let assume this contract deploy without fix this issue: |
The protocol would not be able to whitelist user by calling updateMember if this is deployed, so the issue is likely to be discovered very early and can be redeployed without chance of user fund getting stuck. |
@gzeon-c4 Thanks for answer. yes this issue would discovered before users use this protocol. But my main point while giving 'what if' scenario is show that how big impact of the this vulnerability .Because this vulnerability makes whole system unusable. Also this vulnerability happens not rely to possibility. %100 system will suffer from this.Because of that i think this issue should high instead of medium. |
https://docs.code4rena.com/awarding/judging-criteria/severity-categorization
Since asset is not at risk and the mistake can be easily discovered early with a easy fix to reconfigure the gateway using the delayed admin, I think Med is appropriate as it only impact the availability of the protocol temporarily. |
hmm i get it. Thanks for clarification. |
Mitigated in centrifuge/liquidity-pools#168 |
Lines of code
https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/gateway/routers/axelar/Router.sol#L44
Vulnerability details
Vulnerability details
In
AxelarRouter.sol
, we need to ensure the legitimacy of theexecute()
method execution, mainly through two methodsaxelarGateway.validateContractCall ()
to validate if thecommand
is approved or notonlyCentrifugeChainOrigin()
is used to validate thatsourceChain
sourceAddress
is legal.Let's look at the implementation of
onlyCentrifugeChainOrigin()
The problem is that this restriction
msg.sender == address(axelarGateway)
When we look at the official
axelarGateway.sol
contract, it doesn't provide any call external contract 'sexecute()
methodso
msg.sender
cannot beaxelarGateway
, and the official example does not restrictmsg.sender
the security of the command can be guaranteed by
axelarGateway.validateContractCall()
,sourceChain
,sourceAddress
.there is no need to restrict
msg.sender
axelarGateway
code addresshttps://github.com/axelarnetwork/axelar-cgp-solidity/blob/main/contracts/AxelarGateway.sol
can't find anything that calls
router.execute()
Impact
router.execute()
cannot be executed properly, resulting in commands from other chains not being executed, protocol not working properlyRecommended Mitigation
remove
msg.sender
restrictionmodifier onlyCentrifugeChainOrigin(string calldata sourceChain, string calldata sourceAddress) { - require(msg.sender == address(axelarGateway), "AxelarRouter/invalid-origin"); require( keccak256(bytes(axelarCentrifugeChainId)) == keccak256(bytes(sourceChain)), "AxelarRouter/invalid-source-chain" ); require( keccak256(bytes(axelarCentrifugeChainAddress)) == keccak256(bytes(sourceAddress)), "AxelarRouter/invalid-source-address" ); _; }
Assessed type
Context
The text was updated successfully, but these errors were encountered: