Nouns Builder contest - sorrynotsorry's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L307 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L315 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L323

Vulnerability details

Impact

The Auction.sol has a set of functions that update the settings. However, these functions can be called by the owner even during the auction. And if the owner is somehow compromised, this vector can be utilized to manipulate the auction intentionally as well. If any settings are changed during an auction, the whole auction turns out to be an unfair game. One user's advantage would be another user's disadvantage and the auction loses its fairness along with the platform's trust score.

Proof of Concept

The 3 update setting function is as follows; 1.

307:     function setDuration(uint256 _duration) external onlyOwner {

Permalink 2.

315:     function setReservePrice(uint256 _reservePrice) external onlyOwner {

Permalink 3.

323:     function setTimeBuffer(uint256 _timeBuffer) external onlyOwner {

Permalink

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend to add whenPaused modifier or at least;

When No #1 function is called during an auction, it must be ensured that the duration should not be greater than the previous one. Else, it will be unfair for the user who is about to create a bid to be the highest bidder but couldn't make it. Also, it would be best for the last highest bidder who closed out the auction as a winner.

When No #2 function is called during an auction, it must be ensured that the reserve price should be greater than the previous price. Else, the previous bidder whose bid was rejected being under the reserve price will have the disadvantage of this. And the last user who has bid after the update has the advantageous position since the other user may have already left the platform due to lack of funds.

When No #3 function is called during an auction, it must be ensured that the time buffer is greater than the previous one. Else, the previous user who failed to bid due to the previous setting is in an unfair position.

QA (LOW & NON-CRITICAL)

[L-01] Missing SafeCast

Failing to safecast from a greater value type to a lesser one might cause unintended math troubles and there are missing safecast operations with the instances below;

141:  extend = (_auction.endTime - block.timestamp) < settings.timeBuffer; //Missing safe casting to uint40

Permalink

[L-02] No indexed events in Auction Contract

There are no indexed events in Auction.sol. This makes the project remain trackable only on one network.

[L-03] safeTransferFrom to be used instead of transferFrom

It's the user's obligation to prepare his/her smart contract to be able to receive NFT's if he/she participates in an auction via their custom contract. However, it would be safer to use safeTransferFrom for the project as well.

192:             token.transferFrom(address(this), _auction.highestBidder, _auction.tokenId);

Permalink

[L-04] Critical address changes

Changing critical addresses in contracts should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one (i.e. claims ownership). This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible. Reference, Reference Critical address changes are not done in two steps for the following instances:

250:             transferOwnership(settings.treasury);

Permalink-1

102:             transferOwnership(settings.treasury);

Permalink-2

It's safer to use safeTransferOwnership.

[L-05] Checking the success of transfer

The ERC20.transfer() and ERC20.transferFrom() functions return a boolean value indicating success. This parameter needs to be checked for success. It would be a best practice to wrap them in require statements. Return values of the transfers are not checked in the below instances;

363: IWETH(WETH).transfer(_to, _amount);

Permalink

[L-06] No Storage Gap for Base Contracts

For upgradeable contracts, there must be a storage gap to "allow developers to freely add new state variables in the future without compromising the storage compatibility with existing deployments". Otherwise, it may be very difficult to write new implementation code. Without a storage gap, the variable in the child contract might be overwritten by the upgraded base contract if new variables are added to the base contract. This could have unintended and very serious consequences to the child contracts. Reference Recommend adding an appropriate storage gap at the end of upgradeable contracts such as the below. Please reference OpenZeppelin upgradeable contract templates.

uint256[50] private __gap;

[L-07] Contract declared Interfaces

AuctionTypesV1.sol, TreasuryTypesV1.sol are declared as contracts. However, they don't have states and they should be declared as interfaces.

[L-08] Check contract existence

Manager.sol contract has registerUpgrade function which offers a new implementation upgrade. However, there is no check whether the offered address exists.

    function registerUpgrade(address _baseImpl, address _upgradeImpl) external onlyOwner {
        isUpgrade[_baseImpl][_upgradeImpl] = true;
        emit UpgradeRegistered(_baseImpl, _upgradeImpl);
    }

Permalink