Escher contest - RaymondFam's results

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L105 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L85-L86 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/FixedPrice.sol#L109 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/OpenEdition.sol#L92

Vulnerability details

Impact

The protocol uses Solidity’s transfer() when transferring ETH to the recipients. This has some notable shortcomings when the recipient is a smart contract, which can render ETH impossible to transfer. Specifically, the transfer will inevitably fail when the smart contract:

• does not implement a payable fallback function, or
• implements a payable fallback function which would incur more than 2300 gas units, or
• implements a payable fallback function incurring less than 2300 gas units but is called through a proxy that raises the call’s gas usage above 2300.

Proof of Concept

File: LPDA.sol

85:            ISaleFactory(factory).feeReceiver().transfer(fee);
86:            temp.saleReceiver.transfer(totalSale - fee);

105:        payable(msg.sender).transfer(owed);

File: FixedPrice.sol#L109

109:        ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);

File: OpenEdition.sol#L92

92:        ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);

Issues pertaining to the use of transfer() in the code blocks above may be referenced further via:

• CONSENSYS Diligence's article
• OpenZeppelin news & events

Tools Used

Manual inspection

Recommended Mitigation Steps

Using call with its returned boolean checked in combination with re-entrancy guard is highly recommended after December 2019.

For instance, line 105 in LPDA.sol may be refactored as follows:

- payable(msg.sender).transfer(owed);
+ (bool success, ) = payable(msg.sender).call{ value: owed }('');
+ require(success, " Transfer of ETH Failed");

Alternatively, Address.sendValue() available in OpenZeppelin Contract’s Address library can be used to transfer the Ether without being limited to 2300 gas units.

And again, in either of the above measures adopted, the risks of re-entrancy stemming from the use of this function can be mitigated by tightly following the “Check-effects-interactions” pattern and/or using OpenZeppelin Contract’s ReentrancyGuard contract.

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L68

Vulnerability details

Impact

A buyer could plan on buying early at higher prices to make sure he would secure a portion (say 50%) of NFTs he desired. When the number of NFTs still available got smaller and that sale.endTime were yet to hit, he would then watch the mempool and repeatedly attempt to thwart the final group of buyers from successfully completing their respective transactions amidst the efforts to prolong the Dutch auction till sale.endTime was reached.

Proof of Concept

Assuming this particular edition pertained to a 100 NFT collection that would at most last for 60 minutes, and Bob planned on minting 10 of them. At the beginning of the dutch auction, he would first mint 5 NFTs at higher prices no doubt. At 50th minute, sale.currentId == 95. Alice, upon seeing this, made up her mind and proceeded to buying the remaining NFTs. Bob, seeing this transaction queuing in the mempool, invoked buy() to mint 1 NFT by sending in higher amount of gas to front run Alice. Needless to say, Alice's transaction was going to revert on line 68 because newId == 101:

File: LPDA.sol#L68

68:        require(newId <= temp.finalId, "TOO MANY");

Noticing the number of NFTs still available had become 4, Alice attempted to mint the remaining 4 NFTs this time. Bob, upon seeing the similar queue in the mempool again, front ran Alice with another mint of 1 NFT.

These steps were repeatedly carried out until Bob managed to get all NFTs he wanted where the last one was minted right after sale.endTime hit. At this point, every successful buyers was happy to get the biggest refund possible ensuring that each NFT was only paid for the lowest price. This intended goal, on the contrary, was achieved at the expense of the seller getting the lowest amount of revenue and that the front run buyers minting nothing.

Tools Used

Manual inspection

Recommended Mitigation Steps

Considering refactoring the affected code line as follows:

- require(newId <= temp.finalId, "TOO MANY");
+ if(newId > temp.finalId) {
+      uint256 diff = newId - temp.finalId; 
+      newId = temp.finalId;
+      amountSold -= diff;
+      amount -= diff;
+ }

Two-step Transfer of Ownership

