cawfree - The incumbent steward can avoid paying the periodic honorarium.

[sherlock-admin3]

cawfree

high

The incumbent steward can avoid paying the periodic honorarium.

Summary

Incumbent stewards, as the current owner of the token under auction, are subject to a unique auction mechanism where the collateral value of their placed bids contribute only towards paying the periodic honorarium, as this eliminates the admittedly redundant process of paying themselves.

However, this execution path can be manipulated by the steward by transferring the token in the time period between winning the auction and settling it, enabling the incumbent steward to win auctions risk free and even turn an attacker-controlled profit in proportion to the periodic honorarium.

Vulnerability Detail

Firstly, we need to emphasise that whenever an incumbent steward makes a bid via placeBid(uint256,address,uint256,uint256), they must specify a value of bidAmount and collateralAmount which will ensure that a precise feeAmount is paid.

Let's quickly run through how this works.

For an incumbent steward, their computed feeAmount is hardcoded to the totalCollateralAmount:

if (bidder == currentBidder) {
  // If current bidder, collateral is entire fee amount
  feeAmount = totalCollateralAmount;
}

To ensure the transaction doesn't revert with EnglishPeriodicAuction: Incorrect bid amount, the incumbent steward call is expected provide an equivalent bidAmount for the collateral value that will guarantee the call to _checkBidAmount(uint256, uint256) will satisfy the following strict equality check:

/**
 * @notice Check that fee is within rounding error of bid amount
 */
function _checkBidAmount(
    uint256 bidAmount,
    uint256 feeAmount
) internal view returns (bool) {
    uint256 calculatedFeeAmount = _calculateFeeFromBid(bidAmount);

    return calculatedFeeAmount == feeAmount;
}

Here, the feeAmount (which we now know is hard-wired to the totalCollateralAmount) must equal the result of running _calculateFeeFromBid(uint256) on the caller-supplied bidAmount value:

/**
 * @notice Calculate fee from bid
 */
function _calculateFeeFromBid(
    uint256 bidAmount
) internal view returns (uint256) {
    uint256 feeNumerator = IPeriodicPCOParamsReadable(address(this))
        .feeNumerator();
    uint256 feeDenominator = IPeriodicPCOParamsReadable(address(this))
        .feeDenominator();

    return (bidAmount * feeNumerator) / feeDenominator;
}

For any auction which intends on collecting fees on behalf of the creator circle*, the bidAmount specified by the caller must be artificially inflated relative to the collateralAmount in order for the strict equality check to be satisfied.

due to low severity implementation quirks this applies to all* auctions

The Exploit

With that initial context out of the way, let's get down to the main issue.

Let's assume that the incumbent steward intends to submit a collateralAmount of 1 ether and the periodic honorarium is configured to 10%. This will necessitate a corresponding bidAmount of 1.1 ether, and save a Bid as follows:

bid.bidder = incumbentStewardAddress;
bid.bidAmount = 1.1 ether;
bid.feeAmount = 1 ether;
bid.collateralAmount = 1 ether;

Let's fast forward to the end of the auction, and the incumbent steward is the winner.

Before settling the auction via the call to _closeAuction(uint256), the incumbent steward transfers the token to another incumbent-controlled address, 0xdeadbeef.

Let's step through what happens.

1. Firstly, the oldBidder variable is assigned to 0xdeadbeef, and not the incumbent steward themselves.

address oldBidder;

if (IStewardLicense(address(this)).exists(tokenId)) {
  oldBidder = IStewardLicense(address(this)).ownerOf(tokenId); /// 0xdeadbeef
}

2. Next, because the incumbent steward is not 0xdeadbeef, we execute the following logic:

if (l.highestBids[tokenId][currentAuctionRound].bidder != oldBidder) {
  // Transfer bid to previous bidder's collateral
@> l.availableCollateral[oldBidder] += l.highestBids[tokenId][currentAuctionRound].bidAmount;
   l.highestBids[tokenId][currentAuctionRound].collateralAmount = 0;
   l.bids[tokenId][currentAuctionRound][
     l.highestBids[tokenId][currentAuctionRound].bidder
   ].collateralAmount = 0;
}

Here, we demonstrably transfer 1.1 ether of bidAmount to 0xdeadbeef, the incumbent-controlled address.

Remember, we initially only deposited 1 ether!

3. Next, we give the incumbent steward back control of the auctioned token by transferring it back from 0xdeadbeef:

// Transfer to highest bidder
IStewardLicense(address(this)).triggerTransfer(
  oldBidder, /// @audit 0xdeadbeef
  l.highestBids[tokenId][currentAuctionRound].bidder, // @audit incumbant_steward
  tokenId
);

4. Finally, we distribute the feeAmount to the beneficiary - that's right, everyone gets paid! 🤑

// Distribute fee to beneficiary
if (l.highestBids[tokenId][currentAuctionRound].feeAmount > 0) {
  IBeneficiary(address(this)).distribute{
    value: l.highestBids[tokenId][currentAuctionRound].feeAmount
}();
}

The incumbent steward gets to hold onto their token, and make a profit of 0.1 ether for the trouble.

Impact

