Overlay Protocol contest - gpersoon's results

Handle

gpersoon

Vulnerability details

Impact

The contract OverlayV1Market is abstract and includes several other contracts that are also abstract. The only deviation is the contract OverlayV1OI which isn't abstract.

The risk is that OverlayV1OI could be accidentally deployed separately.

Proof of Concept

https://github.com/code-423n4/2021-11-overlay/blob/914bed22f190ebe7088194453bab08c424c3f70c/contracts/market/OverlayV1Market.sol#L13

abstract contract OverlayV1Market is OverlayV1Governance {

https://github.com/code-423n4/2021-11-overlay/blob/914bed22f190ebe7088194453bab08c424c3f70c/contracts/market/OverlayV1Governance.sol#L11-L14

abstract contract OverlayV1Governance is OverlayV1Comptroller,  OverlayV1OI,  OverlayV1PricePoint {

https://github.com/code-423n4/2021-11-overlay/blob/914bed22f190ebe7088194453bab08c424c3f70c/contracts/market/OverlayV1Comptroller.sol#L9

abstract contract OverlayV1Comptroller {

https://github.com/code-423n4/2021-11-overlay/blob/914bed22f190ebe7088194453bab08c424c3f70c/contracts/market/OverlayV1OI.sol#L6

contract OverlayV1OI {    // only contract that isn't abstract

https://github.com/code-423n4/2021-11-overlay/blob/914bed22f190ebe7088194453bab08c424c3f70c/contracts/market/OverlayV1PricePoint.sol#L7

abstract contract OverlayV1PricePoint {

Tools Used

Recommended Mitigation Steps

Make contract OverlayV1OI also abstract

Handle

gpersoon

Vulnerability details

Impact

Suppose you try to build a position and have set the leverage accidentally to 0 (which can be done if you call the smart contract directly). Then the function build() will call enterOI() which will revert when trying to calculate debtAdjusted .

However no user friendly error message is given.

Proof of Concept

https://github.com/code-423n4/2021-11-overlay/blob/914bed22f190ebe7088194453bab08c424c3f70c/contracts/market/OverlayV1Market.sol

function enterOI ( bool _isLong,  uint _collateral,  uint _leverage ) external onlyCollateral returns (...) {
 ...
        collateralAdjusted_ = _collateral - _impact - fee_;       // will be > 0
        oiAdjusted_ = collateralAdjusted_ * _leverage;            // if _leverage==0 then oiAdjusted_  == 0
        debtAdjusted_ = oiAdjusted_ - collateralAdjusted_;   // will be negative and thus will revert

Tools Used

Recommended Mitigation Steps

Add something like the following to the function build() require(_leverage != 0, "OVLV1:leverage==0")

Handle

gpersoon

Vulnerability details

Impact

The function roll() of OverlayV1Comptroller.sol can be simplified. This saves some gas and also makes the function easier to read. See below at "Recommended Mitigation Steps"

Proof of Concept

https://github.com/code-423n4/2021-11-overlay/blob/914bed22f190ebe7088194453bab08c424c3f70c/contracts/market/OverlayV1Comptroller.sol#L352-L385

function roll( Roller[60] storage rollers, Roller memory _roller, uint _lastMoment, uint _cycloid ) internal returns ( uint cycloid_) {
        if (_roller.time != _lastMoment) {
            _cycloid += 1;
            if (_cycloid < CHORD) {
                rollers[_cycloid] = _roller;
            } else {
                _cycloid = 0;
                rollers[_cycloid] = _roller;
            }
        } else {
            rollers[_cycloid] = _roller;
        }
        cycloid_ = _cycloid;
    }

Tools Used

Recommended Mitigation Steps

Change the function to:

    function roll (Roller[60] storage rollers,Roller memory _roller,uint _lastMoment,uint _cycloid) internal returns (uint cycloid_) {
        if (_roller.time != _lastMoment) 
             _cycloid = (_cycloid + 1) % CHORD;                              
        rollers[_cycloid] = _roller;
        cycloid_ = _cycloid;
    }

Handle

gpersoon

Vulnerability details

Impact

The function oiCap() of OverlayV1Comptroller.sol save the value of brrrrdExpected in a tmp variable _brrrrdExpected. Lateron brrrrdExpected is still used while _brrrrdExpected could also be used. This saves a bit of gas.

Proof of Concept

https://github.com/code-423n4/2021-11-overlay/blob/914bed22f190ebe7088194453bab08c424c3f70c/contracts/market/OverlayV1Comptroller.sol#L255-L279

 function oiCap() public virtual view returns (  uint cap_ ) {
    ...
        uint _brrrrdExpected = brrrrdExpected;
    ...
        cap_ = _surpassed ? 0 : _burnt || _expected
            ? _oiCap(false, depth(), staticCap, 0, 0)
            : _oiCap(true, depth(), staticCap, _brrrrd, brrrrdExpected);  // can also use _brrrrdExpected 

Tools Used

Recommended Mitigation Steps

Replace

     : _oiCap(true, depth(), staticCap, _brrrrd, brrrrdExpected);

with

     : _oiCap(true, depth(), staticCap, _brrrrd, _brrrrdExpected);

Handle

gpersoon

Vulnerability details

Impact

The function value() of OverlayV1OVLCollateral.sol doesn't explicitly check for liquidated positions. However because oiShares and debt are set to 0 during liquidation the resulting value will still be 0.

It seems more logical to check for liquidation in the beginning of the function and immediately return 0. This saves gas for the situation where the function value() is called from another smart contract.

Proof of Concept

https://github.com/code-423n4/2021-11-overlay/blob/914bed22f190ebe7088194453bab08c424c3f70c/contracts/collateral/OverlayV1OVLCollateral.sol#L424-L448

function value (  uint _positionId ) public view returns ( uint256 value_) {
        Position.Info storage pos = positions[_positionId];
        IOverlayV1Market _market = IOverlayV1Market(pos.market);
        (   uint _oi,  uint _oiShares,   uint _priceFrame ) = _market.positionInfo( pos.isLong, pos.pricePoint );
        value_ = pos.value(  _oi, _oiShares,  _priceFrame );
}

Tools Used

Recommended Mitigation Steps

Add something like the following to function value():

        if (pos.oiShares == 0) return 0; // liquidated

Handle

gpersoon

Vulnerability details

Impact

In the function unwind() of OverlayV1OVLCollateral.sol a tmp variable _userOiShares is used the store the value of _shares. However _shares is still uses multiple times in the function.

Using _userOiShares everywhere would save gas.

Proof of Concept

https://github.com/code-423n4/2021-11-overlay/blob/914bed22f190ebe7088194453bab08c424c3f70c/contracts/collateral/OverlayV1OVLCollateral.sol#L273-L336

 function unwind (  uint256 _positionId,  uint256 _shares ) external {
        require( 0 < _shares && _shares <= balanceOf(msg.sender, _positionId), "OVLV1:!shares");    // uses _shares
        ...
        uint _userOiShares = _shares; // move to start of the function
        uint _userNotional = _shares * pos.notional(_oi, _oiShares, _priceFrame) / _totalPosShares;    // uses _shares
        uint _userDebt = _shares * pos.debt / _totalPosShares;                                                          // uses _shares
        uint _userCost = _shares * pos.cost / _totalPosShares;                                                           // uses _shares
        uint _userOi = _shares * pos.oi(_oi, _oiShares) / _totalPosShares;                                          // uses _shares
...  
      _burn(msg.sender, _positionId, _shares);   // uses _shares

Tools Used

Recommended Mitigation Steps

Move "uint _userOiShares = _shares;" to the start of function unwind() Replace all other instances of "_shares" with "_userOiShares"