Platform: Code4rena
Start Date: 01/09/2023
Pot Size: $36,500 USDC
Total HM: 4
Participants: 70
Period: 6 days
Judge: kirk-baird
Id: 281
League: ETH
Rank: 7/70
Findings: 2
Award: $1,340.19
🌟 Selected for report: 0
🚀 Solo Findings: 0
1333.1052 USDC - $1,333.11
https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/bridge/SourceBridge.sol#L61 https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/bridge/DestinationBridge.sol#L90
Loss of a token ownership if a smart contract perform a cross-chain transfer using source bridge
When a caller call burnAndCallAxelar, the token is burnt on source chain
and the payload is encoded in this way:
bytes memory payload = abi.encode(VERSION, msg.sender, amount, nonce++);
when on the dest chain triggers _execute,
(bytes32 version, address srcSender, uint256 amt, uint256 nonce) = abi .decode(payload, (bytes32, address, uint256, uint256));
and the token is minted to srcSender, which is the same address from the source chain
but the issue is, if a smart contract call burnAndCallAxelar on source chain, the owner of the smart contract may not belong to the original caller in dest chain
in that case, the token ownership is lost
for example https://rekt.news/wintermute-rekt/
the false assumption of a mutlisig smart contract address is controlled by same owner in different network has cost 20M OP lost
Manual Review
let user specify a recipient address in the source bridge when calling burnAndCallAxelar
then the recipient on dest bridge receives the minted token
Token-Transfer
#0 - c4-pre-sort
2023-09-08T03:45:36Z
raymondfam marked the issue as duplicate of #119
#1 - c4-pre-sort
2023-09-08T03:45:46Z
raymondfam marked the issue as sufficient quality report
#2 - c4-judge
2023-09-17T06:07:34Z
kirk-baird marked the issue as duplicate of #406
#3 - c4-judge
2023-09-17T06:10:14Z
kirk-baird marked the issue as satisfactory
🌟 Selected for report: adriro
Also found by: 0x6980, 0xStalin, 0xanmol, 0xmystery, 0xpanicError, Arz, Aymen0909, BenRai, Breeje, Lalanda, MohammedRizwan, Raihan, SovaSlava, Stormreckson, Udsen, ast3ros, bin2chen, castle_chain, catellatech, codegpt, dev0cloo, gkrastenov, hals, klau5, kutugu, ladboy233, matrix_0wl, nirlin, ohm, peanuts, pipidu83, sandy, wahedtalash77
7.08 USDC - $7.08
https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/bridge/DestinationBridge.sol#L90 https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/bridge/SourceBridge.sol#L91
Blocklist and SanctionList can be gamed by queueing bridge transaction
To comply with law, the USDY and rUSDY token implement blocklist and SanctionList
when a address is added to blocklist or scantionlist, the address should not be able to receive token or transfer token
However, this blocklist / sanction list can be gamed by submiting a bridge transaction on source chain
In the current implementation, when use the source bridge, first the source token is burnt
then the user pay the gas fee and a cross-chain function call is made by calling AXELAR_GATEWAY.callContract
the relevant logic is here
there are two part of fee
if a user detects that he will be blocklisted or sanctioned,
he can submit a burnAndCallAxelar by paying enough gas to executes the transaction to burn his token on source chain, but he can intentionally underpays the execution fee to GAS_RECEIVER to not let the relay transaction executes immediately
then the transaction triggered on destion bridge and the token is burnt on source chain but minted on dest chain
there is a few technical details, when executes on dest chain, the payload is extracted in this line of code
function _execute( string calldata srcChain, string calldata srcAddr, bytes calldata payload ) internal override whenNotPaused { (bytes32 version, address srcSender, uint256 amt, uint256 nonce) = abi .decode(payload, (bytes32, address, uint256, uint256));
the srcSender refers to the msg.sender in source chain
and the token will be minted to the same srcSender on dest chain
TOKEN.mint(txn.sender, txn.amount);
there are two cases, if the msg.sender in source chain is a EOA address or a smart contract address
In conclusion:
if an msg.sender is a EOA account and admin blocklist the address in source chain but not on dest chain, the Blocklist and SanctionList can be escaped if there are pending bridge transaction
if an msg.sender is a smart contract account and admin cannot block same smart contract on source chain and dest chain because the same smart contract may have different owner in dest chain, the Blocklist and SanctionList can be escaped if there are pending bridge transaction
suppose I hold a EOA address: 0x14dC79964da2C08b23698B3D3cc7Ca32193d9955
I bridge 1000 USDY from ethereum to optimism
the token is burnt from address 0x14dC79964da2C08b23698B3D3cc7Ca32193d9955 in ethereum and the same amount receved on optimism in happy flow
but my account is blocklisted in ethereum, that does not mean my account is blocklisted in optimism as long as there are pending cross-chain bridge transaction via axelar
if the admin blocklist the same EOA address across all network (including both ethereum and optimism, this is not a issue, I cannot receive the minted token on optimism)
so one may argue as long as the admin blocklist the same address across all network, blocklist works
but but but
suppose I hold a smart contract address:
I bridge 1000 USDY from ethereum to optimism
the token is burnt from smart contract address on ethreum, and the same amount received for the same smart contract address
in this case, the admin cannot really blocklist both smart contract on both ethereum and optimism
because the same smart contract on different network may have different owner and the owner of the optimism smart contract does not misbehave so there is no reason for admin to blocklist the same smart contract address in optimism
so this mean the admin can blocklist the smart contract on ethereum, but the same smart contract may be owned by different person on optimism
so admin blocklist the same address across all network does not really work
Manual Review
there are a few solution, one of the solution is not let smart contract call bridge transaction or only let whitelisted smart contract call bridge transaction
then a user cannot use smart contract to queueing pending transaction to escape blocklist and sanction
Timing
#0 - c4-pre-sort
2023-09-08T05:39:59Z
raymondfam marked the issue as primary issue
#1 - c4-pre-sort
2023-09-08T05:40:06Z
raymondfam marked the issue as sufficient quality report
#2 - c4-pre-sort
2023-09-10T07:41:16Z
raymondfam marked the issue as high quality report
#3 - c4-sponsor
2023-09-13T15:17:05Z
tom2o17 (sponsor) disputed
#4 - kirk-baird
2023-09-21T09:56:53Z
Not valid as the user will have isAllowed() return false when attempting to bridged on destination chain. The net result is the bridged transaction cannot be executed.
_beforeTokenTransfer() is triggered on the destination chain when minting. It will cause reject sanctioned users. See here
#5 - c4-judge
2023-09-21T09:57:07Z
kirk-baird marked the issue as unsatisfactory: Invalid
#6 - JeffCX
2023-09-26T02:01:25Z
Emmm, I highly respect judge's expertise, but I think I really want to elaborate:
Not valid as the user will have isAllowed() return false when attempting to bridged on destination chain. The net result is the bridged transaction cannot be executed.
As the report shown
the malicious actor does not have to frontrun the blacklist transaction,
they can send the cross-chain transaction by burning their token in source chain and intentionally underpay the gas long before they get blacklisted
so the cross-chain message is never relayed, but it is queud
then after the address is blacklisted in source chain
the user can increase the gas so the cross-chain transaction is relayed and the blacklisted user is able to receive the asset in dest chain
https://docs.axelar.dev/dev/general-message-passing/gas-services/increase-gas#increase-gas
again, as the report highlight, the sender address that is blacklisted can be a smart contract
and the owner of the address is source chain and dest chain can be different person
so adding both smart contract address in source chain and dest chain to blocklist basically means the protocol sanction the wrong target
_beforeTokenTransfer() is triggered on the destination chain when minting. It will cause reject sanctioned users.
as for the allowlist, if the target is the smart contract address
it is always return True in the allowlist because the allowlist check always return True if the target is a smart contract when the minting transaction is landed in dest chain
emm so I politely think the severity is medium because the sancation mechanism can by gamed if there are pending corss-chain transaction, andI I think only https://github.com/code-423n4/2023-09-ondo-findings/issues/396 is the duplicate because other submission does not really mention the cross-chain part
#7 - kirk-baird
2023-09-26T02:46:02Z
@JeffCX Thanks for the feedback you're correct that smart contracts are treated differently in the allow list and always pass. Therefore my initial response that _beforeTokenTransfer() handles the isAllowed() is incorrect. However, I believe we'll still have issues for isBlocked() and isSanctioned() as they do not have the smart contract logic.
In terms of validity of this issue we've established that the Allowlist can be bypassed due to the is contract check. However, I don't see how we are able to pass the isBlocked() or isSanctioned() checks here.
For example isBlocked() does not differ in it's implementation based on whether the address is a smart contract or not.
Agreed that this issue and #396 are duplicates not related to the others. The other issues are related to front-running as opposed to using smart contracts over the bridge are a different set of issue and will be downgraded to QA.
#8 - JeffCX
2023-09-26T19:55:47Z
Thanks for the response
In terms of validity of this issue we've established that the Allowlist can be bypassed due to the is contract check. However, I don't see how we are able to pass the isBlocked() or isSanctioned() checks here.
For example isBlocked() does not differ in it's implementation based on whether the address is a smart contract or not.
this is the issue, as highlight in the original submission
oh well
assume the blacklisted address in source chain is a smart contract and have pending transaction to mint token in dest chain
because the same address in different chain may have different owner or logic, protocol cannot blacklist the same address in dest chain to not let mint transaction land
how to prove?
ok let us revisit the hack
https://rekt.news/wintermute-rekt/
optimism want to send the 20M OP token to the wintermute multisig address
wintermute control the multisig address in ethereum mainnet
but no one control the same multisig address in OP network
and optimism team send the the 20M OP token to the address in optimism
the hacker, who is able to extract the genosis safe factory nonce, the deploy the address before the wintermute team to control the 20M OP token
ok assume the lost token is 20M USDY ondo token
and the hacker has pending cross-chain transaction, the token is already burned in the source chain (optimism)
does it make sense to blocklist the same address in ethereum? no,
the same address owned by hacker in optimism can be rightfully blocked
but the same multisig wallet address in ethereum is owned by wintermute (other owner)
if the protocol want to prevent the mint transaction landing, they block the same multisig wallet in ethereum, they wrongly block all in-bounding transfer / outbonding transfer of a different owner in ethereum
ok, let us use a more extreme case to domonstrate how hacker can get the money out by using cross-chain transfer
https://twitter.com/sstrenev/status/1702345227808219593
in this twitter, the tweet post highlight a low-efforts MEV bot, the bot backrun the lido ETH positive rebase by calling the uniswap V2 skim function to extract the additional stETH
skim function is a way to get the token out
https://twitter.com/sstrenev/status/1702345231922860391
To those that are not familiar with what skim() does - it is a function that anyone can call on a pair contract.
It checks the differences between the internal and actual balances of the tokens and transfers the difference to an address specified by the caller.
this is the Uniswap V2 stETH / WETH pool
https://etherscan.io/address/0x4028daac072e492d34a3afdbef0ba7e35d8b55c4#writeContract
assume the token pair address is USDY / WETH address in Uniswap V2 in mainnet
and the same smart contract address in polygon get blocklisted but has queuing cross-chain transaction from polygon to mainnet and the USDY will be mint to token pair is USDY / WETH address in Uniswap V2
the protocol cannot blacklist the the token pair is USDY / WETH address in Uniswap V2 because it will block the trading of USDY
and when the asset is minted to USDY / WETH address in Uniswap V2, the blacklisted user can use another account to call skim to get the token out
emm the comment is long, but my main point is
For example isBlocked() does not differ in it's implementation based on whether the address is a smart contract or not.
the protocol is at the high risk of blocklisting wrong address in the dest chain to block the cross-chain token minting by a blacklisted address in source chain
but if protocol does not blocklist the same smart contract address in dest chain,
Blocklist and SanctionList can be escaped by queueing bridge transaction and the mint transaction will land on dest chain, the blacklisted user may or may not get the token out in dest chain but that violate the invariant: blacklisted / blocklisted user cannot transfer the out
#9 - stalinMacias
2023-09-26T20:49:27Z
I was interested in the discussion about this report and I thought I might add something, but looks like what I was thinking to add, the good @tom2o17 has already written it in a different report, see here.
So based on what @tom2o17 mentioned, I think the key is to understand that the fact that the call to bridge tokens in the destination chain is processed by Axelar, that doesn't automatically mean that those tokens will be minted in reality, as tom mentioned in the other report, depending on the number of tokens being minted will be the number of approvers required to authorize the minting of those tokens, one could argue that the malicious user could send many bridge operation with a low amount of tokens each, but I think it's wrong to assume that the lowest tier of the approversThreshold will be only 1 approver, i.e, To mint from 0-100 USDY that is required only 1 approver, this is not a fact since the protocol can set any number of approvers, it could be 2, and this means that to mint any amount of USDY in the destination chain it will be require, at least, 1 approver from the ONDO Team....
I think that the approval mechanism correctly protects the protocol from users trying to abuse what's described in this report.
I just wanted to share my thoughts about this finding because I also thought that this could be a vulnerability but when I dug more in the code I realized that it was a false-positive and that's why I didn't submit it, but I may be missing something, please, feel free to prove me wrong about my conclusions 💯
#10 - JeffCX
2023-09-26T23:25:17Z
I think that the approval mechanism correctly protects the protocol from users trying to abuse what's described in this report.
thanks for the input, based on the comments above, looks like the approver can just not approve the message to not let the abuse described in this report work
not a issue I guess
looks like the approver needs to check whether the address sender in source chain is blocklisted before approve the message, but how does the approver do is out of scope
#11 - kirk-baird
2023-09-26T23:43:55Z
This is an interesting edge case that can be prevent by approvers off-chain or by applying the blocklist / sanction list to all chains at the same time.
Since the resolution for this issue is to improve the off-chain operations I'll adjust this issue to be QA.
#12 - c4-judge
2023-09-26T23:44:00Z
kirk-baird removed the grade
#13 - c4-judge
2023-09-26T23:44:11Z
kirk-baird changed the severity to QA (Quality Assurance)
#14 - c4-judge
2023-09-26T23:45:30Z
kirk-baird marked the issue as grade-b