Platform: Code4rena
Start Date: 03/07/2023
Pot Size: $40,000 USDC
Total HM: 14
Participants: 74
Period: 7 days
Judge: alcueca
Total Solo HM: 9
Id: 259
League: ETH
Rank: 2/74
Findings: 4
Award: $4,694.79
🌟 Selected for report: 2
🚀 Solo Findings: 2
372.5257 USDC - $372.53
Judge has assessed an item in Issue #180 as 2 risk. The relevant finding follows:
[L-04] Some slot data is incorrectly overwritten during storage
#0 - c4-judge
2023-08-05T21:36:33Z
alcueca marked the issue as duplicate of #259
#1 - c4-judge
2023-08-05T21:36:38Z
alcueca marked the issue as partial-50
#2 - c4-judge
2023-08-05T21:36:58Z
alcueca marked the issue as satisfactory
#3 - c4-judge
2023-08-15T19:40:58Z
alcueca marked the issue as not a duplicate
#4 - c4-judge
2023-08-15T19:41:08Z
alcueca marked the issue as duplicate of #260
#5 - c4-judge
2023-08-15T19:41:36Z
alcueca marked the issue as partial-50
🌟 Selected for report: kutugu
2152.3705 USDC - $2,152.37
Compared with div, there is a larger precision error in calculating lp through sqrt, so there should be a way to check whether there are excess tokens left when adding liquidity.
function testCalcLpTokenSupplyDiff() public { uint256[] memory reserves = new uint256[](2); reserves[0] = 1e24 + 1e4; reserves[1] = 10000; uint256 lp1 = this.calcLpTokenSupply(reserves, bytes("")); reserves[0] = 1e24; reserves[1] = 10000; uint256 lp2 = this.calcLpTokenSupply(reserves, bytes("")); assert(lp1 == lp2); }
When reserve[0] is larger relative to reserve[1], the accuracy error is larger, and unlike div, the accuracy error is only 1, the accuracy error of sqrt is larger. When the user input the imbalance the amount will left excess reserve, searchers will monitor the contract after the excess reserve accumulation to a certain degree, will withdraw them by removeLiquidityImbalanced.
Manual review
Excess reserve tokens should be returned when the user adds liquidity
Math
#0 - c4-pre-sort
2023-07-11T15:18:07Z
141345 marked the issue as primary issue
#1 - 141345
2023-07-13T06:08:37Z
precision error in sqrt lead to excess token
#2 - 141345
2023-07-13T07:48:22Z
previous audit by cyfrin talks about similar thing in High finding 1 and 3.
Not sure if this is known issue
#3 - publiuss
2023-07-18T10:53:31Z
This is a known issue. The documentation should be updated.
#4 - c4-sponsor
2023-07-18T10:53:35Z
publiuss marked the issue as sponsor acknowledged
#5 - c4-judge
2023-08-04T12:38:07Z
alcueca marked the issue as satisfactory
#6 - c4-judge
2023-08-19T18:36:14Z
alcueca marked the issue as selected for report
#7 - publiuss
2023-08-23T00:56:24Z
This was appended to the documentation here: https://github.com/BeanstalkFarms/Basin/blob/91233a22005986aa7c9f3b0c67393842cd8a8e4d/src/functions/ConstantProduct2.sol#L49
🌟 Selected for report: kutugu
2152.3705 USDC - $2,152.37
https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/Well.sol#L610 https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/Well.sol#L304 https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/Well.sol#L370
The well does not check the actual transferout amount and the k value, which for exclusive feeOnTransfer tokens causes the well to have a portion of the token fee cut out of air every time the token is transferred, which is not included in the transfer amount. Attackers can take advantage of this to run out of well.
The most common attack flow is through the skim function:
Manual review
Check the correct amount every time you transfer out
Uniswap
#0 - c4-pre-sort
2023-07-11T16:45:54Z
141345 marked the issue as low quality report
#1 - 141345
2023-07-13T07:20:48Z
seems invalid
#2 - alcueca
2023-08-04T05:26:30Z
I think this is correct. Most fee-on-transfer tokens will take the fee on the recipient, but there isn't a rule or even a standard that forbids the fee-on-transfer token to take the fees from the sender, or from both. My bank has all of these options for transfers that include fees.
The Well in scope is intended to support fee-on-transfer tokens, but nowhere it is specified that it is intended to support only recipient-fee-on-transfer tokens, and therefore is vulnerable. This could be a critical except that recipient-fee-on-transfer tokens are exceedingly rare at the time, and only Wells deployed for those exceedingly rare tokens would be drained.
#3 - c4-judge
2023-08-04T05:26:36Z
alcueca marked the issue as satisfactory
#4 - c4-sponsor
2023-09-28T12:08:32Z
publiuss (sponsor) disputed
#5 - publiuss
2023-09-28T12:11:00Z
skim will only transfer tokens out of the Well if the balances of tokens in the well are greater than the reserves of the Well. This only happens if someone sends tokens to the Well without calling sync or shift.
There is no way to abuse this mechanism as the Well's token balances can never drop below reserve balances as a result of skim.
#6 - alcueca
2023-09-29T05:39:30Z
@publiuss, I can reduce the severity to QA on account of the low quality and invalid Proof of Concept, but I think that the attack vector is valid.
Correct me if I'm wrong, but swapFromFeeOnTransfer only considers fees in the incoming token. For the outgoing token it assumes that fees will be deducted from the amount received, and calls _swapFrom as for non-fee tokens.
Normally I wouldn't worry too much about fee-on-transfer tokens, but you explicitly intend to support them, and there is no standard defining how fees should be collected, so they could be deducted from the sender's balance after each successful transfer.
During a swapFromFeeOnTransfer, inside the _swapFrom call, we will calculate what the reserves should be, and from there the amountOut. Then the Well transfers out the amountOut and sets the reserves to the calculated amounts.
Only that if a fee is collected from the sender after the transfer, the stored reserves will be higher than the actual well balances, and this discrepancy will grow with each swap.
Would you please check again if my reasoning is right?
I don't know if I would fix this in the code, I would most likely just state in the docs that only fee-on-transfer tokens where the fee is collected from the receiver are supported, and those where the fee is collected from the sender are not, but that is up to you.
#7 - publiuss
2023-10-03T21:19:42Z
@publiuss, I can reduce the severity to QA on account of the low quality and invalid Proof of Concept, but I think that the attack vector is valid.
Correct me if I'm wrong, but swapFromFeeOnTransfer only considers fees in the incoming token. For the outgoing token it assumes that fees will be deducted from the amount received, and calls
_swapFromas for non-fee tokens.Normally I wouldn't worry too much about fee-on-transfer tokens, but you explicitly intend to support them, and there is no standard defining how fees should be collected, so they could be deducted from the sender's balance after each successful transfer.
During a
swapFromFeeOnTransfer, inside the_swapFromcall, we will calculate what the reserves should be, and from there the amountOut. Then the Well transfers out theamountOutand sets the reserves to the calculated amounts.Only that if a fee is collected from the sender after the transfer, the stored reserves will be higher than the actual well balances, and this discrepancy will grow with each swap.
Would you please check again if my reasoning is right?
I don't know if I would fix this in the code, I would most likely just state in the docs that only fee-on-transfer tokens where the fee is collected from the receiver are supported, and those where the fee is collected from the sender are not, but that is up to you.
Hello @alcueca, your reasoning is correct and I agree with your thoughts in regards to the solution.
🌟 Selected for report: 0xprinc
Also found by: 0x11singh99, 0xAnah, 0xWaitress, 0xkazim, 2997ms, 33audits, 404Notfound, 8olidity, CRIMSON-RAT-REACH, CyberPunks, DanielWang888, Deekshith99, Eeyore, Eurovickk, Inspecktor, JGcarv, John, Jorgect, Kaysoft, LosPollosHermanos, MohammedRizwan, Qeew, QiuhaoLi, Rolezn, TheSavageTeddy, Topmark, Trust, Udsen, a3yip6, alexzoid, bigtone, codegpt, erebus, fatherOfBlocks, ginlee, glcanvas, hunter_w3b, josephdara, kaveyjoe, kutugu, mahdirostami, max10afternoon, oakcobalt, peanuts, pfapostol, ptsanev, qpzm, radev_sw, ravikiranweb3, sces60107, seth_lawson, te_aut, twcctop, zhaojie, ziyou-
17.5208 USDC - $17.52
| ID | Title | Severity |
|---|---|---|
| [L-01] | LibContractInfo fail to convert the address to string | Low |
| [L-02] | LibContractInfo data decode may fail with wrong type | Low |
| [L-03] | It is possible to create the same well, resulting in less liquidity | Low |
| [L-04] | Some slot data is incorrectly overwritten during storage | Low |
String is represented by ascii. Digits cannot be directly converted to string; otherwise, garbled characters may occur.
function testSymbol() external returns (string memory symbol) { symbol = new string(4); address addr = address(0x480b9a9E9Aa1c9db991C7721a92d351Db4FaC990); assembly { mstore(add(symbol, 0x20), shl(224, shr(128, addr))) } }
[PASS] testSymbol():(string) (gas: 693) Traces: [693] InvariantTest::testSymbol() └─ ← H ��
Use openzeppelin Strings library
LibContractInfo makes compatible with unsupported interfaces, but not perfect. When function selectors conflict or do not support types, the returned data may not be the correct type. In this case, calling abi.decode will revert.
function testWrongTypeRevert() external returns (string memory symbol) { symbol = abi.decode(abi.encode(address(1)), (string)); }
Use the try catch package abi.decode
When creating a well, there is no way to restrict the creation of the same well.
Even create2 will create different well if the tokens of immutableData entered by the user are not in the same order or salt is different.
These same Wells dilute liquidity, and when users swap in the well, they will suffer greater slippage, which will bring capital losses to users.
Consider adding a way to limit the creation of duplicate Wells and aggregate liquidity
// LibBytes16.sol#L45 assembly { sstore( add(slot, mul(maxI, 32)), add(mload(add(reserves, add(iByte, 32))), shr(128, sload(add(slot, maxI)))) ) } // LibLastReserveBytes.sol#L58 assembly { sstore( add(slot, mul(maxI, 32)), add(mload(add(reserves, add(iByte, 32))), shr(128, shl(128, sload(add(slot, maxI))))) ) }
There slot should be overwritten according to the before data, so the last line should read sload(add(slot, mul(maxI, 32))) instead of sload(add(slot, maxI)).
Use correct slot data to overwrite
#0 - 141345
2023-07-13T14:36:53Z
#1 - c4-pre-sort
2023-07-13T14:38:12Z
141345 marked the issue as high quality report
#2 - c4-sponsor
2023-08-02T23:05:22Z
publiuss marked the issue as sponsor acknowledged
#3 - c4-judge
2023-08-04T21:39:34Z
alcueca marked the issue as grade-a