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: 5/17
Findings: 3
Award: $3,191.56
🌟 Selected for report: 4
🚀 Solo Findings: 1
🌟 Selected for report: hyh
0.4973 ETH - $2,297.86
hyh
Actual available fees are less than recorded. That's because a part of them corresponds to underwater positions, and will not have the correct amount stored with the contract: when calculation happens the fee is recorded first, then there is a check for position health, and the funds are channeled to cover the debt firsthand. This way in a case of unfunded position the fee is recorded, but cannot be allocated, so the fees accounted can be greater than value of fees stored.
This can lead to fee withdrawal malfunction, i.e. disburse() will burn more and attempt to transfer more than needed. This leads either to inability to withdraw fees when disburse be failing due to lack of funds, or funds leakage to fees and then inability to perform other withdrawals because of lack of funds.
The fees are accounted for before position health check and aren't corrected thereafter when there is a shortage of funds.
Adjust fees after position health check: accrue fees only on a remaining part of position that is available after taking debt into account.
Now:
uint _feeAmount = _userNotional.mulUp(mothership.fee()); uint _userValueAdjusted = _userNotional - _feeAmount; if (_userValueAdjusted > _userDebt) _userValueAdjusted -= _userDebt; else _userValueAdjusted = 0;
To be:
uint _feeAmount = _userNotional.mulUp(mothership.fee()); uint _userValueAdjusted = _userNotional - _feeAmount; if (_userValueAdjusted > _userDebt) { _userValueAdjusted -= _userDebt; } else { _userValueAdjusted = 0; _feeAmount = _userNotional > _userDebt ? _userNotional - _userDebt : 0; }
🌟 Selected for report: hyh
hyh
As OVL token is used as collateral for value storage and this way access to its mechanics equals access to the collateral funds.
A malicious Governor can switch OVL implementation to the one that steals funds on transfers, etc.
An observed possibility to alter core token implementation can also lead to reputational issues.
Governor role can instantly change OVL implementation: https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/mothership/OverlayV1Mothership.sol#L75
It's recommended to allow setting of OVL token implementation only once.
Now:
function setOVL (address _ovl) external onlyGovernor { ovl = _ovl; }
To be:
function setOVL (address _ovl) external onlyGovernor { require(ovl == address(0), "setOVL: ovl already set"); ovl = _ovl; }
If the project roadmap requires implementation change in the future it is advised to add a Timelock to the functionality.
#0 - commercium-sys
2021-11-24T17:00:25Z
I would say this is an incidental detail that would have been worked out as we get the contracts ready for their prime time deployment.
Whether or not we only set it once or allow it to be moved is a decision that we still need to consider finally as a team.
It could be we upgrade our token, after all.
#1 - dmvt
2021-12-18T12:44:32Z
This is hypothetical at best. The attack described would require the governor to intentionally attack the users. There is no way it could happen by accident. The governor choosing to attack the protocol is highly unlikely. However, since it is possible and would cause a loss of user funds, the issue stands as a low severity.
🌟 Selected for report: hyh
0.0161 ETH - $74.28
hyh
Gas is overspent on the function calls.
Function calls are redundant: https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/market/OverlayV1Governance.sol#L66
Remove the calls and streamline the function.
Now:
setK(_k); setSpread(_pbnj); setPeriods( _compoundPeriod ); setComptrollerParams( _lmbda, _staticCap, _brrrrdExpected, _brrrrdWindowMacro, _brrrrdWindowMicro );
To be:
// setSpread pbnj = _pbnj; // setK k = _k; // setPeriods compoundingPeriod = _compoundingPeriod; // setComptrollerParams lmbda = _lmbda; staticCap = _staticCap; brrrrdExpected = _brrrrExpected; brrrrdWindowMacro = _brrrrdWindowMacro; brrrrdWindowMicro = _brrrrdWindowMicro;
#0 - commercium-sys
2021-11-24T16:53:00Z
We're not terribly interested in saving gas for very occasionally used governance functions.
🌟 Selected for report: Meta0xNull
hyh
Being instantiated with wrong configuration, the contract is inoperable, so deploy gas costs will be lost.
Both constructors don't check for zero _mothership and OVL addresses. _mothership can be supplied as zero, while OVL can be not initialized yet. Both variables are immutable.
https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/market/OverlayV1Governance.sol#L49 https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/collateral/OverlayV1OVLCollateral.sol#L78
Add checks for zero address supplied and obtained:
Now:
mothership = IOverlayV1Mothership(_mothership); ovl = address(IOverlayV1Mothership(_mothership).ovl());
To be:
require (_mothership != address(0), "Zero mothership not allowed"); mothership = IOverlayV1Mothership(_mothership); ovl = address(IOverlayV1Mothership(_mothership).ovl()); require (ovl != address(0), "Zero OVL not allowed");
#0 - commercium-sys
2021-11-24T17:30:48Z
Again this burden is just on us to not mess up.
#1 - mikeyrf
2021-12-07T00:02:49Z
duplicate #32 - check zero address
🌟 Selected for report: hyh
Also found by: Meta0xNull
0.0072 ETH - $33.42
hyh
Gas is overspent on storage access.
pos.market variable is being read up to three times from storage:
https://github.com/code-423n4/2021-11-overlay/blob/main/contracts/collateral/OverlayV1OVLCollateral.sol#L371
Save the needed storage variable to memory and use it.
Now:
Position.Info storage pos = positions[_positionId]; ... ( uint _oi, uint _oiShares, uint _priceFrame ) = IOverlayV1Market(pos.market) .exitData( _isLong, pos.pricePoint ); MarketInfo memory _marketInfo = marketInfo[pos.market]; ... IOverlayV1Market(pos.market).exitOI(...
To be:
Position.Info storage pos = positions[_positionId]; address memory pos_market = pos.market; ... ( uint _oi, uint _oiShares, uint _priceFrame ) = IOverlayV1Market(pos_market) .exitData( _isLong, pos.pricePoint ); MarketInfo memory _marketInfo = marketInfo[pos_market]; ... IOverlayV1Market(pos_market).exitOI(...
#0 - commercium-sys
2021-11-24T16:57:17Z
Yeah, this is among the changes that I was considering for gas, after I got around to re-imagining the storage layout itself.