Platform: Code4rena
Start Date: 08/11/2022
Pot Size: $60,500 USDC
Total HM: 6
Participants: 72
Period: 5 days
Judge: Picodes
Total Solo HM: 2
Id: 178
League: ETH
Rank: 32/72
Findings: 2
Award: $117.17
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: RaymondFam
Also found by: 0x1f8b, 0x52, 0xSmartContract, 0xc0ffEE, 0xhacksmithh, 8olidity, Awesome, BClabs, Bnke0x0, Chom, Deivitto, Hashlock, IllIllI, Josiah, KingNFT, Nyx, R2, ReyAdmirado, Rolezn, SamGMK, Sathish9098, SinceJuly, V_B, Vadis, Waze, a12jmx, adriro, ajtra, aphak5010, bearonbike, bin, brgltd, carlitox477, carrotsmuggler, cccz, ch0bu, chaduke, datapunk, delfin454000, erictee, fatherOfBlocks, fs0c, horsefacts, jayphbee, ktg, ladboy233, pashov, perseverancesuccess, rbserver, ret2basic, tnevler, zaskoh
36.3434 USDC - $36.34
QA1 round up fee calculation the fee is calculated as such:
uint256 orderFee = (orders[i].price * feeBp) / 10000;
when the last 4 digits of orders[i].price * feeBp is greater than 5000, say 9999, dividing by 10000 rounds it down to 0. Instead, the calculation can be changed to
uint256 orderFee = (orders[i].price * feeBp + 5000) / 10000;
This way fees are rounded more properly.
QA2. remove magic number
in setFee, instead of using 10000, use a constant ONE_PERCENTAGE, which is defined as
uint256 constant ONE_PERCENTAGE = 10000
QA3. ERC20/721/1155 griefing attack Attack could send malicious ERC20/721/1155 to the aggregator. When the owner tries to rescue them, these tokens can do something unexpected, such as use up all gas and revert, simply to annoy the owner.
QA4. add timelock to setFee(), to mitigate sudden fee changes to the user
#0 - c4-judge
2022-11-21T19:51:03Z
Picodes marked the issue as grade-b
#1 - 0xhiroshi
2022-11-23T00:00:05Z
QA1: Invalid, it is highly unlikely for us to see NFTs traded on the aggregator worth less than 0.001 ETH (1e15) so realistically we won't lose precision. QA2: Valid QA3: We just won't bother with rescuing fake tokens. QA4: We already have a fee slippage.
#2 - c4-sponsor
2022-11-23T00:00:12Z
0xhiroshi marked the issue as sponsor disputed
#3 - c4-sponsor
2022-11-23T00:00:18Z
0xhiroshi requested judge review
#4 - 0xhiroshi
2022-12-12T23:20:26Z
@Picodes what is the edict of this report?
80.8321 USDC - $80.83
Running 6 tests for test/foundry/LooksRareProxyBenchmark.t.sol:LooksRareProxyBenchmarkTest
original: [PASS] testExecuteDirectlySingleOrder() (gas: 257225) [PASS] testExecuteThroughAggregatorSingleOrder() (gas: 4562334) [PASS] testExecuteThroughAggregatorTwoOrdersAtomic() (gas: 4761371) [PASS] testExecuteThroughAggregatorTwoOrdersNonAtomic() (gas: 4761376) [PASS] testExecuteThroughV0AggregatorSingleOrder() (gas: 3450078) [PASS] testExecuteThroughV0AggregatorTwoOrders() (gas: 3644601)
G1:
BasicOrder memory order = orders[i]; can be changed to BasicOrder calldata order = orders[i];
https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/proxies/LooksRareProxy.sol#L65
[PASS] testExecuteDirectlySingleOrder() (gas: 257225) [PASS] testExecuteThroughAggregatorSingleOrder() (gas: 4521877) [PASS] testExecuteThroughAggregatorTwoOrdersAtomic() (gas: 4719113) [PASS] testExecuteThroughAggregatorTwoOrdersNonAtomic() (gas: 4719118) [PASS] testExecuteThroughV0AggregatorSingleOrder() (gas: 3409597) [PASS] testExecuteThroughV0AggregatorTwoOrders() (gas: 3602319)
G2. change function _splitSignature(bytes memory signature) to function _splitSignature(bytes calldata signature)
as well as:
if (signature.length == 64) {
bytes32 vs;
assembly {
// r := mload(add(signature, 0x20))
// vs := mload(add(signature, 0x40))
r := calldataload(add(signature.offset, 0x00))
vs := calldataload(add(signature.offset, 0x20))
s := and(vs, 0x7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff)
v := add(shr(255, vs), 27)
}
} else if (signature.length == 65) {
assembly {
// r := mload(add(signature, 0x20))
// s := mload(add(signature, 0x40))
// v := byte(0, mload(add(signature, 0x60)))
r := calldataload(add(signature.offset, 0x00))
s := calldataload(add(signature.offset, 0x20))
v := byte(0, calldataload(add(signature.offset, 0x40)))
}
[PASS] testExecuteDirectlySingleOrder() (gas: 257225) [PASS] testExecuteThroughAggregatorSingleOrder() (gas: 4510093) [PASS] testExecuteThroughAggregatorTwoOrdersAtomic() (gas: 4707165) [PASS] testExecuteThroughAggregatorTwoOrdersNonAtomic() (gas: 4707170) [PASS] testExecuteThroughV0AggregatorSingleOrder() (gas: 3397805) [PASS] testExecuteThroughV0AggregatorTwoOrders() (gas: 3590363)
#0 - c4-judge
2022-11-21T18:48:03Z
Picodes marked the issue as grade-b
#1 - c4-sponsor
2022-11-24T12:36:25Z
0xhiroshi marked the issue as sponsor confirmed