orbland / contracts Goto Github PK

Description

The codebase makes extensive use of modifiers. There is room for consolidation and simplification to improve the readability of the codebase.

Suggestion

• Remove modifiers from getter functions. The clients that pull the data should be informed when the data returned by these getter functions makes or doesn't make sense. Functions: minimumBid(), lastSettlementTime, holderSolvent, price(), foreclosureTime, cooldownRemaining
• Remove modifiers that offer UX guardrails but no real protocol value: hasFunds
• #30

Description

The modifiers onlyHolderSolvent and onlyHolder are always used together in the codebase. That indicates that they should not be different modifiers.

Suggestion

Simplify the codebase, remove the modifiers, and introduce onlySolventHolder modifier that checks for both a) solvency and b) that msg.sender is the holder.

Description

The codebase uses native ether. It's considered a bad practice for the following reasons:

• Different logic between handling ERC20 and Ether. That leads to more codepaths and thus potential attack surface
• Potential reentrancy shenanigans from sending ether (if the recipient is a contract, it can trigger fallback/receive functions
• Potential loss of precision from doing complex math without a math library (e.g PRBMath

Suggestion

• Use weth instead and make it easy for users to wrap their ether via the front-end. WETH unifies the handling of tokens, be it ERC20 or wrapped ether

Description

A holder can flag the response to the question of a previous holder.

• Alice is a holder of the Orb and triggers it. She receives an answer A, and she is happy about it. That happens on day 0.
• Bob purchases the Orb from Alice on the following day, day 1. He can call flagResponse, and give the triggerId of Alice's invocation.

This is because all the following checks will pass:

• A response exists for the triggerId
• The flagging period has not expired (7 days)
• The response has not been flagged

if (!_responseExists(triggerId)) {
    revert ResponseNotFound(triggerId);
}

if (block.timestamp - responses[triggerId].timestamp > RESPONSE_FLAGGING_PERIOD) {
    revert FlaggingPeriodExpired(
        triggerId, block.timestamp - responses[triggerId].timestamp, RESPONSE_FLAGGING_PERIOD
    );
}

if (responseFlagged[triggerId] == true) {
    revert ResponseAlreadyFlagged(triggerId);
}

Suggestion

• Introduce a storage variable flaggingEnd
• In trigger(), add the following: flaggingEnd = block.timestamp + RESPONSE_FLAGGING_PERIOD;
• In flagResponse check that: block.timestamp < flaggingEnd
• In purchase() and foreclose() add: flaggingEnd = block.timestamp

Description

The function serves only as a getter. It's simpler to make the storage variable public, thus removing the need of the function and simplifying the codebase.

Storage

• Make the storage variable public
• Remove the function

Description

The modifier is mainly used in external functions, which by definition, can not be called by the contract. The only public function that uses the modifier is lastSettlementTime, where the modifier is used as a UX guardrail since the returned variable only makes sense if a user holds it. The orb. Removing the modifier would not affect the protocol in any way (as long as that function is concerned).

Suggestion

The modifier should be removed in an effort to simplify the codebase, as no real benefit rises from it's use.

Description

Solmate offers a more gas efficient version of the ERC721 token than OpenZeppelin

Suggestion

Migrate the codebase to use Solmate

Description

Currently purchase(uint256 currentPrice, uint256 newPrice) checks that newPrice != 0. This is not required, as the protocol can function as expected if newPrice = 0, since the user can purchase the orb with a non zero new price and then set it to zero via setPrice(uint256 newPrice).

Suggestion

Remove the check

Description

In _withdraw(), we first check for sufficient funds before subtracting the amount to be withdrawn. This is not required as Solidity 0.8.X ships with underflow/overflow assurances. Thus, when removing from the mapping of the amount is larger than the available funds of the user, the subtraction will cause an underflow and will revert.

The check incurs gas costs without the need for it.

Suggestion

Remove the if clause at the start of the function

Description

fundsOf mapping can be public, without needing a specific user-defined getter function.

Suggestion

• Make the mapping public
• Remove fundsOf()

Description

In finalizeAuction() the event emission is placed inside the if/else clause. That is not required and leads to duplicated code.

Suggestions

Move the event emission outside the if/else clause, at the end.

emit AuctionFinalized(winningBidder, winningBid);
startTime = 0;
endTime = 0;

Description

In various parts of the codebase, CEI pattern needs to be more strictly followed.

• line 777
• line 792
• line 457
• line 731

Suggestion

Reorder lines to follow CEI pattern

Description

The use of _msgSender() helper function by OpenZeppelin is not required, as the contract is not intended to be used with meta-transactions. It's more gas efficient to use msg.sender.

Suggestion

Replace _msgSender() with msg.sender throughout the contract.

Description

#26

Suggestion

Removee onlyHolderHeld if onlyHolder and external are present in the function.

Affected functions: setPrice, exit, flagResponse.

After discussions with @lekevicius, we aligned on removing the requirement from the protocol side of transferring more funds than the bid amount and adding UI affordances to help the user calculate how much funds they have to transfer to support a bid for X amount of time.

Description

The codebase does not have a CI/CD pipeline that runs the test suite against every new PR. That means that buggy code could be merged at some point and eventually be deployed.

Suggestion

Install a GitHub Action that runs the test suite against every new PR and every commit to main.

Description

The variable _price is not required to be internal/private and have a public getter function. The modifiers in the function are used to enforce when price should be called, as price doesn't make sense in all the states of the smart contract (e.g when owned by the contract itself).

Suggestion

I suggest we remove the getter function and make the variable public. Interfaces and smart contracts built on top of the orb should know when price makes sense and when it should not be read. This will remove unnecessary code from the smart cotnract.

Description

When trigger() is invoked by the holder, they can submit cleartext and hash, with cleartext being optional. The cleartext and contentHash need not to be stored.

The reason is that the storage happens so that there is an immutable record of that trigger and its question. It's a social contract. That means that storage is not required, but rather just the fact that the hash and/or cleartext was submitted.

That means that even if the hash were submitted as a calldata argument without actually being used, the requirement for proof would be met with minimal gas implications. The calldata are part of the transaction and, thus, impossible to forge or alter.

Suggestion

Simply accept the data as calldata and emit the hash for ease of use by 3rd party apps and interfaces that build on top of the orb. The increment of the storage variable triggersCount can happen in the event definition. triggersCount++ will return triggersCount and then increment it.

function triggerText(string memory text) external onlyHolder onlyHolderSolvent {
    if (block.timestamp < lastTriggerTime + COOLDOWN) {
        revert CooldownIncomplete(lastTriggerTime + COOLDOWN - block.timestamp);
    }
    lastTriggerTime = block.timestamp;
    emit Triggered(msg.sender, triggersCount++,  keccak256(bytes(text)), block.timestamp);

}

function triggerHash(bytes32 contentHash) external onlyHolder onlyHolderSolvent
{
    if (block.timestamp < lastTriggerTime + COOLDOWN) {
        revert CooldownIncomplete(lastTriggerTime + COOLDOWN - block.timestamp);
    }
    lastTriggerTime = block.timestamp;
    emit Triggered(msg.sender, triggersCount++, contentHash, block.timestamp);
}

Description

A sophisticated user can perform the next attack after they become holders of the orb and render the orb impossible to be bought by another person, while also keeping the orb's price to a minimum (even zero).

The user would need to set a relatively simple front-running bot that does the following:

• listen for invocations of purchase(oldPrice, newPrice)
• frontrun these transactions with a transaction that sets the price to something other than the current price, e.g. setPrice(1).
• bid high enough so that the transaction comes before the one that calls purchase()

Then, the purchase(oldPrice, newPrice) will always revert, as the oldPrice will have changed. By setting the price to something as small as 1*10e-18, the attacker can invalidate the purchase and, simultaneously, have a little economic cost ( in the context of the orb, the gas cost could be high). The attacked can do that and invalidate all purchase attempts while at the same time keeping themselves solvent (as the price is so low). Thus the issuer can never foreclose the orb and take back control.

It's worth noting that since the Orb has an essential element of a social contract, it doesn't mean that the attacked can reap the benefits of the Orb, as the issue can clearly see the attack and simply select to ignore any triggers. The orb is more or less bricked, and thus, the attack is more of a grieving attack.

Finally, a white-hat hacker could attempt to front-run the attacker's bot and, by winning a gas auction, get their purchase() transaction in front of the attacker's and thus unbrick the orb.

Suggestion

Add a cooldown to set the price, for example, once per day:

uint256 cooldown;
function _setPrice(uint256 newPrice_) internal {
	if(block.timestamp > cooldown){ 
	    cooldown = block.timestamp + 24 hours;
	    if (newPrice_ > MAX_PRICE) {
	        revert InvalidNewPrice(newPrice_);
	    }
	
	    uint256 oldPrice = _price;
	    _price = newPrice_;
	
	    emit NewPrice(oldPrice, newPrice_);
	    } else {
		revert SetPriceCooldown;
	}
}

Description

The CEI pattern is a well-documented best practices approach that safeguards against vulnerabilities like reentrancies. Although no such vulnerability exists in the purchase() function, it would be best to be changed, so the pattern is followed.

Suggestion

_transfer(holder, msg.sender, ERIC_ORB_ID);
_lastSettlementTime = block.timestamp;

_setPrice(newPrice);

emit Purchase(holder, msg.sender);

should be:

_lastSettlementTime = block.timestamp;
_setPrice(newPrice);
emit Purchase(holder, msg.sender);
_transfer(holder, msg.sender, ERIC_ORB_ID);

Description

In line 416, the mapping is read and then copied to a memory variable. This is not required, as the value is only read once, thus it's more gas efficient to directly read from the mapping without declaring a memory variable in the first place.

Suggestion

uint256 totalFunds = _funds[_msgSender()] + msg.value;

Description

When a user calls purchase, they input the currentPrice. It's cheaper to use the calldata input variable than the storage variable _price throughout the function.

Suggestion

Replace_price with currentPrice

Description

triggerExists can be simplified by simply checking the triggerCount. In essence, the function can be removed altogether and be replaced by a simple check:

triggerId < triggerCount

If a triggerId is lower than the triggerCount, then the triggerId exists. Otherwise, it doesn't.

We can verify the fact by looking at trigger and seeing that the count is incremented with every new invocation of trigger. There is no other way for the counter to be incremented.

Suggestion

• Remove function and replace it's invocation with triggerId < triggerCount

Description

In line 373, an addition is made that we know will never overflow. By adding unchecked { .. } around it, we inform the compiler that it should not check for overflow, and thus, we save a tiny bit of gas.

Suggestion

unchecked { 
	return winning bid + MINIMUM_BID_STEP;
}

Description

foreclosureTime does not need to have an access modifier for the public getter and be internal. It's a UX guardrail that increases the codebase with no real protocol value.

Moreover, no need to check that _price != 0. If it's set to zero, it will simply revert.

Suggestion

• Make function public, remove modifier
• Remove check for zero that returns INFINITY

Description

Private variables make it very hard to inspect their state and thus test against them. It's best to use internal as there is no difference for the grand majority of use-cases.

Suggestion

Replace private variables with internal.

Description

In line 385, the memory variable is not required. We can save gas by removing it and doing the calculation directly in the return statement.

Suggestion

Remove the memory variable

Description

No need to check for address(0) in fundsOf(). The returned data don't have any meaning in the context of the Orb, but it doesn't pose any danger to the protocol. The imposed gas cost on every invocation of fundsOf, due to the check, can be avoided.

Suggestion

Remove the check

Description

_settle() does mainly two things:

• Computes the effectiveFunds of the holder and the owner
• Assigns that effectiveFunds to the respective fields of the _funds mapping.

It replicates the calculations inside effectiveFunds, which can lead to a possible source of errors in the future if not handled carefully. Codepaths should be unique, especially in paths that do math calculations.

Suggestion

The codepaths should be unified. An example:

1. effectiveFundsOf will be converted to public instead of external
2. _settle() will change as follows:

  function _settle() internal {
    address holder = ERC721.ownerOf(ERIC_ORB_ID);

    if (owner() == holder) {
      return;
    }

    // Should never be reached if this contract holds the orb.
    assert(address(this) != holder);

	uint256 ownerEffective = effectiveFundsOf(owner());
    uint256 transferableToOwner = _funds[owner()]  - ownerEffective;

    _funds[holder] = effectiveFundsOf(holder);
    _funds[owner()]  = ownerEffective;
    
    _lastSettlementTime = block.timestamp;

    emit Settlement(holder, owner(), transferableToOwner);
  }

Description

OZ ERC721 contract is excellent but could be more gas optimized, as it prioritizes guardrails above all. Moreover, the orb has some assumptions that render OZ default implementation moot, for example, the existence of only a single NFT. That means that we can have many gas optimization by simply not using a mapping to retrieve the owner of the NFT, but rather a simple storage variable address holder.

By overriding ERC721 functions, we can create ERC721 compatible parts that are informed of these design characteristics of the Orb and thus make gas optimized informed decisions

Gas costs before

|                    Solc version: 0.8.17                    ·  Optimizer enabled: true  ·  Runs: 1000  ·  Block limit: 30000000 gas  │
·····························································|···························|··············|······························
|  Methods                                                                                                                            │
··································|··························|·············|·············|··············|···············|··············
|  Contract                       ·  Method                  ·  Min        ·  Max        ·  Avg         ·  # calls      ·  usd (avg)  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  bid                     ·      41100  ·      95093  ·       67659  ·           14  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  closeAuction            ·      31728  ·     154895  ·      140405  ·           17  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  deposit                 ·      30221  ·      47321  ·       40607  ·            8  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  exit                    ·          -  ·          -  ·       82306  ·            3  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  flagResponse            ·          -  ·          -  ·       82130  ·            2  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  foreclose               ·          -  ·          -  ·       73049  ·            4  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  purchase                ·      85135  ·     109835  ·       97018  ·           12  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  recordTriggerCleartext  ·          -  ·          -  ·       59553  ·            2  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  respond                 ·      73092  ·      73104  ·       73098  ·            4  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  setPrice                ·      33565  ·      52802  ·       44766  ·            6  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  settle                  ·      26021  ·      46417  ·       39618  ·            6  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  startAuction            ·          -  ·          -  ·       76160  ·            8  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  trigger                 ·     108439  ·     131627  ·      114294  ·           10  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  withdraw                ·      39992  ·      58047  ·       50309  ·            7  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  withdrawAll             ·      35126  ·      53181  ·       39138  ·            9  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  Deployments                                               ·                                          ·  % of limit   ·             │
·····························································|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb                             ·          -  ·          -  ·     3265713  ·       10.9 %  ·          -  │
·------------------------------------------------------------|-------------|-------------|--------------|---------------|-------------·

Gas costs after

·------------------------------------------------------------|---------------------------|--------------|-----------------------------·
|                    Solc version: 0.8.17                    ·  Optimizer enabled: true  ·  Runs: 1000  ·  Block limit: 30000000 gas  │
·····························································|···························|··············|······························
|  Methods                                                                                                                            │
··································|··························|·············|·············|··············|···············|··············
|  Contract                       ·  Method                  ·  Min        ·  Max        ·  Avg         ·  # calls      ·  usd (avg)  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  bid                     ·      40859  ·      94777  ·       67386  ·           14  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  closeAuction            ·      31604  ·     129218  ·      117734  ·           17  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  deposit                 ·      30083  ·      47183  ·       40433  ·            8  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  exit                    ·          -  ·          -  ·       56233  ·            3  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  flagResponse            ·          -  ·          -  ·       81711  ·            2  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  foreclose               ·          -  ·          -  ·       46986  ·            4  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  purchase                ·      59044  ·      83744  ·       70927  ·           12  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  recordTriggerCleartext  ·          -  ·          -  ·       59112  ·            2  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  respond                 ·      73092  ·      73104  ·       73098  ·            4  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  setPrice                ·      32987  ·      52224  ·       44188  ·            6  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  settle                  ·      25737  ·      46133  ·       39334  ·            6  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  startAuction            ·          -  ·          -  ·       76004  ·            8  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  trigger                 ·     108020  ·     131208  ·      113875  ·           10  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  withdraw                ·      39854  ·      57763  ·       50088  ·            7  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb  ·  withdrawAll             ·      34988  ·      52897  ·       38968  ·            9  ·          -  │
··································|··························|·············|·············|··············|···············|··············
|  Deployments                                               ·                                          ·  % of limit   ·             │
·····························································|·············|·············|··············|···············|··············
|  contracts/EricOrb.sol:EricOrb                             ·          -  ·          -  ·     2950829  ·        9.8 %  ·          -  │
·------------------------------------------------------------|-------------|-------------|--------------|---------------|-------------·

Suggestion

• introduce a new storage variable: address public holder
• in the constructor, replace _safeMint with holder = address(this)
• Replace all ERC721.ownerOf(ERIC_ORB_ID) with holder
• Override as follows:

function tokenURI(uint256 tokenId) public view override returns (string memory) {
    string memory baseURI = _baseURI();
    return bytes(baseURI).length > 0 ? string.concat(baseURI, "69") : "";
}

function _transfer(address, address to, uint256) internal override {
    holder = to;
    emit Transfer(from, to, ERIC_ORB_ID);
}

function ownerOf(uint256) public view virtual override returns (address) {
    return holder;
}

function balanceOf(address owner) public view override returns (uint256) {
    return owner == holder ? 1 : 0;
}

Description

finalizeAuction doesn't need to check for contract ownership via the onlyContractHeld modifier, since this is checked when the Auction starts. Thus, to get into the auction state, the contract needs to own the orb.

Moreover, it's not possible to exit the auction state without calling finalizeAuction(). Thus it's impossible to change owners of the Orb before calling finalizeAuction()

Suggestion

Remove the modifier