0xbrivan - If the admin extends the auction length when it is in the extension-window period, the auction will not be extended because of a wrong logic in `_auctionEndTime`

[sherlock-admin2]

0xbrivan

medium

If the admin extends the auction length when it is in the extension-window period, the auction will not be extended because of a wrong logic in _auctionEndTime

Summary

If the admin sets a new length of the auction when the auction is in an extension-window period, the auction will not be extended and the new length will not be applied.

Vulnerability Detail

The auction admin calls setAuctionLengthSeconds to set a new length for the auction, and similarly, extends it. The internal function _setAuctionLengthSeconds will be executed which updates the storage value.

function _setAuctionLengthSeconds(uint256 auctionLengthSeconds) internal {
    EnglishPeriodicAuctionStorage
        .layout()
        .auctionLengthSeconds = auctionLengthSeconds;

    emit AuctionLengthSet(auctionLengthSeconds);
}

There is an edge case where the new length will not be applied, it is when the auction admin calls setAuctionLengthSeconds when the auction is in the extension-window period, the _auctionEndTime function --that is used in the code to determine whether the auction has ended or not, will return true while it should return false. That is because the function will not read from the storage, but from the previously set currentAuctionLength when the auction entered the extension-window period:

// EnglishPeriodicAuctionInternal::_auctionEndTime

// previous function code
uint256 auctionLengthSeconds;
if (l.currentAuctionLength[tokenId] == 0) {
    auctionLengthSeconds = _auctionLengthSeconds(); // @audit new length set by admin will not be read when the auction is in extension period
} else {
    auctionLengthSeconds = l.currentAuctionLength[tokenId];
}

Proof of Concept

The following proof of concept illustrates the previously described issue. Place the following test in test/auction/EnglishPeriodicAuction.ts:

describe('setAuctionLengthSeconds', function () {
// ... other tests

it('PoC', async function () {
    // Auction start: Now - 200
    // Auction end: Now + 100
    const instance = await getInstance({
        auctionLengthSeconds: 300,
        initialPeriodStartTime: (await time.latest()) - 200,
        licensePeriod: 1000,
        hasOwner: true,
    });

    const bidAmount = ethers.utils.parseEther('1.0');
    const feeAmount = await instance.calculateFeeFromBid(bidAmount);
    const collateralAmount = feeAmount.add(bidAmount);

    await time.increase(91); // placing a bid just before the auction is ended, the auction will be extended for few seconds
    await instance
        .connect(bidder1)
        .placeBid(0, bidAmount, { value: collateralAmount });

    // Set Auction length to 500 , auction should end at Now + 209
    await instance.connect(owner).setAuctionLengthSeconds(500);

    await time.increase(20);

    expect(await instance.isReadyForTransfer(0)).to.be.equal(true); // New length is not applied

});
})

EnglishPeriodicAuction
    setAuctionLengthSeconds
      ✔ PoC (990ms)
  1 passing

Impact

As a consequence of this issue, the auction will not be extended as intended by the auction admin.

Code Snippet

https://github.com/sherlock-audit/2024-02-radicalxchange/blob/459dfbfa73f74ed3422e894f6ff5fe2bbed146dd/pco-art/contracts/auction/EnglishPeriodicAuctionInternal.sol#L368-L370 https://github.com/sherlock-audit/2024-02-radicalxchange/blob/459dfbfa73f74ed3422e894f6ff5fe2bbed146dd/pco-art/contracts/auction/EnglishPeriodicAuctionInternal.sol#L581-L586

Tool used

Manual Review

Recommendation

The root reason for this issue is a wrong check in EnglishPeriodicAuctionInternal::__auctionEndTime, below is a suggestion of an updated code:

function _auctionEndTime(
    uint256 tokenId
) internal view returns (uint256 auctionEndTime) {
    EnglishPeriodicAuctionStorage.Layout
        storage l = EnglishPeriodicAuctionStorage.layout();

-    uint256 auctionLengthSeconds;
+    uint256 auctionLengthSeconds = _auctionLengthSeconds();
-    if (l.currentAuctionLength[tokenId] == 0) {
-        auctionLengthSeconds = _auctionLengthSeconds();
-    } else {
-        auctionLengthSeconds = l.currentAuctionLength[tokenId];
-    }
+    if (l.currentAuctionLength[tokenId] != 0 && l.currentAuctionLength[tokenId] > auctionLengthSeconds) {
+        auctionLengthSeconds = l.currentAuctionLength[tokenId];
+    }

    auctionEndTime = _auctionStartTime(tokenId) + auctionLengthSeconds;
}

[Brivan-26]

