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: 6/17
Findings: 5
Award: $2,670.28
🌟 Selected for report: 3
🚀 Solo Findings: 0
defsec
When ERC1155 tokens are minted, a callback is invoked on the receiver of those tokens, as required by the spec. When including the ERC1155Supply extension, total supply is not updated until after the callback, thus during the callback the reported total supply is lower than the real number of tokens in circulation.
If a system relies on accurately reported supply, an attacker may be able to mint tokens and invoke that system after receiving the token balance but before the supply is updated.
"https://github.com/code-423n4/2021-11-overlay/blob/main/brownie-config.yaml#L13"
Vulnerable version of "@openzeppelin/contracts": "4.3.2" is used.
Code Review
A fix is included in version 4.3.3 of @openzeppelin/contracts and @openzeppelin/contracts-upgradeable.
https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories/GHSA-wmpv-c2jp-j2xg
#0 - mikeyrf
2021-12-06T23:38:39Z
duplicate #127
defsec
During the dynamic test, The Burn and Mint function does not increase/decrease total supply. That will cause liquidity loss on the protocol.
"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"
Code Review
Ensure that totalSupply is adjusted according to mint/burn functions.
#0 - mikeyrf
2021-12-06T23:45:08Z
duplicate #59
defsec
In the adjustGlobalParams function on line 1603of "https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/mothership/OverlayV1Mothership.sol#L1630", adjustGlobalParams function does not have any upper or lower bounds. Values that are too large will lead to reversions in several critical functions.
None
Consider to define upper and lower bounds on the adjustGlobalParams function.
#0 - dmvt
2021-12-18T12:29:50Z
Several wardens have marked this issue as high severity due to the potential for governance to rug users. Several have marked it as low risk because it's really just a bounding issue and presumably governance would not willingly choose to rug their users.
I view this a medium severity issue. If exploited, the impact would be high. The likelihood that it would be exploited intentionally or happen unintentionally is low, but not impossible as the uninformed users dynamic could come into play here.
defsec
On the code review, GUARDIAN ROLE has not been used on the contract.
modifier onlyGuardian () { require(hasRole(GUARDIAN, msg.sender), "OVLV1:!guard"); _; }
None
Delete unused modifier from the code base.
#0 - mikeyrf
2021-12-07T00:30:12Z
duplicate #122 - dead code to be removed
defsec
There are a few functions declared as public that are never called internally within the contract. It is best practice to mark such functions as external instead, as this saves gas (especially in the case where the function takes arguments, as external functions can read arguments directly from calldata instead of having to allocate memory).
Code Review
All of the public functions in the contract are not called internally, so access can be changed to external to reduce gas.
#0 - commercium-sys
2021-11-24T18:05:06Z
Agreed, I would put this under the final housekeeping stage that we were going to do.
#1 - mikeyrf
2021-12-06T23:58:50Z
duplicate #6 - public to external
#2 - dmvt
2021-12-21T14:45:53Z
duplicate of #25, not #6
🌟 Selected for report: WatchPug
Also found by: defsec, harleythedog, ye0lde
defsec
For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.
https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/market/OverlayV1OI.sol#L103
if (_funded == 0) { uint _oiNow = _fundingFactor.mulDown(_funder); fundingPaid_ = int(_funder - _oiNow); _funder = _oiNow; } else { // TODO: we can make an unsafe mul function here uint256 _oiImbNow = _fundingFactor.mulDown(_funder - _funded); uint256 _total = _funder + _funded; fundingPaid_ = int( ( _funder - _funded ) / 2 ); _funder = ( _total + _oiImbNow ) / 2; _funded = ( _total - _oiImbNow ) / 2; }
None
Consider applying unchecked arithmetic where overflow/underflow is not possible.
#0 - mikeyrf
2021-12-07T00:18:12Z
duplicate #56
defsec
Some storage variables are only set once and never changed.
Changing them to constant can save gas.
The following variable can mark as constant.
Code Review
Consider changing to constant and using all caps for the names.
#0 - commercium-sys
2021-11-24T18:04:35Z
Indeed, I think this would have been changed when we did final house cleaning and preparations for actual deployment.
#1 - mikeyrf
2021-12-07T00:24:43Z
duplicate #85
0.0072 ETH - $33.42
defsec
That would Increase gas costs on all privileged operations.
The following role variables are marked as constant.
https://github.com/code-423n4/2021-11-overlay/blob/914bed22f190ebe7088194453bab08c424c3f70c/contracts/collateral/OverlayV1OVLCollateral.sol#L21 https://github.com/code-423n4/2021-11-overlay/blob/914bed22f190ebe7088194453bab08c424c3f70c/contracts/OverlayToken.sol#L9 https://github.com/code-423n4/2021-11-overlay/blob/914bed22f190ebe7088194453bab08c424c3f70c/contracts/ovl/OverlayToken.sol#L17 https://github.com/code-423n4/2021-11-overlay/blob/914bed22f190ebe7088194453bab08c424c3f70c/contracts/market/OverlayV1Governance.sol#L18
This results in the keccak operation being performed whenever the variable is used, increasing gas costs relative to just storing the output hash. Changing to immutable will only perform hashing on contract deployment which will save gas.
See: ethereum/solidity#9232 (https://github.com/ethereum/solidity/issues/9232#issuecomment-646131646)
Code Review
Consider to change the variable to be immutable rather than constant.
#0 - commercium-sys
2021-11-24T18:05:34Z
Interesting I did not know this one. Seems like a minor gas improvement.
🌟 Selected for report: defsec
0.0161 ETH - $74.28
defsec
!= 0 is a cheaper operation compared to > 0, when dealing with uint.
https://github.com/code-423n4/2021-11-overlay/blob/914bed22f190ebe7088194453bab08c424c3f70c/contracts/libraries/LogExpMath.sol#L321 https://github.com/code-423n4/2021-11-overlay/blob/914bed22f190ebe7088194453bab08c424c3f70c/contracts/libraries/UniswapV3OracleLibrary/FullMath.sol#L34 https://github.com/code-423n4/2021-11-overlay/blob/914bed22f190ebe7088194453bab08c424c3f70c/contracts/libraries/UniswapV3OracleLibrary/FullMath.sol#L119
None
Consider to replace > 0 with != 0 for gas optimization.
#0 - commercium-sys
2021-11-24T18:06:18Z
Nice. Seems like a very small gas improvement.
#1 - mikeyrf
2021-12-08T22:31:39Z
sponsor disputed reason - mentioned libraries are out of scope
#2 - dmvt
2021-12-21T14:49:14Z
FullMath.mulDiv is used by functions that are in scope. Report would save gas for in scope functions so is not out of scope.