code-423n4 / 2021-08-yield-findings

1 stars 0 forks source link

Combine get and peek #6

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

gpersoon

Vulnerability details

Impact

The get and peek functions of the *MultiOracle.sol contracts are very similar, within each contract. (ChainlinkMultiOracle.sol, CompositeMultiOracle.sol, CTokenMultiOracle.sol)

They can be integrated to make the contracts easier to maintain and also save a bit of gas. When different underlying functions need to be called, a function pointer can be used. See suggestion below.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Recommended Mitigation Steps

Suggestion for combining the get and peek functions:

// SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.6;

contract Test2 { function get(bytes32 , bytes32 , uint256 ) external virtual returns (uint256 value, uint256 updateTime) { return (0,0); } function peek(bytes32, bytes32 , uint256 ) external virtual returns (uint256 value, uint256 updateTime) { return (0,0); } }

contract Test1 { Test2 under=new Test2();

function CallGetOrPeek(bytes32 base, bytes32 quote, uint256 amount ,function(bytes32,bytes32,uint256)external returns(uint256,uint256) fp) internal returns (uint256 value, uint256 updateTime)
{
    return fp(base, quote, amount);
}
function TestGetOrPeek() external 
{   
    CallGetOrPeek(0,0,0,under.get);
    CallGetOrPeek(0,0,0,under.peek);
}

}

alcueca commented 3 years ago

While it would make the code easier to maintain, it would make it more difficult to integrate as peek wouldn't be a view function anymore, breaking with the IOracle interface.

It would work in our code, but I don't think it's worth it.

ghoul-sol commented 3 years ago

I'll keep this suggestion valid however I understand that it's not worth doing as there's a ripple effect of refactoring.