Escalate I don't see why it is invalid, no comment was provided at all. Please, read the finding description, there is also a runnable PoC

[sherlock-admin2]

Escalate I don't see why it is invalid, no comment was provided at all. Please, read the finding description, there is also a runnable PoC

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

[gravenp]

The standard recommendation for owners/admins is not to change configuration while an auction is in progress (there are "worse" functional outcomes than the one outlined here--like an auction ending with no warning). Adding all of the necessary checks/logic was deemed overly complex, so by design, the owner/admins are trusted within the scope of their granted roles: https://github.com/sherlock-audit/2024-02-radicalxchange?tab=readme-ov-file#q-are-there-any-additional-protocol-roles-if-yes-please-explain-in-detail

[Brivan-26]

@gravenp

The standard recommendation for owners/admins is not to change configuration while an auction is in progress

I don't get it, if it is not intended to extend the length of the current auction, why there's no explicit check in the codebase on setAuctionLengthSeconds. Also, why there's a test in EnglishPeriodicAuction.ts (should change length of current auction)? which simply confirms that the current auction can be extended. It is also mentioned in the docs the possibility of extending an auction: Auction Duration (can be extended)

so by design, the owner/admins are trusted within the scope of their granted roles

This finding does not attempt to highlight a malicious action by an admin. It highlights a possible edge case of extending the auction, in which the action extension will not take place and the output will not be expected by the admin

[Brivan-26]

@Hash01011122 I need your input here, please.

[WangSecurity]

Escalate

I'm escalating on the behalf of @Brivan-26 and will leave to him, but a small input from me: as I understand the admin can extend the auction length after the extension-window period, then I think it might be low, cause to me it doesn't look as a core functionality break or financial loss. Also the rule about trsuted owners/admins cannot be applied here. But, want to highlight that I haven't looked into other issues or the code of radical, therefore, I'm leaving it to Brivan.

[sherlock-admin2]

Escalate

I'm escalating on the behalf of @Brivan-26 and will leave to him, but a small input from me: as I understand the admin can extend the auction length after the extension-window period, then I think it might be low, cause to me it doesn't look as a core functionality break or financial loss. Also the rule about trsuted owners/admins cannot be applied here. But, want to highlight that I haven't looked into other issues or the code of radical, therefore, I'm leaving it to Brivan.

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.

[Brivan-26]

I don't think it is low. An admin trying to extend the auction at its last minutes/seconds means it is an important decision, I can't know what reason could lead the admin to extend the auction at its last minutes, but if he did, it means it is something urgent. However, the auction will not be extended in such a situation as demonstrated by the PoC. So, low likelihood with high impact leads to Medium severity

[zzykxx]

There is a whole bunch of closed issues related to changing an auction parameters while the auction is ongoing.

@gravenp Is this "The standard recommendation for owners/admins is not to change configuration while an auction is in progress" stated anywhere in the docs, readme, discord or code? If this is not the case they should all be valid.

[Hash01011122]

As far as I have discussed with sponsors any issue which comes under change of configuration by admin is invalid as admin is trusted @gravenp any thoughts?

[Brivan-26]

@Hash01011122 @zzykxx Check the following unit test presented in the tests suite: should change the length of current auction It demonstrates that length parameter is intended to be changed in the current auction

[Brivan-26]

@Hash01011122

As far as I have discussed with sponsors any issue which comes under change of configuration by admin is invalid as admin is trusted

I'm saying this again, this finding does not attempt to demonstrate a malicious action by an admin or an action that can break the protocol. It highlights a possible edge case of extending the auction, in which the auction extension will not take place and the output will not be expected by the admin, as demonstrated by PoC. I strongly believe the rule about trsuted owners/admins cannot be applied here

[Al-Qa-qa]

I want to engage in this discussion.

As stated in the README.

Changes will take immediate effect including if an auction is in progress

So the changes made to auctions are subject to take effect even for running ones. This issue explains an edge case that will make the change (extending period) not apply for the auction. So I think it is valid.

[gravenp]

At the bottom of this section of the docs, we have a "buyer beware" info box about changing configuration: https://pco-art-docs.vercel.app/for-artists/admin-permissions#2-component-admins.

Regardless of the outcome of this issue and others, we will be beefing up the documentation/instructions that artists & admins should only attempt configuration changes during ongoing auctions during what they deem to be emergencies (e.g. security incidents) and where their powers are still limited. From a business & trust perspective, all would agree that it's best not to change the rules/parameters of an auction once it has already started.

