search

code-423n4 / 2022-07-yield-findings

0 stars 0 forks source link

QA Report #48

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Summary

Low Risk Issues

Issue Instances
[L‑01] setLine() parameters inconsistently followed 1
[L‑02] Events will contain the wrong timestamp in the future 1
[L‑03] Integer overflow due to casting will cause contract accounting to break 1

Total: 3 instances over 3 issues

Non-critical Issues

Issue Instances
[N‑01] Incomplete documentation 1
[N‑02] Timestamps in events are redundant 1
[N‑03] constants should be defined rather than using magic numbers 8
[N‑04] Redundant cast 2
[N‑05] Typos 6
[N‑06] NatSpec is incomplete 1
[N‑07] Event is missing indexed fields 6
[N‑08] Duplicated require()/revert() checks should be refactored to a modifier or function 2

Total: 27 instances over 8 issues

Low Risk Issues

[L‑01] setLine() parameters inconsistently followed

In setLine() there's a require() that ensures that the initial offer is either equal to zero, or greater than 1%. It stands to reason that therefore, offers less than 1% are considered dust and are not actionable. If this is the case, then in the else-block on line 589, does not follow this same dust-skipping logic when initialProportion is set to zero, and will waste time offering proportions that nobody will take. This may lead to auctions taking longer than necessary, and more funds being lost. It is incorrect state handling and therefore of Low risk. If there was some other meaning behind the conditions, there should be a require() enforcing it

There is 1 instance of this issue:

File: contracts/Witch.sol

584          if (duration == type(uint32).max) {     // Interpreted as infinite duration
585              proportionNow = initialProportion;
586          } else if (elapsed > duration) {
587              proportionNow = 1e18;
588          } else {
589              proportionNow =
590                  uint256(initialProportion) +
591                  uint256(1e18 - initialProportion).wmul(elapsed.wdiv(duration));
592:         }

https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L584-L592

[L‑02] Events will contain the wrong timestamp in the future

While the timestamp field in the event might not affect on-chain processing, it will impact off-chain tools that have to parse it. This is incorrect state handling and therefore of Low risk

There is 1 instance of this issue:

File: contracts/Witch.sol

217:         emit Auctioned(vaultId, uint32(block.timestamp));

https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L217

[L‑03] Integer overflow due to casting will cause contract accounting to break

