auditsbyradev - Due to premature `forkEscrow` updates, the user will lose their escrow nouns

[sherlock-admin4]

auditsbyradev

high

Due to premature forkEscrow updates, the user will lose their escrow nouns

Summary

Throughout the escrow phase, users have the option to deposit or retrieve their assets into forkHoldings. Additionally, actions outlined in proposals can be carried out during this time frame.

Due to premature forkEscrow updates, the user will lose their escrow nouns.

Vulnerability Detail

The problem is that, if the DAO's escrow contract address (forkEscrow) is updated before the threshold for a successful fork (forkThreshold) is reached. This update can lead to a scenario where Nouns tokens escrowed in the original forkEscrow become inaccessible because the withdrawal mechanism exclusively interacts with the current forkEscrow address set in the DAO's storage.

Impact

This flaw can have several detrimental impacts:

• Loss of Tokens: Users who have escrowed their Nouns tokens in anticipation of a fork may lose access to these tokens if the forkEscrow address is updated before the fork is finalized.
• Reduced Trust: The potential for token loss undermines trust in the DAO's governance mechanisms, potentially deterring participation in future proposals and escrow activities.
• Governance Disruption: The ability to change the forkEscrow address and impact escrowed tokens can be exploited to disrupt the governance process, particularly in sensitive periods of potential forking.

Code Snippet

• NounsDAOFork.sol#withdrawFromForkEscrow() function:

  /**
   * @notice Withdraw Nouns from the fork escrow. Only possible if the fork has not been executed.
   * Only allowed to withdraw tokens that the sender has escrowed.
   * @param tokenIds the tokenIds to withdraw
   */
  function withdrawFromForkEscrow(NounsDAOTypes.Storage storage ds, uint256[] calldata tokenIds) external {
      if (isForkPeriodActive(ds)) revert ForkPeriodActive();
  
      INounsDAOForkEscrow forkEscrow = ds.forkEscrow;
      forkEscrow.returnTokensToOwner(msg.sender, tokenIds);
  
      emit WithdrawFromForkEscrow(forkEscrow.forkId(), msg.sender, tokenIds);
  }

• NounsDAOForkEscrow.sol#returnTokensToOwner() function:

  /**
   * @notice Unescrows nouns tokens
   * @dev Can only be called by the DAO contract
   * @param owner The address which asks to unescrow, must be the address which escrowed the tokens
   * @param tokenIds The ids of the tokens being unescrowed
   */
  function returnTokensToOwner(address owner, uint256[] calldata tokenIds) external onlyDAO {
      for (uint256 i = 0; i < tokenIds.length; i++) {
          if (currentOwnerOf(tokenIds[i]) != owner) revert NotOwner();
  
          nounsToken.transferFrom(address(this), owner, tokenIds[i]);
          escrowedTokensByForkId[forkId][tokenIds[i]] = address(0);
      }
  
      numTokensInEscrow -= tokenIds.length;
  }

• NounsDAOAdmin.sol#_setForkEscrow() function

  /**
   * @notice Admin function for setting the fork escrow contract
   */
  function _setForkEscrow(address newForkEscrow) public onlyAdmin {
      emit ForkEscrowSet(address(ds().forkEscrow), newForkEscrow);
  
      ds().forkEscrow = INounsDAOForkEscrow(newForkEscrow);
  }

Tool used

Manual Review

Recommendation

I suggest several things:

1. Enable users to directly interact with the forkEscrow contract to withdraw their escrowed tokens. This reduces dependency on the DAO contract for escrow management.
2. Incorporate isForkPeriodActive checks within the forkEscrow contract's returnTokensToOwner function to centralize state validation and prevent premature withdrawal.
3. During active escrow periods leading up to a potential fork, consider making the forkEscrow address immutable or subject to stricter change controls, such as requiring a supermajority for updates.

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

WangAudit commented:

I believe this one is invalid; 1. NounsDAO can warn users about the change so they withdraw everything. 2. As I see using _setForkEscrow DAO can set back the previous forkEscrow and users will be able to withdraw them. 3. I believe admins are trusted not to update escrow contract before reaching the threshold

[radeveth]

I think that this issue is not only valid but also highlights critical aspects of trust, governance integrity, and smart contract design within the Nouns DAO protocol.

While the DAO can indeed warn users about changes to the escrow contract, relying solely on user notifications for critical actions such as withdrawals presents a significant risk. This approach assumes that all users will be adequately informed and able to act in time, a dangerous presumption in the decentralized and autonomous environment where users span different time zones and levels of engagement.