This exploit enables the incumbent steward to permanently hold the asset risk-free and make profit on the asset in proportion to the periodic honorarium fee on their totalCollateralAmount, an attacker-controlled unit, at the expense of honest bidders.

High.

Code Snippet

📄 EnglishPeriodicAuctionInternal.sol

Tool used

Manual Review

Recommendation

Don't give the incumbent steward a privileged path of execution.

[utkuerkin]

Escalate Report states: "Before settling the auction via the call to _closeAuction(uint256), the incumbent steward transfers the token to another incumbent-controlled address, 0xdeadbeef." However during the auction token transfer is locked and this vulnerability is not possible. Token transfer before an auction makes this way too unlikely and this should be invalidated.

[sherlock-admin2]

Escalate Report states: "Before settling the auction via the call to _closeAuction(uint256), the incumbent steward transfers the token to another incumbent-controlled address, 0xdeadbeef." However during the auction token transfer is locked and this vulnerability is not possible. Token transfer before an auction makes this way too unlikely and this should be invalidated.

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.

[Aamirusmani1552]

Escalate

Invalid. The transfer isn't possible as _beforeTokenTransfer() locks the token after the auction start.

[sherlock-admin2]

Escalate

Invalid. The transfer isn't possible as _beforeTokenTransfer() locks the token after the auction start.

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.

[cawfree]

@Aamirusmani1552 @utkuerkin

The token is eligible for transfer after the auction has ended.

The issue states that the malicious auction winner transfers the token to an address under their control prior to closing out the finished auction, not whilst the auction is ongoing.

[liam-thunder]

@Aamirusmani1552 @utkuerkin

The token is eligible for transfer after the auction has ended.

The issue states that the malicious auction winner transfers the token to an address under their control prior to closing out the finished auction, not whilst the auction is ongoing.

Still don't get the idea about how malicious auction winner could transfer the token after auction has ended. The isAuctionPeriod check inside _beforeTokenTransfer() is all about block.timestamp >= _auctionStartTime(tokenId), while the way to change the return of _auctionStartTime is to call _closeAuction. So how could malicious auction winner transfer the token before call _closeAuction?

[ulasacikel]

The token is eligible for transfer after the auction has ended.

I don't think this is true. So if I understand correctly, you are saying we can transfer the token after the auction has ended and before the closeAuction is called.

In order for token transfer the take place, isAuctionPeriod should be false. Code:

 bool isAuctionPeriod = IPeriodicAuctionReadable(address(this))
                .isAuctionPeriod(tokenId);
            require(
                !isAuctionPeriod,
                'StewardLicenseFacet: Cannot transfer during auction period'
            );

Lets see the condition for isAuctionPeriod() to return false. Code:

 return block.timestamp >= _auctionStartTime(tokenId);

Above expression only returns false if _auctionStartTime is greater than block.timestamp. Lets see how _auctionStartTime can be greater than block.timestamp. Below is how the auctionStartTime is calculated. Code:

 auctionStartTime = l.lastPeriodEndTime[tokenId] + l.currentLicensePeriod[tokenId]; 

In order for auctionStartTime to be greater than block.timestamp, the token should be in the license period. However, if the token is in license period, we cannot place a bid. So it's not possible to transfer the token before you call closeAuction.

[cawfree]

The token is eligible for transfer after the auction has ended.

I don't think this is true. So if I understand correctly, you are saying we can transfer the token after the auction has ended and before the closeAuction is called.

In order for token transfer the take place, isAuctionPeriod should be false. Code:

 bool isAuctionPeriod = IPeriodicAuctionReadable(address(this))
                .isAuctionPeriod(tokenId);
            require(
                !isAuctionPeriod,
                'StewardLicenseFacet: Cannot transfer during auction period'
            );

Lets see the condition for isAuctionPeriod() to return false. Code:

return block.timestamp >= _auctionStartTime(tokenId);

Above expression only returns false if _auctionStartTime is greater than block.timestamp. Lets see how _auctionStartTime can be greater than block.timestamp. Below is how the auctionStartTime is calculated. Code:

 auctionStartTime = l.lastPeriodEndTime[tokenId] + l.currentLicensePeriod[tokenId]; 

In order for auctionStartTime to be greater than block.timestamp, the token should be in the license period. However, if the token is in license period, we cannot place a bid. So it's not possible to transfer the token before you call closeAuction.

Agreed, I don't think this issue is valid. Back to writing PoCs I think. 😜

[cawfree]

I believe #33 should be de-duplicated from this issue as it does describe a valid edge case where a token can be transferred during auction.

[St4rgarden]

Yeah looks like this one is a non-issue. _beforeTokenTransfer hook prevents transfer of the NFT anytime after the auction has started; and the next auction start time is set when the auction is closed.

Yup #33 can be de-duplicated.

[Hash01011122]

Considering the consensus among sponsors to remove this issue’s association with #33, and with the watson’s concurrence on its invalidation, it stands to reason that this issue should be deemed invalid.

[Czar102]

Planning to invalidate this issue and duplicates, without #33, which will be considered separately.

[Czar102]

Result: Invalid Unique

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• Aamirusmani1552: accepted