In Base.sol, the contract inherits from OwnableUpgradeable where _transferOwnership() is invoked in initialize() allowing the oldOwner to transfer ownership to the newOwner. This function does not implement zero address check (like transferOwnership does) and proceeds directly to write the new owner's address into the owner's state variable, _owner. If the nominated account is not a valid account, it is entirely possible the contract deployer may accidentally transfer ownership to an uncontrolled account or a zero address, breaking all functions with the onlyOwner() modifier.

Consider implementing a two step process where the deployer nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed (using OpenZeppelin's Ownable2StepUpgradeable.sol if need be). This will ensure the new owner is going to be fully aware of the ownership assigned/transferred other than having the above mistake avoided.

All other instances entailed:

File: FixedPrice.sol#L80

        _transferOwnership(_sale.saleReceiver);

File: OpenEdition.sol

        _transferOwnership(sale.saleReceiver);

File: LPDA.sol#L112

        _transferOwnership(sale.saleReceiver);

Shadowed Variable

The _owner state variable inherited from OwnableUpgradeable.sol is shadowed in initialize() of Base.sol.

File: Base.sol#L18-L20

    function initialize(address _owner) public initializer {
        _transferOwnership(_owner);
    }

Consider renaming this parameter to owner_ to avoid shadowing the inherited variable.

Add a Constructor Initializer

As per Openzeppelin's recommendation:

https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/6

The guidelines are now to prevent front-running of initialize() on an implementation contract, by adding an empty constructor with the initializer modifier. Hence, the implementation contract gets initialized atomically upon deployment.

This feature is readily incorporated in the Solidity Wizard since the UUPS vulnerability discovery. You would just need to check UPGRADEABILITY to have the following constructor code block added to the contract:

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
    _disableInitializers();
}

Here are the contract instances entailed:

