Platform: Code4rena
Start Date: 22/04/2021
Pot Size: $120,000 USDC
Total HM: 41
Participants: 10
Period: 7 days
Judge: LSDan
Total Solo HM: 28
Id: 5
League: ETH
Rank: 5/10
Findings: 8
Award: $6,094.54
🌟 Selected for report: 5
🚀 Solo Findings: 4
5.9395 VETH - $308.86
0.1425 ETH - $356.37
shw
The lockUnits and unlockUnits functions in Pools.sol allow anyone to call without any restrictions or access control on the caller. An attacker can steal any user's member units by directly calling lockUnits.
Referenced code: Pool.sol#L179-L187
PoC: Link to PoC
See the file 300_lockUnits.js for a PoC of this attack. To run it, use npx hardhat test 300_lockUnits.js.
None
Add access control on both functions to allow calls only from the router, e.g., require(msg.sender == router).
#0 - strictly-scarce
2021-05-01T07:30:05Z
#1 - 0xBrian
2021-05-11T08:13:23Z
#2 - dmvt
2021-05-26T22:23:16Z
duplicate of #208
🌟 Selected for report: shw
32.5901 VETH - $1,694.68
0.7822 ETH - $1,955.40
shw
The removeLiquidity function in Pools.sol uses tx.origin to determine the person who wants to remove liquidity. However, such a design is dangerous since the pool assumes that this function is called from the router, which may not be true if the user is under a phishing attack, and he could unintendedly remove liquidity.
Referenced code: Pool.sol#L77-L79
None
Consider making the function _removeLiquidity external, which can be utilized by the router, providing information of which person removes his liquidity.
#0 - strictly-scarce
2021-05-01T07:29:20Z
If a user has been phished, consider all their funds already stolen.
Vader's security assumption is a user is not phished.
#1 - Mervyn853
2021-05-01T08:16:23Z
Our decision matrix for severity:
0: No-risk: Code style, clarity, off-chain monitoring (events etc), exclude gas-optimisations 1: Low Risk: UX, state handling, function incorrect as to spec 2: Funds-Not-At-Risk, but can impact the functioning of the protocol, or leak value with a hypothetical attack path with stated assumptions, but external requirements 3: Funds can be stolen/lost directly, or indirectly if a valid attack path shown that does not have handwavey hypotheticals.
Recommended: 0
#2 - dmvt
2021-05-26T19:32:01Z
This is reasonably easy to mitigate as an issue and failure to do so does leave an attack vector open. If exploited it will result in a loss of user funds.
🌟 Selected for report: shw
9.777 VETH - $508.40
0.2346 ETH - $586.62
shw
In Router.sol, the setup of the five anchors can be interrupted by anyone adding a new anchor due to the lack of access control of the listAnchor function. Also, duplicate anchors are allowed. If the same anchor is added three times, then this anchor biases the result of getAnchorPrice.
Referenced code: Router.sol#L245-L252
PoC: Link to PoC
See the file 200_listAnchor.js for a PoC of this attack. To run it, use npx hardhat test 200_listAnchor.js.
None
Only allow listAnchor to be called from the deployer by adding a require statement. Also, check if an anchor is added before by require(_isCurated == false).
#0 - strictly-scarce
2021-05-01T13:32:56Z
Deployer will list the anchors, seems highly unlikely they will get griefed in practice. Severity: 1
🌟 Selected for report: shw
3.259 VETH - $169.47
0.0782 ETH - $195.54
shw
Out-of-bound index access is possible in the function getAnchorPrice of Router.sol if the number of anchors equals 1 or 2. Also, the returned anchor price is not the overall median in those situations.
Referenced code: Router.sol#L288
None
Consider using arrayPrices.length/2 as the index to get the median of prices.
#0 - 0x1d00ffff
2021-05-10T07:29:04Z
duplicate of #213
🌟 Selected for report: shw
0 VETH - $0.00
0.0638 ETH - $159.60
shw
In Router.sol, the second else if statement in the function swapWithSynthsWithLimit is unnecessary.
Referenced code: Router.sol#L162
None
Consider using else {...}, which has the identical behavior to save gas.
🌟 Selected for report: shw
0 VETH - $0.00
0.0638 ETH - $159.60
shw
In Pools.sol, the addLiquidity function includes unnecessary function calls, which are isAnchor and isAsset. Since the two functions are defined as public view, and are compiled into calls at the bytecode level.
Referenced code: Pools.sol#L58 Pools.sol#L63 Pools.sol#L218-L222
None
Consider changing to _isAnchor[token] and _isAsset[token] to save gas on calling funtions.