Platform: Code4rena
Start Date: 29/06/2022
Pot Size: $50,000 USDC
Total HM: 20
Participants: 133
Period: 5 days
Judge: hickuphh3
Total Solo HM: 1
Id: 142
League: ETH
Rank: 32/133
Findings: 4
Award: $365.59
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: xiaoming90
Also found by: 0x1f8b, 0x29A, 0x52, 0xDjango, 0xNazgul, 0xNineDec, 0xSolus, 0xf15ers, 0xsanson, AmitN, Bnke0x0, BowTiedWardens, Chom, David_, ElKu, Funen, GalloDaSballo, GimelSec, Hawkeye, IllIllI, JC, JohnSmith, Kaiziron, Kenshin, Lambda, Limbooo, MadWookie, Metatron, MiloTruck, Nethermind, Picodes, ReyAdmirado, Sneakyninja0129, StErMi, TomJ, Treasure-Seeker, TrungOre, Waze, Yiko, _Adam, __141345__, antonttc, async, aysha, catchup, cccz, cryptphi, csanuragjain, danb, datapunk, defsec, delfin454000, dirk_y, doddle0x, durianSausage, exd0tpy, fatherOfBlocks, gogo, hake, hansfriese, horsefacts, hubble, itsmeSTYJ, joestakey, oyc_109, pedroais, peritoflores, rajatbeladiya, reassor, robee, rokinot, samruna, saneryee, sashik_eth, shenwilly, shung, simon135, sseefried, unforgiven, zer0dot, zzzitron
66.349 USDC - $66.35
Code is well written, fully documented and coverage is high
The codebase could benefit with a few small refactoring to further simplify and to save extra gas
treasury for receiving fundsMost of the times a DAO ends up separating the owner, the Multisig with execution power, from the treasury the multisig to handle fees and funds.
The contract is currently using owner to handle settings as well as receive funds, it may be preferable to separate that by adding treasury
if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {
Can be refactored with
// transfer strike to owner if put is expired or call is exercised if (order.isCall == isExercised) {
As in both case they were both true or both false
Because of the gas save above we can refactor the entire block to an if / else
if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) { // etc... return; } // transfer assets from putty to owner if put is exercised or call is expired if ((order.isCall && !isExercised) || (!order.isCall && isExercised)) { // etc... return; }
Can be refactored to
if (order.isCall == isExercised)) { // transfer assets from putty to owner if put is exercised or call is expired // send the fee to the admin/DAO if fee is greater than 0% uint256 feeAmount = 0; if (fee > 0) { feeAmount = (order.strike * fee) / 1000; ERC20(order.baseAsset).safeTransfer(owner(), feeAmount); } ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount); } else { // transfer assets from putty to owner if put is exercised or call is expired _transferERC20sOut(order.erc20Assets); _transferERC721sOut(order.erc721Assets); // for call options the floor token ids are saved in the long position in fillOrder(), // and for put options the floor tokens ids are saved in the short position in exercise() uint256 floorPositionId = order.isCall ? longPositionId : uint256(orderHash); _transferFloorsOut(order.floorTokens, positionFloorAssetTokenIds[floorPositionId]); }
TL;DR: Fee could change mid order, it's also more gas efficient to store them in the order struct
Fee for orders are calculated at time of withdraw, however the order taker may have accepted the order at a time in which the fee was different, this can create a difference in their PnL.
Because order validation is done on creation and only the hash is stored onChain, it may be desirable to add a order.fee to the order struct, and then use that fee instead of the one from storage.
This will also save 2.1k in by avoiding one SLOAD during withdraw
Can move require(order.isLong, "Can only exercise long positions"); to the first line to make it fail faster
This would be consistent with how withdraw is written
Quoting the standard for a Struct
Definition of encodeType The type of a struct is encoded as name ‖ "(" ‖ member₁ ‖ "," ‖ member₂ ‖ "," ‖ … ‖ memberₙ ")" where each member is written as type ‖ " " ‖ name. For example, the above Mail struct is encoded as Mail(address from,address to,string contents).
It follows that the definition provided:
bytes32 public constant ORDER_TYPE_HASH = keccak256( abi.encodePacked( "Order(", "address maker,", "bool isCall,", // etc... "ERC721Asset[] erc721Assets", ")",
Is incorrect (if slightly), as the EIP-712 compliant implementation would have the parenthesis on a separate line. While the closing parenthesis would be with the last entry
abi.encodePacked( "Order", "(", // etc... "ERC721Asset[] erc721Assets)"
I've tested on 0.8.13 and because of encodePacked there will be no difference in the produced type
Am adding this because from my experience other wardens will submit such findings, and I don't want to lose points, although I don't believe there's any real risk beside the need to inform end-users that any token can be used with the Smart Contract.
Because of the open ended nature, any malicious token (revert on transfer, wrong accounting, deceiving name), could be used, and it's up to the caller (maker / taker) to verify all the addresses.
I think the finding should be at most Non-Critical (Informational)
These values would be easier to maintain, understand and parse through if they were constants, ideally in ALL_CAPS
Most of these functions are not called by the contract, yet they have public visibility, there is no additional cost to this, however you'd always want to mark functions meant to be called from outside as external
#0 - outdoteth
2022-07-07T18:46:29Z
Lock-in Fee with Order Creation
Duplicate: Admin can change fee at any time for existing orders: https://github.com/code-423n4/2022-06-putty-findings/issues/422
Malicious Token Risk
Duplicate: Setting malicious or invalid erc721Assets, erc20Assets or floorTokens prevents the option from being exercised: https://github.com/code-423n4/2022-06-putty-findings/issues/50
#1 - outdoteth
2022-07-07T18:46:34Z
high quality report
#2 - outdoteth
2022-07-07T18:47:53Z
Question for "Slightly incorrect EIP-712 Definition".
Is incorrect (if slightly), as the EIP-712 compliant implementation would have the parenthesis on a separate line. While the closing parenthesis would be with the last entry
What do you mean by this? From the quoted spec it looks like the closing parentheses should be on the same line? (which is how it is currently). So I don't see what the issue is here.
🌟 Selected for report: GalloDaSballo
Also found by: 0v3rf10w, 0x1f8b, 0xA5DF, 0xDjango, 0xHarry, 0xKitsune, 0xNazgul, 0xNineDec, 0xc0ffEE, 0xf15ers, 0xkatana, 0xsanson, ACai, Aymen0909, Bnke0x0, BowTiedWardens, Chom, ElKu, Fitraldys, Funen, Haruxe, Hawkeye, IllIllI, JC, JohnSmith, Kaiziron, Kenshin, Lambda, Limbooo, MadWookie, Metatron, MiloTruck, Picodes, PwnedNoMore, Randyyy, RedOneN, ReyAdmirado, Ruhum, Sm4rty, StErMi, StyxRave, TerrierLover, TomJ, Tomio, UnusualTurtle, Waze, Yiko, _Adam, __141345__, ajtra, ak1, apostle0x01, asutorufos, c3phas, cRat1st0s, catchup, codetilda, cryptphi, datapunk, defsec, delfin454000, durianSausage, exd0tpy, fatherOfBlocks, gogo, grrwahrr, hake, hansfriese, horsefacts, ignacio, jayfromthe13th, joestakey, ladboy233, m_Rassska, mektigboy, minhquanym, mrpathfindr, natzuu, oyc_109, rajatbeladiya, reassor, rfa, robee, rokinot, sach1r0, saian, sashik_eth, simon135, slywaters, swit, z3s, zeesaw, zer0dot
207.2246 USDC - $207.22
The codebase is already pretty well thought out, by extending the idea of using ERC721 IDs as identifiers, we can save massive amount of gas for day to day operations, we can also apply a few basic logic transformations as well as basic gas saving tips to reduce the total gas for the average operation by over 20k gas
The first few refactoring will offer strong gas savings with minimal work (20k+ gas), the remaining findings are minor and I expect most other wardens to also be suggesting them
exercise by removing exercisedPositions - Around 20k gas on every exercise (17k to 23k from foundry tests)It seems like exercisedPositions is used exclusively for withdraw, however through the cleverly designed "send to 0xdead" system, we can replace the exercisedPositions function with the following
function exercisedPositions(uint256 id) external view returns(bool) { return ownerOf(id) == address(0xdead); }
In fact, a position was exercised if it was transfered to 0xdead, as such there's no need to store a mapping in storage.
We can delete every mention of exercisedPositions as well as the storage mapping
This single change will reduce almost 20k gas from exercise
To make withdraw work we can write the following
// Inlined is even cheaper (8 gas for the JUMP) bool isExercised = ownerOf(uint256(longPositionId)) == address(0xdead);
And we can delete the exercisedPositions mapping from the contract
You can use the function above so the test still passes, in case you need a way to verify if a position was exercised, otherwise you can just use the isExercised line and delete very other mention of exercisedPositions
BEFORE
exercise ┆ 5759 ┆ 55920 ┆ 68002 ┆ 133534 ┆ 18
withdraw ┆ 3075 ┆ 27840 ┆ 24545 ┆ 71168 ┆ 10
AFTER exercise ┆ 5759 ┆ 42356 ┆ 45806 ┆ 115777 ┆ 18 withdraw ┆ 3075 ┆ 26968 ┆ 23807 ┆ 70836 ┆ 10
155c155,157 < mapping(uint256 => bool) public exercisedPositions; --- > function exercisedPositions(uint256 id) external view returns(bool) { > return ownerOf(id) == address(0xdead); > } 415,417d416 < // mark the position as exercised < exercisedPositions[uint256(orderHash)] = true; < 478c477 < bool isExercised = exercisedPositions[longPositionId]; --- > bool isExercised = ownerOf(longPositionId) == address(0xdead);
Because you already computed isExercised, you can save an SLOAD (2.1k gas) if you check for isExercised first.
require(block.timestamp > positionExpirations[longPositionId] || isExercised, "Must be exercised or expired");
You could also refactor to the reverse check (if you expect the majority of positions to expire worthless), either way the short circuit can be used to save a SLOAD, whicever priority you give it
require(isExercised || block.timestamp > positionExpirations[longPositionId], "Must be exercised or expired");
To save 2.1k gas if the option was exercised
Everytime a Storage Variable is loaded twice, (SLOAD), it would be cheaper to use a supporting memory variable The cost of a Second SLOAD is 100 gas, the cost of an MSTORE is 3 and and MLOAD another 3 gas.
Using a supporting variable will save 94 gas on the second read, and 97 gas for each subsquent MSTORE
In the case of fee this can save 94 gas
if (fee > 0) { feeAmount = (order.strike * fee) / 1000;
floorTokenIds will save gas (avoid copying whole storage to memory) - Between 400 and 2k gas saved on Withdraw and Exercise - 400 / 2k * 2 gas savedIf you pass a storage pointer as argument to a memory typed function like _transferFloorsOut
function _transferFloorsOut(address[] memory floorTokens, uint256[] memory floorTokenIds) internal {
You are copying the storage values to memory and then reading them and looping through that copy, this incurs an overhead and is equivalent to looping over storage, copying into memory and then passing the memory variable.
This is one of the few cases where a storage declaration (passing the storage pointer) will save gas
function _transferFloorsOut(address[] memory floorTokens, uint256[] storage floorTokenIds) internal {
exercise ┆ 5759 ┆ 55920 ┆ 68002 ┆ 133534 ┆ 18 withdraw ┆ 3075 ┆ 27840 ┆ 24545 ┆ 71168 ┆ 10
#### After
exercise ┆ 5759 ┆ 55176 ┆ 65795 ┆ 133534 ┆ 18
withdraw ┆ 3075 ┆ 27365 ┆ 24545 ┆ 71003 ┆ 10
// check floor asset token ids length is 0 unless the position type is put !order.isCall ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds") : require(floorAssetTokenIds.length == 0, "Invalid floor tokenIds length");
Because the logic is fully known, it would be cheaper to swap the if else to:
order.isCall ? require(floorAssetTokenIds.length == 0, "Invalid floor tokenIds length"); : require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
As that would avoid the extra NOT
Reading msg.value costs 2 gas, while reading from memory costs 3, this will save 1 gas with no downside
if (weth == order.baseAsset && msg.value > 0) {
Change to
if (msg.value > 0 && weth == order.baseAsset) {
Below are a bunch of basic gas saving tips you probably will receive in most competent submissions added below for the sake of completeness
orders.length - 3 gas per instance - 27 gas savedrequire(orders.length == signatures.length, "Length mismatch in input");
orders.length is read multiple times, because of that, especially for the loop, you should cache it in a memory variable to save 3 gas per instance
Refactor to:
ordersLength = orders.length; require(orders.length == signatures.length, "Length mismatch in input");
Declaring uint256 i = 0; means doing an MSTORE of the value 0
Instead you could just declare uint256 i to declare the variable without assigning it's default value, saving 3 gas per declaration
for (uint256 i = 0; i < orders.length; i++) {
You can get cheaper for loops (at least 25 gas, however can be up to 80 gas under certain conditions), by rewriting:
for (uint256 i = 0; i < orders.length; /** NOTE: Removed i++ **/ ) { // Do the thing // Unchecked pre-increment is cheapest unchecked { ++i; } }
if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {
Can be refactored with
// transfer strike to owner if put is expired or call is exercised if (order.isCall == isExercised) {
As in both case they were both true or both false
Because of the gas save above we can refactor the entire block to an if / else
if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) { // send the fee to the admin/DAO if fee is greater than 0% uint256 feeAmount = 0; if (fee > 0) { feeAmount = (order.strike * fee) / 1000; ERC20(order.baseAsset).safeTransfer(owner(), feeAmount); } ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount); return; } // transfer assets from putty to owner if put is exercised or call is expired if ((order.isCall && !isExercised) || (!order.isCall && isExercised)) { _transferERC20sOut(order.erc20Assets); _transferERC721sOut(order.erc721Assets); // for call options the floor token ids are saved in the long position in fillOrder(), // and for put options the floor tokens ids are saved in the short position in exercise() uint256 floorPositionId = order.isCall ? longPositionId : uint256(orderHash); _transferFloorsOut(order.floorTokens, positionFloorAssetTokenIds[floorPositionId]); return; }
Becomes
if (order.isCall == isExercised)) { // transfer assets from putty to owner if put is exercised or call is expired // send the fee to the admin/DAO if fee is greater than 0% uint256 feeAmount = 0; if (fee > 0) { feeAmount = (order.strike * fee) / 1000; ERC20(order.baseAsset).safeTransfer(owner(), feeAmount); } ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount); } else { // transfer assets from putty to owner if put is exercised or call is expired _transferERC20sOut(order.erc20Assets); _transferERC721sOut(order.erc721Assets); // for call options the floor token ids are saved in the long position in fillOrder(), // and for put options the floor tokens ids are saved in the short position in exercise() uint256 floorPositionId = order.isCall ? longPositionId : uint256(orderHash); _transferFloorsOut(order.floorTokens, positionFloorAssetTokenIds[floorPositionId]); }
Which reduces bytecode size, and saves gas in most situations as you're avoiding up to 3 extra comparisons
#0 - outdoteth
2022-07-07T20:17:48Z
cool idea with relying on 0xdead to see if an option is exercised or not. unfortunately this doesnt work because a user can intentionally "burn" their NFT to 0xdead which then messes up the logic in withdraw. This can be mitigated, but requires too many changes.
high quality report though.