Attempting to change the auction end time during the automatic auction extension period is an extreme version of a last-minute change that violates participant fairness assumptions. As I don't see a security angle for extending the auction in this scenario, it's not worth adding additional logic to enable it from my perspective. We want to prioritize simplicity where we can versus overcomplicating marginal judgment scenarios—that includes drawing the line somewhere on not enabling more judgment calls.

I'll leave it to the judges to address validity/severity per contest rules, but we will document this as a reasonable limitation to trusted admin powers rather than extending them.

[Brivan-26]

This will be my last comment, I can't say more than what I have already said

I think we all agree that extending the current auction is something that is intended and can happen based on the test written should change length of current auction and based on the following comment:

we will be beefing up the documentation/instructions that artists & admins should only attempt configuration changes during ongoing auctions during what they deem to be emergencies (e.g. security incidents) and where their powers are still limited.

One more thing, based on this comment specifically:

artists & admins should only attempt configuration changes during ongoing auctions during what they deem to be emergencies

I believe emergencies can happen also at last minutes of the auction. And if the admin wants to extend the auction length is such case, the auction will not be extended as demonstrated by the provided PoC

Leaving it to the judge.

[Hash01011122]

Well as said earlier this is indeed invalid issue even from sherlock's rule and also from README file of contest.

[Brivan-26]

@Hash01011122

from sherlock's rule and also from README file of contest.

Can you point me to the rules you are referring to? (Hope they are not related to TRUSTED rule, because it can not be applied here as I, @WangSecurity, @Al-Qa-qa already explained)

[Hash01011122]

Reevaluating the issue with a new perspective reveals it as a valid concern, previously overlooked due to uncertainty around administrative configuration changes during ongoing auctions. The project's documentation clearly states that auction durations can be extended, highlighting a misunderstanding in the initial assessment. This shifts the severity assessment to borderline low/medium. However, given that this issue likely does not impact the protocol or its users significantly, the inclination is towards classifying it as low severity. Do you want to add something here @Brivan-26?

[Brivan-26]

Thanks for giving it another look @Hash01011122 .

However, given that this issue likely does not impact the protocol or its users significantly, the inclination is towards classifying it as low severity

This issue has low likelihood with high impact, which is the reason I labeled it with Medium severity. Here is why I think so:

1. Low likelihood because for this issue to happen an admin needs to extend the auction at its last minutes.

2. High impact because in the first place, if an admin is trying to extend the auction at its last minute it is surely due to emergency situation (as confirmed by the sponsor), in which the issue submitted here demonstrates that the auction will not be extended as intended. So, the emergency situation is not resolved.

[gravenp]

Re: High Impact - "emergency situation (as confirmed by the sponsor)"

That's not an accurate representation of our position in this context. Our resolution to this issue will be to document that auctions cannot be manually extended via reconfiguration by an auction component admin during the extension-window period. In our view, the Owner/Collection Admin role is more appropriate and necessary for security-related updates that might rise to a high-level impact as they are the admin able to alter the diamond cuts/contract addresses. They wouldn't use this role or function to do so. Extending the auction can also be achieved by placing a bid.

[Brivan-26]

• You already said in the first part of your comment that you will document the fact that the admin is not intended to extend the auction at its last minutes. I understand that, but this was not available during the context

• the Owner/Collection Admin role is more appropriate and necessary for security-related updates

The context of this issue is not about security-related updates. Security updates require pausing/upgrading the contracts, not extending the auction.

• Extending the contract can also be achieved by placing a bid

So, the admin bypasses the presented edge case by placing a new bid? Does not make sense at all to me.

Anyway, this discussion has to end at some point. I do not wanna spend more time convincing more than what I already have done. I will not drop more comments unless requested. I will take the lead judge or head of judge decisions.

[Hash01011122]

As I said earlier it's borderline low/medium issue, but I am inclined towards low waiting for @St4rgarden and @Czar102 response for this one.

[zzykxx]

Just a reminder that there are a bunch of other issues related to wrong behavior on changing auction parameters while an auction is ongoing, if this will be considered valid those should as well.

[Evert0x]

An admin trying to extend the auction at its last minutes/seconds

Planning to reject the escalation as keep issue as is.

Medium severity doesn't apply to this issue as core functionality isn't broken. It's just not available for the extra second (or block) that the protocol intended.

[Brivan-26]

Hi @Evert0x , I don't understand the argument you stated:

It's just not available for the extra second (or block) that the protocol intended.

The new auction length doesn't have to be some extra seconds/blocks. It is provided by the admin. In case of emergencies, the duration can have a non-negligible value that doesn't fit into some extra seconds

[Evert0x]

Result: Invalid Unique

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• WangSecurity: rejected