Platform: Code4rena
Start Date: 16/11/2021
Pot Size: $50,000 ETH
Total HM: 11
Participants: 17
Period: 7 days
Judge: LSDan
Total Solo HM: 8
Id: 49
League: ETH
Rank: 7/17
Findings: 4
Award: $2,539.32
🌟 Selected for report: 2
🚀 Solo Findings: 1
🌟 Selected for report: harleythedog
0.4973 ETH - $2,297.86
harleythedog
The function isUnderwater should return true iff the position value is < 0. In the case of a short position, this is when oi * (2 - priceFrame) - debt < 0 (based on the logic given in the _value function). Rearranging this equation, a short position is underwater iff oi * 2 < oi * priceFrame + debt. However, in the function _isUnderwater in Position.sol, the left and right side of this equation is flipped, meaning that the function will return the opposite of what it should when called on short positions.
Fortunately, the V1 implementation of OverlayOVLCollateral does not directly use the isUnderwater function in major control flow changes. However, line 304 of OverlayV1OVLCollateral.sol is a comment that says:
// TODO: think through edge case of underwater position ... and fee adjustments ...
which hints that this function is going to be used to deal with underwater positions. As a result, this issue would have a huge impact if not properly dealt with.
See code for _isUnderwater here: https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/libraries/Position.sol#L70
Notice that for short positions the inequality is flipped from what it should be (indeed, when self.debt is higher it is more likely that isUnder will be false, which is obviously incorrect).
Also, see the TODO comment here that shows isUndewater is important: https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/collateral/OverlayV1OVLCollateral.sol#L304
Inspection
Flip the left and right side of the inequality for short positions in _isUnderwater.
#0 - mikeyrf
2021-12-07T21:39:37Z
disagree with severity - isUnderwater() isn't used anywhere in the collateral manager and markets. Is more for information purposes, so would rate this at a severity of 2 - Medium in the event we had actually used this function for something more important
#1 - dmvt
2021-12-18T12:35:46Z
I agree with the sponsor here. This represents a severe, but hypothetical issue.
harleythedog
The implementation of _transferBurn in ovl/OverlayToken.sol does not actually burn any tokens since _totalSupply is not decreased (see the implementation for _burn for reference of what should be done). The _transferBurn function is a helper function that is used in both transferBurn and transferFromBurn, both of which are used within other contracts frequently. This means that all calculations (either within this application or external to this application) based on OVL supply will be different than what it should be, which has far reaching and severe consequences as more transactions happen and the supply differs from expected.
See the code for _transferBurn here: https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/ovl/OverlayToken.sol#:~:text=function-,_transferBurn,-(
Inspection
Make sure to decrease _totalSupply in the _transferBurn function.
#0 - commercium-sys
2021-11-18T04:44:51Z
Replied regarding this in #13
#1 - mikeyrf
2021-12-06T23:42:16Z
duplicate #59
🌟 Selected for report: pants
Also found by: harleythedog
harleythedog
Within OverlayV1Mothership.sol, the variable marginBurnRate is set in the constructor and can not be set anywhere else. To optimize gas this variable could be declared as immutable.
See variable here: https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/mothership/OverlayV1Mothership.sol#:~:text=uint%20public%20marginBurnRate%3B
Manual inspection
Change variable to immutable.
#0 - mikeyrf
2021-12-07T00:22:54Z
duplicate #84
#1 - dmvt
2021-12-21T15:04:36Z
Duplicate of #95, not #84. Warden describes gas savings through the use of immutable without mentioning the missing setter function.
🌟 Selected for report: WatchPug
Also found by: defsec, harleythedog, ye0lde
harleythedog
Due to the require statement on line 64 of OverlayV1Market.sol, line 66 will never underflow, so you can wrap the line in unchecked {} to save some gas.
See line of code here: https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/market/OverlayV1Market.sol#:~:text=collateralAdjusted_%20%3D%20_collateral%20-%20_impact%20-%20fee_%3B
Inspection
Wrap line of code in unchecked {}
#0 - mikeyrf
2021-12-07T00:17:43Z
duplicate #56
🌟 Selected for report: harleythedog
0.0043 ETH - $20.05
harleythedog
There are a lot of integer overflow/underflow checks, for example in the FixedPoint library. In solidity 0.8, arithmetic operations automatically reverts (see solidity docs here: https://docs.soliditylang.org/en/v0.8.0/080-breaking-changes.html#:~:text=Arithmetic%20operations%20revert%20on%20underflow%20and%20overflow.%20You%20can%20use%20unchecked%20%7B%20...%20%7D%20to%20use%20the%20previous%20wrapping%20behaviour.) As a result, all of these integer overflow/underflow checks are unnecessarily eating up gas usage.
There are a lot of underflow/overflow checks in this library: https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/libraries/FixedPoint.sol
For example, the line:
require(aInflated / a == ONE, Errors.DIV_INTERNAL); // mul overflow
is useless since the previous line would have already reverted if there was an overflow.
Inspection
Remove unncessary require statements
#0 - mikeyrf
2021-12-07T19:28:37Z
sponsor disputed reason - libraries/FixedPoint.sol was not included in scope
#1 - dmvt
2021-12-21T14:16:29Z
As pointed out in a previous issue, FixedPoint is used by functions that are in scope. Gas savings here impact those functions, leaving this report in scope.