BiasedMerc - DAO updating forkEscrow before forkThreshold is reaches causes user's escrowed Nouns to be lost

[sherlock-admin2]

BiasedMerc

medium

DAO updating forkEscrow before forkThreshold is reaches causes user's escrowed Nouns to be lost

Summary

If the DAO changes the ds().forkEscrow using NounsDAOAdmin._setForkEscrow() this will cause NounsDAOFork.withdrawFromForkEscrow() to call the new forkEscrow locking all Nouns in the previous forkEscrow.

Vulnerability Detail

During the escrow period, users can deposit, NounsDAOFork.escrowToFork(), or withdraw, NounsDAOFork.withdrawFromForkEscrow() their nouns. The DAO can also pass proposals during the escrow period, which means that NounsDAOAdmin._setForkEscrow() can be used to change ds().forkEscrow.

NounsDAOLogicV4.sol#L571-L573

    function withdrawFromForkEscrow(uint256[] calldata tokenIds) external {
        ds.withdrawFromForkEscrow(tokenIds);
    }

NounsDAOFork.withdrawFromForkEscrow() is called from NounsDAOLogicV4.withdrawFromForkEscrow(), meaning users are not able to pass in their own ds value and can only use the one that is currently set, which as mentioned can be changed with a proposal during the escrow period.

NounsDAOFork.sol#L95-L102

    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);
    }

NounsDAOFork.withdrawFromForkEscrow() calls forkEscrow.returnTokensToOwner() for ds.forkEscrow and returnTokensToOwner can only be called by the DAO.

NounsDAOForkEscrow.sol#L116-L125

    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#L473-L477

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

        ds().forkEscrow = INounsDAOForkEscrow(newForkEscrow);
    }

Meaning that if ds().forkEscrow is changed by the DAO using NounsDAOAdmin._setForkEscrow() then users will be unable to unescrow their Nouns as they will be unable to call the returnTokensToOwner() function themselves and withdrawFromForkEscrow will now interact with the new ds().forkEscrow`. This finding was reported previously during the code4rena audit in July 2023, where it was acknowledged but has not been resolved.

Impact

Users may lose their Nouns if ds().forkEscrow is changed by a DAO proposal. This change would prevent withdrawal as returnTokensToOwner() can only be called by the DAO. If ds().forkEscrow is changed, the DAO cannot call it using withdrawFromForkEscrow.

Code Snippet

NounsDAOLogicV4.sol#L571-L573

NounsDAOFork.sol#L95-L102

NounsDAOAdmin.sol#L473-L477

Tool used

Manual Review

Recommendation

One solution would be to store previous escrows within NounsDAOLogicV4.sol and create a sister function similar to NounsDAOLogicV4.withdrawFromForkEscrow() but allowing users to choose an older escrow to withdraw their Nouns from.

Another solution would be to allow users to call forkEscrow.returnTokensToOwner() directly by changing the function modifier whilst also conducting the isForkPeriodActive(ds)) check. Although this file is out of scope, the issue involves NounsDAOAdmin.sol, NounsDAOFork.sol and NounsDAOLogicV4.sol, which are within the scope of this audit.

[sherlock-admin3]

2 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

takarez commented:

this seem to be an admin functionlity.

[eladmallel]

As discussed on discord, this is a known issue from a previous audit and we're not planning to fix at the moment, thank you!

[WangSecurity]

I have to validate this report due to sherlock's rules.

Such issues are invalid only if they're accepted in the README file of the contest or it's an update contest on Sherlock and this issue was found in the previous contest on Sherlock and tagged as won't fix.

Therefore, I have to mark it as valid, even tho it was found in C4 contest. And I've consulted with the head of judging regarding it and he confirmed.

[dmitriia]

As I can see, the direct link to C4 contest is included in this contest page. All acknowledged issues from the previous audits are excluded by default. I don’t see any point for including this one, absolutely zero grounds for an exception.

[WangSecurity]

As I've said earlier, Czar said that in that case invalid issues should be only if they are in the accepted issues section (this one is not there and the report from C4 is not there as well) and if such issues were found in the previous Sherlock contests and was labelled with "won't fix" (which means this contest should be an update contest which is also not the case here). Therefore, it's judged as Med

[dmitriia]

Any acknowledged issues from the audits explicitly listed in the previous audits sections should be included in the accepted issues by default. Accepted issues section is for additional issues that team is aware of and acknowledges in advance.

[WangSecurity]

I've consulted with the Head of Judging and he confirmed there is no rule forbidding reporting of acknowledged issues from past audits (if they're not done by Sherlock). But here's another problem. Firstly, the owner/admin is trusted to do this. Moreover, we can set bacj to the previous forkEscrow and users will be able to withdraw their assets. It is invalid, sorry for the confusion caused here, it should've remained invalid from the start.