The suggestion that the DAO could revert to the previous forkEscrow to enable withdrawals overlooks the technical and operational complexities involved in managing smart contract states and addresses. Furthermore, it fails to account for the trust and logistical issues arising from toggling critical infrastructure elements like escrow addresses.

Assuming that administrators will act in good faith and not prematurely update the escrow contract before reaching the fork threshold underestimates the potential for human error or malicious actions. The decentralized ethos of DAOs seeks to minimize reliance on trust in individuals or groups by codifying rules and procedures within the smart contracts themselves.

The mechanism for withdrawing tokens from escrow, which requires interaction with the DAO contract that in turn calls the escrow contract, is a design that inherently limits direct control by token holders over their assets. This design choice, while potentially streamlining governance processes, introduces a vector for loss of control over assets if the escrow contract is swapped out without a corresponding upgrade in the DAO governance logic to facilitate withdrawals from the deprecated escrow.

Given these considerations, the vulnerability poses a real threat to the integrity of the governance process and the security of assets escrowed by participants in anticipation of a fork. It underscores a critical balance that must be maintained between administrative flexibility and the immutable assurances provided by smart contracts in decentralized systems.

For a token holder to retrieve their tokens from escrow, they must initiate a DAO function that subsequently interacts with the escrow. This setup restricts the escrow's withdrawal function to being callable solely by the DAO. Should a proposal be approved to replace the current escrow contract with a new version, the retrieval of tokens stored in the previous escrow contract becomes conditional on updating the governing (DAO) logic to incorporate a new function. This new function would permit token holders to directly execute withdrawals from the former escrow contract. Lacking this modification, token holders would be unable to access their escrowed tokens.

[Arabadzhiew]

Escalate

[sherlock-admin2]

Escalate

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[WangSecurity]

The main problem is in this sentence:

Assuming that administrators will act in good faith and not prematurely update the escrow contract before reaching the fork threshold underestimates the potential for human error or malicious actions.

Under NounsDAO contest's README, owners and admins are trusted to not act malicious or make a human error. Therefore, the main reason of invalidation is that the Owner/Admin is trusted to call _setForkEscrow correctly.

Second reason, the Owner/Admin, even if they do change forkEscrow contract and users cannot claim their tokens, can call _setForkEscrow and reset it to previous forkEscrow address and call _setForkEscrow again and set back to the new version.

[radeveth]

The main problem is in this sentence:

Assuming that administrators will act in good faith and not prematurely update the escrow contract before reaching the fork threshold underestimates the potential for human error or malicious actions.

Under NounsDAO contest's README, owners and admins are trusted to not act malicious or make a human error. Therefore, the main reason of invalidation is that the Owner/Admin is trusted to call _setForkEscrow correctly.

Second reason, the Owner/Admin, even if they do change forkEscrow contract and users cannot claim their tokens, can call _setForkEscrow and reset it to previous forkEscrow address and call _setForkEscrow again and set back to the new version.

My issue isn't about admin mistake. If during the escrow period ds.forkEscrow is changed by the proposal call to _setForkEscrow, then the user's escrowed Nouns will not be withdrawn by withdrawFromForkEscrow.

Imagine a scenario where certain Nouners cast their votes on a proposal intended to modify the ds.forkEscrow. In this scenario, numerous Nouns are already held in forkEscrow. Once the proposal passes and is implemented, resulting in an update to ds.forkEscrow, the Nouns previously escrowed become inaccessible for withdrawal.

[WangSecurity]

As I've said, in such scenario, admin can call _setForkEscrow back to the old forkEscrow, let the users withdraw their NFTs and reset forkEscrow to the new address.

[radeveth]

And if the admin notices or is notified, after some time (which is very likely due to the fact that the admin and users are expected to be non-technical people) that the users cannot withdraw their funds and change the forkEscrow to old one, the same scenario can exist for new Nouners that held their funds in a new forkEscrow. This is certainly a bottle neck in the protocol. Also, for the time that the new forkEscrow is applied, users funds will be locked (this is also a hundred percent unwanted behavior).

@WangSecurity

[WangSecurity]

I'm still of the opinion it should remain invalid. If hte DAO creates proposal to change the forkEscrow then users will see it and withdraw their tokens accordingly. In that case, if they don't see it or didn't do it in time, for me it seems user mistake since the proposal is known from its creation.

