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: 4/17
Findings: 4
Award: $4,111.15
🌟 Selected for report: 5
🚀 Solo Findings: 1
xYrYuYx
https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/ovl/OverlayToken.sol#L268
https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/ovl/OverlayToken.sol#L194
In transferMint function, new tokens will be minted, but this does not increase total supply. In transferBurn function, some tokens will be burned, but did not decrease total supply.
Manual
Update _totalSupply in _transferMint and _transferBurn function
#0 - commercium-sys
2021-11-19T15:00:37Z
Replied to this in #13
#1 - mikeyrf
2021-12-06T23:44:14Z
duplicate #59
🌟 Selected for report: xYrYuYx
0.4973 ETH - $2,297.86
xYrYuYx
https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/ovl/OverlayToken.sol#L366
The burner could burn any amount of tokens of any user. This is not good solution of burn
Manual
Update burn function for only owner can burn his tokens. Now, ovl.burn function is used in OverlayV1OVLCollateral.sol file, and this updates won’t make any issue in protocol.
#0 - mikeyrf
2021-12-07T20:41:58Z
sponsor acknowledged reason - onlyBurner modifier with access control privileges prevent unexpected burn amounts, given only collateral managers are given burn permissions
🌟 Selected for report: xYrYuYx
xYrYuYx
In the constructor, you pushed initial value in positions array. But you used positionId_ as position.length - 1 after adding new data to positions. This will make two 0 index for different positions, and could make critical issue.
Manual
Change Line 197 to positionId_ = positions.length and make further updates which use positionId. Or Remove to push initial data to position array in constructor
#0 - mikeyrf
2021-12-07T21:13:21Z
sponsor disputed reason - governance would have to deploy the market, set appropriate permissions on the market, and have a collateral manager enter a position on the market in the same block for this edge case to occur. Even if this happens however, two 0 indexes wouldn't occur but the position would get the original 0 element price point
#1 - dmvt
2021-12-18T13:07:33Z
I'm going to leave this one in place as a low severity since it is technically possible for a wrong value to be used. This is a state handling issue at worst and does not represent any funds being at risk.
🌟 Selected for report: xYrYuYx
xYrYuYx
https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/market/OverlayV1Market.sol#L136 In the comment, you mentioned that update function is internal, but current update function is public function.
Manual
Review access of update function and change to internal if this is an issue.
#0 - commercium-sys
2021-12-07T21:13:42Z
It is completely benign for this to be triggered externally.
#1 - dmvt
2021-12-20T19:43:05Z
Changing to low:
From the judging criteria (emphasis mine):
1 — Low (L): vulns that have a risk of 1 are considered “Low” severity when assets are not at risk. Includes state handling, function incorrect as to spec, and issues with comments.
🌟 Selected for report: xYrYuYx
Also found by: Meta0xNull
0.0072 ETH - $33.42
xYrYuYx
https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/ovl/OverlayToken.sol#L411 https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/ovl/OverlayToken.sol#L417
In OverlayToken.sol, there are _beforeTokenTransfer and _afterTokenTransfer functions which do nothing. These functions are in OZ ERC20 token contracts and will be used to extend ERC20 transfer feature after inhert OZ ERC20 token. Current OverlayToken.sol does not inhert OZ ERC20, so those functions are not need.
Manual
Remove _beforeTokenTransfer and _afterTokenTransfer function and remove other functions which calls them.
#0 - mikeyrf
2021-12-07T20:48:10Z
sponsor acknowledged reason - only saves around 45 gas
xYrYuYx
https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/OverlayV1UniswapV3Market.sol#L90 https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/collateral/OverlayV1OVLCollateral.sol#L139 https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/market/OverlayV1Governance.sol#L54 https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/market/OverlayV1Governance.sol#L60 https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/market/OverlayV1Market.sol#L171
external functions use less gas than public functions.
Manual
Use external keyword instead of public keyword of functions which are not used internally.
#0 - mikeyrf
2021-12-06T23:57:31Z
duplicate #6 - public to external
#1 - dmvt
2021-12-21T14:14:46Z
#6 explanation by sponsor does not hold here. The first function uses variables in memory. Not a duplicate.
#2 - commercium-sys
2021-12-21T14:50:38Z
By a variable in memory I mean a parameter.
It could reduce the cost by having a parameter located in calldata instead of memory.
We don’t have any functions that do that.
On Tue, Dec 21, 2021 at 9:14 AM Dan Matthews @.***> wrote:
#6 https://github.com/code-423n4/2021-11-overlay-findings/issues/6 explanation by sponsor does not hold here. The first function uses variables in memory. Not a duplicate.
— Reply to this email directly, view it on GitHub https://github.com/code-423n4/2021-11-overlay-findings/issues/25#issuecomment-998813441, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADG5UGLRS2B3T2CIYIALYULUSCDWFANCNFSM5IMFPNMA . You are receiving this because you are subscribed to this thread.Message ID: @.***>
🌟 Selected for report: Meta0xNull
xYrYuYx
https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/OverlayV1UniswapV3Market.sol#L28
Please add zero address checker to avoid destroying protocol. It is good practice to check zero address before setting configuration.
Manual
Add zero checker before update configuration.
#0 - mikeyrf
2021-12-07T00:02:22Z
duplicate #32 - check zero address
🌟 Selected for report: harleythedog
xYrYuYx
https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/libraries/FixedPoint.sol#L39 https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/libraries/FixedPoint.sol#L32 https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/libraries/FixedPoint.sol#L46 https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/libraries/FixedPoint.sol#L53 https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/libraries/FixedPoint.sol#L69 https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/libraries/FixedPoint.sol#L75 https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/libraries/FixedPoint.sol#L82 https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/libraries/FixedPoint.sol#L88
From solc 0.8, all operations check underflow and overflow automatically. The FixedPoint library was written in solc 0.5, so SafeMath was required.
It looks like you missed to remove SafeMath things after updating to solc 0.8 If it removed, it can save gas.
Manual
Remove Safe Math checking lines
#0 - mikeyrf
2021-12-07T00:12:11Z
duplicate #48