When block.timestamp becomes larger than type(uint32).max, the cast on line 582 will overflow, causing the elapsed time calculation to be extremely large and wrong if the auction start time was before the wrap. This will cause the proportion to be greater than 100%, and will allow a liquidator to earn a massive fee. Comments in code are not sufficient to prevent client fund loss, and relying on Google calendar is obviously not either. This should be a Medium, but I'm guessing the sponsor will argue that it's already documented here in the code (though it needs to be in the README.md and in Yield's risks documentation), so it's not worth while to write up the whole thing just to have it downgraded by a judge that decides not to follow that rule.

There is 1 instance of this issue:

File: contracts/Witch.sol

575          // If the world has not turned to ashes and darkness, auctions will malfunction on
576          // the 7th of February 2106, at 06:28:16 GMT
577          // TODO: Replace this contract before then 😰
578          // UPDATE: Added reminder to Google calendar ✅
579          uint256 elapsed;
580          uint256 proportionNow;
581          unchecked {
582              elapsed = uint32(block.timestamp) - uint256(auction_.start); // Overflow on block.timestamp is fine
583:         }

https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L575-L583

Non-critical Issues

[N‑01] Incomplete documentation

The infinite duration comment should be in NatSpec, not a normal comment hidden in the code

There is 1 instance of this issue:

File: contracts/Witch.sol

584:         if (duration == type(uint32).max) {     // Interpreted as infinite duration

https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L584

[N‑02] Timestamps in events are redundant

block.timestamp and block.number are added to event information by default so adding them manually wastes gas and is redundant

There is 1 instance of this issue:

File: contracts/Witch.sol

217:         emit Auctioned(vaultId, uint32(block.timestamp));

https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L217

[N‑03] constants should be defined rather than using magic numbers

Even assembly can benefit from using readable constants instead of hex/numeric literals

There are 8 instances of this issue:

File: contracts/Witch.sol

/// @audit 1e18
102:          require(initialOffer <= 1e18, "InitialOffer above 100%");

/// @audit 1e18
103:          require(proportion <= 1e18, "Proportion above 100%");

/// @audit 0.01e18
105:              initialOffer == 0 || initialOffer >= 0.01e18,

/// @audit 0.01e18
108:          require(proportion >= 0.01e18, "Proportion below 1%");

/// @audit 1e18
162:          if (auctioneerReward_ > 1e18) {

/// @audit 1e18
163:              revert AuctioneerRewardTooHigh(1e18, auctioneerReward_);

/// @audit 1e18
587:              proportionNow = 1e18;

/// @audit 1e18
591:                  uint256(1e18 - initialProportion).wmul(elapsed.wdiv(duration));

https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L102

[N‑04] Redundant cast

The type of the variable is the same as the type to which the variable is being cast

There are 2 instances of this issue:

File: contracts/Witch.sol

/// @audit uint256(initialProportion)
590:                  uint256(initialProportion) +

/// @audit uint256(artIn)
594:          uint256 inkAtEnd = uint256(artIn).wdiv(auction_.art).wmul(auction_.ink);

https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L590

[N‑05] Typos

There are 6 instances of this issue:

File: contracts/Witch.sol

/// @audit overriden
213:      /// Useful as a method so it can be overriden by specialised witches that may need to do extra accounting or notify 3rd parties

/// @audit repayed
220:      /// @dev Calculates the auction initial values, the 2 non-trivial values are how much art must be repayed

/// @audit overriden
267:      /// Useful as a method so it can be overriden by specialised witches that may need to do extra accounting or notify 3rd parties

/// @audit differente
385:      /// @dev transfers funds from the ilkJoin to the liquidator (and potentially the auctioneer if they're differente people)

/// @audit overriden
462:      /// Useful as a method so it can be overriden by specialised witches that may need to do extra accounting or notify 3rd parties

/// @audit quoutes
520:      /// @dev quoutes hoy much ink a liquidator is expected to get if it repays an `artIn` amount

https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L213

[N‑06] NatSpec is incomplete

There is 1 instance of this issue:

File: contracts/Witch.sol

/// @audit Missing: '@return'
174       /// @param vaultId Id of vault to liquidate
175       /// @param to Receiver of the auctioneer reward
176       function auction(bytes12 vaultId, address to)
177           external
178:          returns (DataTypes.Auction memory auction_)

https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L174-L178

[N‑07] Event is missing indexed fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (threefields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question

There are 6 instances of this issue:

File: contracts/Witch.sol

33        event Bought(
34            bytes12 indexed vaultId,
35            address indexed buyer,
36            uint256 ink,
37            uint256 art
38:       );

43        event LineSet(
44            bytes6 indexed ilkId,
45            bytes6 indexed baseId,
46            uint32 duration,
47            uint64 proportion,
48            uint64 initialOffer
49:       );

50:       event LimitSet(bytes6 indexed ilkId, bytes6 indexed baseId, uint128 max);

51:       event AnotherWitchSet(address indexed value, bool isWitch);

52        event IgnoredPairSet(
53            bytes6 indexed ilkId,
54            bytes6 indexed baseId,
55            bool ignore
56:       );

57:       event AuctioneerRewardSet(uint128 auctioneerReward);

https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L33-L38

[N‑08] Duplicated require()/revert() checks should be refactored to a modifier or function

The compiler will inline the function, which will avoid JUMP instructions usually associated with functions

There are 2 instances of this issue:

File: contracts/Witch.sol

300:          require(auction_.start > 0, "Vault not under auction");

365:          require(liquidatorCut >= minInkOut, "Not enough bought");

https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L300

alcueca commented 2 years ago

Pretty decent QA report