Moreover, again same thing. Owner/admin changes forkEscrow to old one, tells everyone to withdraw their token and then changes back to new one. If there are still people who didn't withdraw -> user mistake.

Therefore, I believe this report is correct, but only low/info.

And one little question, are you sure DAO can make a proposal to change forkEscrow. As I understand the only way to change it is inside NounsDAOAdmin::_setForkEscrow which is an admin contract, that can be called only by admins, not DAO.

[WangSecurity]

I mean can DAO change forkEscrow before forkThreshold is reached? Or only admins can do so? As I understand only admins can, and DAO can change it only after forkThreshold is reached which is intended behaviour.

[radeveth]

Hey, @WangSecurity! Your concerns relies primarily on the assumption that the protocol's governance mechanism and admin interventions can effectively prevent or mitigate any negative consequences arising from a change to the forkEscrow address. However, this perspective underestimates the risk and the impact on user token security.

1. The suggestion that the admin can simply revert the forkEscrow address to allow users to withdraw their tokens, and then update it again, overlooks the logistical and human error risks involved in managing such changes. This approach requires constant vigilance and swift action by the admin, which may not always be feasible. Furthermore, it imposes an undue burden on users to monitor and respond to these changes promptly, which is not always practical and increases the risk of errors.

2. The periods during which tokens are locked up due to a change in the forkEscrow address create unnecessary barriers and frustrations for users. This can deter participation in the DAO's governance processes, especially for less technical users who may not understand the implications of such changes. Moreover, the need to rely on admin intervention to rectify issues contradicts the decentralized ethos of DAOs, where user empowerment and system reliability should minimize reliance on central figures.

3. Let’s consider the contract code itself to clarify how the system handles changes to the forkEscrow:

function _setForkEscrow(address newForkEscrow) public onlyAdmin {
    emit ForkEscrowSet(address(ds().forkEscrow), newForkEscrow);
    ds().forkEscrow = INounsDAOForkEscrow(newForkEscrow);
}

Currently, if users wishes to reclaim their tokens from escrow, they must utilize a DAO function that subsequently interacts with the escrow. This escrow's withdrawal function is exclusively callable by the DAO. Should the DAO approve a proposal to replace the existing escrow contract with a new one, tokens residing in the former escrow can only be retrieved if the governance logic (DAO) integrates a new function that permits token owners to directly invoke the withdrawal function of the previous escrow. Without such a function, those token owners cannot access their tokens.

4. While you argues that DAO members should monitor and respond to proposals affecting critical operational parameters like forkEscrow, this places an unrealistic expectation on all members to continuously monitor and understand the implications of all governance actions. Not all users have the time or technical knowledge to do so, and missing such critical updates could lead to significant losses, as tokens could be locked or lost in old escrow addresses.

Finally, I think my comments above (1, 2) are still valid and refute your comments and show why this bug is valid.

Let the judge decide on this.

Cheers!

[WangSecurity]

Will answer point by point:

1. As I've said earlier, admins are tursted to not make such mistakes:

   the logistical and human error risks involved in managing such changes

2. This can be considered a DOS, but only a DOS over the week can be considered valid, but I don't see it.

3. No, as I've said we can go to previous admins set previous forkEscrow, retrieve the tokens, return to new forkEscrow. I know it's inconvenient, but I mean there's a solution for this problem, which is easy, but inconvenient and manual:

   tokens residing in the former escrow can only be retrieved if the governance logic (DAO) integrates a new function that permits token owners to directly invoke the withdrawal function of the previous escrow. Without such a function, those token owners cannot access their tokens

4. I believe this can be considered as user mistake:

   Not all users have the time or technical knowledge to do so, and missing such critical updates could lead to significant losses, as tokens could be locked or lost in old escrow addresses

[eladmallel]

this is a known issue from our previous audit, therefore we think it should remain excluded.

[radeveth]

known issues

[cvetanovv]

When the admin is specified as "Trusted" in the Readme, we assume that it will always act correctly and not make any mistakes.

This is according to the Sherlock documentation:

• "Admin incorrectly enters an input parameter." - not valid
• "Admin could have an incorrect call order." - not valid
• "An admin action can break certain assumptions about the functioning of the code." - not valid
• "Admin functions are assumed to be used properly"

The admin can warn users when an update is coming. If users miss this information it is their mistake.

I'm planning to reject the escalation and leave the issue as it is.

[Evert0x]

Result: Invalid Unique

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• Arabadzhiew: rejected