File: Base.sol#L18

    function initialize(address _owner) public initializer {

File: Escher721.sol#L25

    constructor() {}

Inadequate NatSpec

Solidity contracts can use a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables and more. Please visit the following link for further details:

https://docs.soliditylang.org/en/v0.8.16/natspec-format.html

Here are the contract instances lacking NatSpec mostly in its entirety:

File: Unique.sol File: Base.sol

Here are the contract instances partially lacking NatSpec:

File: Generative.sol File: Escher721Factory.sol File: OpenEditionFactory.sol File:LPDAFactory.sol File: Escher.sol File: Escher721.sol File: OpenEdition.sol File: LPDA.sol

Complementary Codehash Checks

Consider implementing an optional codehash check for immutable contract addresses at the constructor that will assure matching inputs of constructor parameters.

For instance, the following code block may be refactored as follows:

File: Escher721Factory.sol#L17-L18

-    constructor(address _escher) {
+    constructor(address _escher, bytes32 _escherCodeHash) {  
+        require(_escher.codehash == _escherCodeHas, "INVALID ADDRESS");
        escher = Escher(_escher);
        ...

Lack of Events for Critical Operations

Critical operations not triggering events will make it difficult to review the correct behavior of the deployed contracts. Users and blockchain monitoring systems will not be able to detect suspicious behaviors at ease without events. Consider adding events where appropriate for all critical operations for better support of off-chain logging API.

Here are the instances entailed:

File: Base.sol

10:    function setBaseURI(string memory _baseURI) external onlyOwner {

File: Generative.sol

14:    function setScriptPiece(uint256 _id, string memory _data) external onlyOwner {

19:    function setSeedBase() external onlyOwner {

File: FixedPriceFactory.sol#L31

42:    function setFeeReceiver(address payable fees) public onlyOwner {

File: OpenEditionFactory.sol

42:    function setFeeReceiver(address payable fees) public onlyOwner {

File: LPDAFactory.sol

46:    function setFeeReceiver(address payable fees) public onlyOwner {

File: Escher721.sol

57:    function updateURIDelegate(address _uriDelegate) public onlyRole(URI_SETTER_ROLE) {

Constructor of Escher.sol

According to README.md, only assigned roles as a Curator or a Creator are ERC1155 soulbound tokens. However, the contract deployer, msg.sender, would also join the party as a soulbound token that could create confusion to the users and the protocol.

Consider having the function call in the constructor refactored as follows:

File: Escher.sol#L15-L67

    constructor() ERC1155("") {
-        _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
+        super._grantRole(DEFAULT_ADMIN_ROLE, msg.sender);

CURATOR_ROLE in Escher.sol

No default CURATOR_ROLE was assigned upon the contract deployment of Escher.sol despite this could separately be done outside the contract.

Consider having the following code line added to the constructor, serving also as an emergency measure by allowing the contract owner to assume the curator role when need be:

        _grantRole(CURATOR_ROLE, msg.sender);

Additionally, it is recommended implementing addCurator() in the contract that could look something as follows:

    function addCurator(address _account) public onlyRole(DEFAULT_ADMIN_ROLE) {
        _grantRole(CURATOR_ROLE, _account);
    }

Inherited Calls to AccessControl.sol

Throughout the code bases, it is noted that the protocol always skip the preliminary functions when making inherited calls to AccessControl.sol. For instance, it would call _transferOwnership() instead of transferOwnership(), _grantRole() instead of grantRole(), and _revokeRole() instead of revokeRole().

While it is understandable that the adoption of _grantRole() would free up more roles other than the admin to carry out their respective duties, care must be taken for the other two calls.

As earlier explained at the top of this report, _transferOwnership() skips zero address check which is duly acknowledged as outside of the scope of this contest.

_revokeRole(), as the name of the function suggests, could sometimes trick developer into thinking an admin modifier would have been taken care of at AccessControl.sol when it is not. It is recommended using revokeRole() as a double measure to better prevent this critical call from being invoked by anyone.

_safeMint() Over _mint() for ERC721 Tokens

Consider using _safeMint() instead of _mint() since the former would determine whether or not the recipient is a contract that has a logic implemented to support ERC721 standard via onERC721Received(). This is to prevent any token from getting trapped in a smart contract that cannot feature future token transfer.

Here is the instance entailed:

File: Escher721.sol#L52

        _mint(to, tokenId);

Cached Variable Not Fully Utilized

In OpenEdition.sol, buy() has the struct, sale, cached on line 59 as temp. However, on line 63, the require statement is using the state variable equivalent instead of the stack local variable. Consider having the affected code line refactored as follows to save gas as originally intended:

File: OpenEdition.sol#L63

-        require(amount * sale.price == msg.value, "WRONG PRICE");
+        require(amount * temp.price == msg.value, "WRONG PRICE");

Unneeded Cached Variable

In OpenEdition.sol, buy() has its parameter, _amount, cast into uint24 prior to getting it assigned to amount on line 58. Caching a function parameter to use it twice has very minimal gas saving benefit unless this pertains to the length of an array getting into a for loop. Consider having the affected code block refactored as follows:

File: OpenEdition.sol#L57-L72

58: - uint24 amount = uint24(_amount);
...
64: - uint24 newId = amount + temp.currentId;
    + uint24 newId = uint24(_amount) + temp.currentId; 
...
71: - emit Buy(msg.sender, amount, msg.value, temp);
    + emit Buy(msg.sender, _amount, msg.value, temp);

Use of Named Returns for Local Variables Saves Gas

You can have further advantages in term of gas cost by simply using named return values as temporary local variable.

As an example, the following instance of code block can refactored as follows:

File: Unique.sol#L10-L11

-    function tokenURI(uint256 _tokenId) external view virtual override returns (string memory) {
-        return string.concat(baseURI, _tokenId.toString());
+    function tokenURI(uint256 _tokenId) external view virtual override returns (string memory _tokenURI) {
+        _tokenURI string.concat(baseURI, _tokenId.toString());

All other instances entailed:

File: Base.sol

14:    function tokenURI(uint256) external view virtual returns (string memory) {

File: Generative.sol

27:    function tokenToSeed(uint256 _tokenId) internal view returns (bytes32) {

File: Escher.sol

33:    ) public view override(ERC1155, AccessControl) returns (bool) {

File: Escher721.sol

80:    ) public view override(ERC721Upgradeable) returns (string memory) {

90:        returns (bool)

File: FixedPrice.sol

86:    function getPrice() public view returns (uint256) {

91:    function startTime() public view returns (uint256) {

96:    function editionContract() public view returns (address) {

101:    function available() public view returns (uint256) {

File: OpenEdition.sol

97:    function getPrice() public view returns (uint256) {

102:    function startTime() public view returns (uint256) {

107:    function timeLeft() public view returns (uint256) {

112:    function editionContract() public view returns (address) {

116:    function available() public view returns (uint256) {

File: LPDA.sol

117:     function getPrice() public view returns (uint256) {

128:    function startTime() public view returns (uint256) {

133:    function editionContract() public view returns (address) {

138:    function available() public view returns (uint256) {

142:    function lowestPrice() public view returns (uint256) {

Payable Access Control Functions Costs Less Gas

Consider marking functions with access control as payable. This will save 20 gas on each call by their respective permissible callers for not needing to have the compiler check for msg.value.

Here are the instances entailed:

File: Base.sol

10:    function setBaseURI(string memory _baseURI) external onlyOwner {

File: Generative.sol

14:    function setScriptPiece(uint256 _id, string memory _data) external onlyOwner {

19:    function setSeedBase() external onlyOwner {

File: FixedPriceFactory.sol

42:    function setFeeReceiver(address payable fees) public onlyOwner {

File: OpenEditionFactory.sol

42:    function setFeeReceiver(address payable fees) public onlyOwner {

File: LPDAFactory.sol

46:    function setFeeReceiver(address payable fees) public onlyOwner {

File: Escher.sol

21:    function setURI(string memory _newuri) public onlyRole(DEFAULT_ADMIN_ROLE) {

27:    function addCreator(address _account) public onlyRole(CURATOR_ROLE) {

File: Escher721.sol

51:    function mint(address to, uint256 tokenId) public virtual onlyRole(MINTER_ROLE) {

57:    function updateURIDelegate(address _uriDelegate) public onlyRole(URI_SETTER_ROLE) {

67:    ) public onlyRole(DEFAULT_ADMIN_ROLE) {

72:    function resetDefaultRoyalty() public onlyRole(DEFAULT_ADMIN_ROLE) {

File: FixedPrice.sol

50:    function cancel() external onlyOwner {

File: OpenEdition.sol

75:     function cancel() external onlyOwner {

File: LPDA.sol

92:    function cancel() external onlyOwner {

Non-strict inequalities are cheaper than strict ones

In the EVM, there is no opcode for non-strict inequalities (>=, <=) and two operations are performed (> + = or < + =).

As an example, consider replacing >= with the strict counterpart > in the following inequality instance:

File: FixedPriceFactory.sol#L31

-        require(_sale.startTime >= block.timestamp, "START TIME IN PAST");
+        require(_sale.startTime > block.timestamp - 1, "START TIME IN PAST");        

All other >= instances entailed:

File: OpenEditionFactory.sol

31:        require(sale.startTime >= block.timestamp, "START TIME IN PAST");

File: LPDAFactory.sol

32:        require(sale.startTime >= block.timestamp, "INVALID START TIME");

File: FixedPrice.sol

        require(block.timestamp >= sale_.startTime, "TOO SOON");

File: OpenEdition.sol

61:        require(block.timestamp >= temp.startTime, "TOO SOON");

91:        require(block.timestamp >= temp.endTime, "TOO SOON");

Similarly, consider replacing <= with the strict counterpart < in the following inequality instance, as an example:

File: FixedPrice.sol#L63

-        require(newId <= sale_.finalId, "TOO MANY");
+        require(newId < sale_.finalId + 1, "TOO MANY");

All other <= instances entailed:

[File: FixedPrice.sol]https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/FixedPrice.sol)

65:        for (uint48 x = sale_.currentId + 1; x <= newId; x++) {

File: OpenEdition.sol

66:        for (uint24 x = temp.currentId + 1; x <= newId; x++) {

Use Storage Instead of Memory for Structs/Arrays

A storage pointer is cheaper since copying a state struct in memory would incur as many SLOADs and MSTOREs as there are slots. In another words, this causes all fields of the struct/array to be read from storage, incurring a Gcoldsload (2100 gas) for each field of the struct/array, and then further incurring an additional MLOAD rather than a cheap stack read. As such, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables will be much cheaper, involving only Gcoldsload for all associated field reads. Read the whole struct/array into a memory variable only when it is being returned by the function, passed into a function that requires memory, or if the array/struct is being read from another memory array/struct.

Here are the instances entailed:

File: FixedPrice.sol

58:        Sale memory sale_ = sale;

File: OpenEdition.sol

59:        Sale memory temp = sale;

90:        Sale memory temp = sale

File: LPDA.sol

60:         Sale memory temp = sale;

100:        Receipt memory r = receipts[msg.sender];

118:        Sale memory temp = sale;

+= and -= Cost More Gas

+= and -= generally cost 22 more gas than writing out the assigned equation explicitly. The amount of gas wasted can be quite sizable when repeatedly operated in a loop.

Here are the += instances entailed that may be refactored as follows:

File: LPDA.sol#L66-L71

66: - amountSold += amount;
    + amountSold = amountSold + amount;
...
70: - receipts[msg.sender].amount += amount;
    + receipts[msg.sender].amount = receipts[msg.sender].amount + amount;
71: - receipts[msg.sender].balance += uint80(msg.value);
    + receipts[msg.sender].balance = receipts[msg.sender].balance + uint80(msg.value);

Function Order Affects Gas Consumption

The order of function will also have an impact on gas consumption. Because in smart contracts, there is a difference in the order of the functions. Each position will have an extra 22 gas. The order is dependent on method ID. So, if you rename the frequently accessed function to more early method ID, you can save gas cost. Please visit the following site for further information:

https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

Activate the Optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
  solidity: {
    version: "0.8.17",
    settings: {
      optimizer: {
        enabled: true,
        runs: 1000,
      },
    },
  },
};

Please visit the following site for further information:

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

Here's one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization
PUSH1 0x00
DUP2
EQ
ISZERO
PUSH1 [cont offset]
JUMPI

after optimization
DUP1
PUSH1 [revert offset]
JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past . A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

Unchecked SafeMath Saves Gas

"Checked" math, which is default in ^0.8.0 is not free. The compiler will add some overflow checks, somehow similar to those implemented by SafeMath. While it is reasonable to expect these checks to be less expensive than the current SafeMath, one should keep in mind that these checks will increase the cost of "basic math operation" that were not previously covered. This particularly concerns variable increments in for loops.

When no arithmetic overflow/underflow is going to happen, unchecked { x++ ;} in the code block to use the previous wrapping behavior further saves gas just as in the for loop below as an example:

File: OpenEdition.sol#L66-L68

        for (uint24 x = temp.currentId + 1; x <= newId;) {
            nft.mint(msg.sender, x);

            unchecked {
                x++;
            }
        }

All other instances entailed:

File: LPDA.sol

73:        for (uint256 x = temp.currentId + 1; x <= newId; x++) {

File: FixedPrice.sol

65:        for (uint48 x = sale_.currentId + 1; x <= newId; x++) {