Biconomy Hyphen 2.0 contest - bitbopper's results
Next-Gen Multichain Relayer Protocol.
General Information
Platform: Code4rena
Start Date: 10/03/2022
Pot Size: $75,000 USDT
Total HM: 25
Participants: 54
Period: 7 days
Judge: pauliax
Total Solo HM: 10
Id: 97
League: ETH
Findings Distribution
Researcher Performance
Rank: 34/54
Findings: 2
Award: $205.19
🌟 Selected for report: 0
🚀 Solo Findings: 0
Findings Information
🌟 Selected for report: hickuphh3
Also found by: 0v3rf10w, 0x1f8b, 0xDjango, 0xNazgul, 0xngndev, 0xwags, Cantor_Dust, CertoraInc, Dravee, IllIllI, PPrieditis, Ruhum, TerrierLover, WatchPug, XDms, benk10, berndartmueller, bitbopper, catchup, cmichel, cryptphi, csanuragjain, danb, defsec, gzeon, hagrid, hubble, jayjonah8, kenta, kyliek, minhquanym, rfa, robee, saian, samruna, throttle, ye0lde, z3s
Awards
141.4016 USDT - $141.40
Labels
External Links
Lines of code
https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L59 https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L196:L224 https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L229:L253
Vulnerability details
Impact
The nftIdsStaked variable introduces a hash collision vulnerability into the LiquidityFarming.sol contract as it is employing a mapping from address to a variable length of data field.
Source: https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L59
The easiest attack scenario allows loss of funds for victims as the attackers can stop victims from unstaking their NFT in the withdraw function.
Two more complicated attack scenarios allow loss of funds as attackers can outright withdraw nfts of victims.
Proof of Concept
This attack depends very on the address of the victim an attacker would want to attack. Therefore let me just illustrate the problem.
When using an address mapping to some fixed length data field one can rely on keccak to prevent collisions. When using an address mapping with an attacker controlled length data field an attacker has a somewhat easy path to create collisions and thereby write to or read from victims data.
The first part to understanding the attack is understanding that all the nfts of a victim are going to be in continous storage locations of the contract. Lets assume that a victims mapped to array starts at 0x1337 and the own 2 nft with the Ids: 23 and 38. The memory layout could look like this:
addr | val | description ------|-----|---- 0x1337| 2 | length of the array 0x1338| 23 | Id of the first owned nft 0x1339| 38 | Id of the second owned nft
Please note if the victim were to deposit more nfts those nftIds would be placed in the storage address 0x133a, 0x133b and so on.
Less complex attack scenario
The easier attack scenario would involve the attacker generating addresses that result in a continous storage region below address 0x1337. Suppose the attacker were to generate an address whose arrays storage of nftIdsStaked lands at 0x1330. The memory layout would look like this.
addr | val | description ------|-----|---- 0x1330| 0 | length of the array 0x1337| 2 | length of the array 0x1338| 23 | Id of the first owned nft 0x1339| 38 | Id of the second owned nft
It is easy to see, that they could grow their array and thereby corrupting the array of the victim. They could essentially write nftIds of worthless nftId into the storage of the victim, thereby overwriting their nftId and preventing them (or anybody else) from withdrawing them.
More complex attack scenario
The more complex attack scenario involves finding two addresses that will have somewhat adjacent storage regions.
Where the lower storage region encroaches on the length field of the array. The length field is attacker controlled, thereby allowing the attacker to call withdraw on arbitrary nftId.
addr | val | description ------|-----|---- 0x1337| 4 | length of lower array 0x1338| 37 | Id of the first owned (worthless) nft 0x1339| 39 | Id of the second owned (worthless) nft 0x133a| 40 | Id of the third owned (worthless) nft 0x133b| N | length of higher array (attacker controlled) AND 4th nftId of lower array 0x133a| 42 | Id of the first owned (worthless) nft
Please note that this "feels" like a traditional bruteforce attack on keccak (because it is) but it is orders of magnitude more likely to be succesful.
Attackers can essentially trade bruteforcing addresses with calling deposit a bunch of times.
As this attack scenario is somewhat less known and more similar to traditional memory corruption attacks allow me to leave a link describing the issue in more detail:
https://xlab.tencent.com/en/2018/11/09/pay-attention-to-the-ethereum-hash-collision-problem-from-the-stealing-coins-incident/
Tools Used
Manual audit
Recommended Mitigation Steps
Track nftIdsStaked like so:
// user address => nth nft => nft id mapping(address => mapping(uint256 => uint256)) public nftIdsStaked;
#0 - ankurdubey521
2022-03-30T10:08:46Z
This is a really great insight, thanks a lot for bringing these up!
#1 - pauliax
2022-05-20T11:38:14Z
After some discussion with other judges (mainly 0xleastwood), we came to a conclusion that it is not feasible in practice to exploit this, here is basically why:
- Instead of a small internal table (usually starting at size 16) to store key/value pairs, Solidity's mapping uses the entire 256-bit addressable memory space. So iterating is computationally infeasible.
- The issue outlined in the document seems like a different problem altogether. In the document, you'd need to control x in mapping[uint256(msg.sender)+x), while here you'd need to control the address.
- The probability to find an address that is remotely close to another address is super low, and even so, you would have to extend the dynamic array that is pointed to by a lot.
#2 - pauliax
2022-05-20T11:38:36Z
#3 - pauliax
2022-05-20T11:41:58Z
Alex The Entreprenerd:
I believe that the assumption of the warden is imprecise, specifically the way Solidity organizes storage.
The finding assumes that the storage for mapping => array will be in contiguous memory (keccak(slot . name) + offset)
But the Solidity devs have thought this through and instead, for Arrays, the memory will be in keccak(slot . name) . keccak(offset)
For this reason, I believe the finding to be invalid.
Please check the following and cross-investigate my statements: https://docs.soliditylang.org/en/v0.8.13/internals/layout_in_storage.html#mappings-and-dynamic-arrays
#4 - pauliax
2022-05-20T11:44:13Z
Even though this probably should be invalidated entirely, I will give the warden a little score for submitting this. Moving this to QA.
#5 - pauliax
2022-05-20T11:45:26Z
Also might be useful, from https://twitter.com/_hrkrshnn
#6 - pauliax
2022-05-21T15:19:28Z
#7 - ankurdubey521
2022-05-21T16:35:30Z
@pauliax Thanks for the detailed review of this issue!
Findings Information
🌟 Selected for report: Dravee
Also found by: 0v3rf10w, 0x1f8b, 0xDjango, 0xNazgul, 0xngndev, 0xwags, Cantor_Dust, CertoraInc, IllIllI, Jujic, Kenshin, Kiep, PPrieditis, TerrierLover, Tomio, WatchPug, antonttc, benk10, berndartmueller, bitbopper, csanuragjain, defsec, gzeon, hagrid, hickuphh3, kenta, minhquanym, oyc_109, pedroais, peritoflores, rfa, robee, saian, samruna, sirhashalot, throttle, wuwe1, z3s
Awards
63.7925 USDT - $63.79
Labels
External Links
Immutable
lpToken should be immutable
liquidityProviders should be immutable
tokenManager should be immutable
_trustedForwarder should be immutable
saving sloads
Often it makes sense to save an sload by copying a value to memory and reusing it.