Forgotten Runes Warrior Guild contest - pedroais's results

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L608

Vulnerability details

Impact

User ETH could be stolen by the owner

Why this matters

In the documentation of the contest, the sponsor writes this as a known issue but I'm still reporting it since I think I can propose a solution that may be of value. Even if the sponsor is a legally doxed trusted entity a better trustlessness could be achieved with this solution. Being legally doxed in the US doesn't ensure everyone since a foreign user may not be able to easily litigate in the US.

Even if the sponsor does everything in good faith, enforcing max trustlessness can give more trust to the user which can increase the value of the sale and NFTs.

Proof Of Concept

The owner is able to withdraw all eth including the funds owed for refunds. If the owner withdraws before all refunds are made user's will lose their refunds.

Recommended Mitigation Steps

Create a function that computes all owed eth and make the withdraw function only able to withdraw balance - owedETH.

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L564 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L571 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L557 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L571

Vulnerability details

Impact

The price for the dutch auction could be altered.

Proof of Concept

I previously sent an issue about start time being settable more than once. This also happens for many other variables. Since they are many I will send them all in a single issue.

The following functions should only be called once to ensure trustlessness and integrity of the dutch auction : setDaPriceCurveLength setStartPrice setLowestPrice setDaDropInterval

The price in the dutch auction is computed by this formula : uint256 dropPerStep = (startPrice - lowestPrice) / (daPriceCurveLength / daDropInterval);

By making dapriceCurveLength or daDropInterval equal to 0 the owner could stop the auction. This could benefit the owner since the price lowers with time and everyone pays the final lower price. If the auction does well at the beginning the owner could stop the auction to stop the price from being lower. This works against the integrity of the dutch auction.

Also changing the Start Price or the Lowest price in the middle of the auction could allow the owner to manipulate the price.

Recommended Mitigation Steps

To each of these setter functions add require (variable == 0) to ensure they are set once in a permanent way. Also, the Lowest price < startPrice should be required.

Forgotten runes warrior guild came to C4 to sponsor a contest on their NFT token and mint. The code is easy to understand thanks to full natspec documentation. The codebase lacks some good practices in terms of trustlessness and decentralization. Even if the company is trustable too much depended on owner keys can be dangerous in case a key theft happens.

These are the low severity and informational issues : Low severity : L1- If the dutchAuction sells out before reaching lowestPrice the view function currentDaPrice will still return the lowest price. If the auction sells out the function should return the final price instead of lowestPrice. https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L279

Informational : I1-The initializer function lacks the initializer modifier. https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L52

I2- Using send for eth transfers. This method will gas bound transfers which means a receiving contract can't implement any fallback logic. https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L164

I3- The minter contract doesn't have any events which is a bad practice https://consensys.github.io/smart-contract-best-practices/development-recommendations/solidity-specific/event-monitoring/