Canto v2 contest - ladboy233's results

Lines of code

https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/Stableswap/BaseV1-periphery.sol#L489

Vulnerability details

Impact

Detailed description of the impact of this finding.

Underlying asset price oracle for CToken in BaseV1-periphery is inaccuarte

Proof of Concept

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

    function getUnderlyingPrice(CToken ctoken) external override view returns(uint price) {
        IBaseV1Pair pair;
        uint8 stable;
        bool stablePair;
        address underlying;

        if (compareStrings(ctoken.symbol(), "cCANTO")) {
            stable = 0;
            underlying = address(wcanto);
        } 
        //set price statically to 1 when the Comptroller is retrieving Price
        else if (compareStrings(ctoken.symbol(), "cNOTE") && msg.sender == Comptroller) {
            return 1; // Note price is fixed to 1
        }

we should not be return 1. 1 is 1 wei. we should be 10 ** 18

Tools Used

VIM

Recommended Mitigation Steps

we can return 10 ** 18

Lines of code

https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/Stableswap/BaseV1-core.sol#L71 https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/Stableswap/BaseV1-core.sol#L166 https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/NoteInterest.sol#L136

Vulnerability details

Impact

Detailed description of the impact of this finding.

Frequent price update make the project vulnerable to price oracle manipulation

Proof of Concept

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

in the code of stableswap V1 core,

    // Capture oracle reading every 30 minutes
    uint constant periodSize = 0;

the periodSize defines how frequently the price is updated.

Then in _update function

        if (timeElapsed > periodSize) {
            observations.push(Observation(blockTimestamp, reserve0CumulativeLast, reserve1CumulativeLast));
        }

previously periodSize, which means the price is updated 30 minutes. If periodSzie is 0 then it means the price can be updated any time.

Hackers can manipulate the price and raise the interest in this favor.

Tools Used

Recommended Mitigation Steps

change periodSize back to 1800.

Lines of code

https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/Stableswap/BaseV1-periphery.sol#L86 https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/Stableswap/BaseV1-periphery.sol#L262 https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/Stableswap/BaseV1-periphery.sol#L278 https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/Stableswap/BaseV1-periphery.sol#L298 https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/Stableswap/BaseV1-periphery.sol#L317 https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/Stableswap/BaseV1-periphery.sol#L391 https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/Stableswap/BaseV1-periphery.sol#L412 https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/Stableswap/BaseV1-periphery.sol#L427 https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/Stableswap/BaseV1-periphery.sol#L441 https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/Stableswap/BaseV1-periphery.sol#L457

Vulnerability details

Impact

Detailed description of the impact of this finding.

The unused modifier that used to ensure that transaction deadline is met is removed. so the transaction order matters and if transaction lands latter block, the transaction is subject to frontend running (MEV)

Proof of Concept

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

    modifier ensure(uint deadline) {
        //require(deadline >= block.timestamp, "BaseV1Router: EXPIRED");
        _;

the modifier is removed. The modifier check to ensure transaction make the deadline requirement. also because the modifier commented.

the function that use the modifier also leave the deadline parameter unused.

 function addLiquidity(
        address tokenA,
        address tokenB,
        bool stable,
        uint amountADesired,
        uint amountBDesired,
        uint amountAMin,
        uint amountBMin,
        address to,
        uint deadline
    ) external ensure(deadline)

Removing the modifier is bad if we consider the following case.

At block 1, the user wants to add 100 tokens B, 100 token B to main the liquidity ratio.

due to a gas issue or congested network, the transaction does not land in blockchain 2, it lands in blockchain 10.

say at block 5, a user already add 200 token A and 250 token B, then when the add liquidity function lands, the liqudity pool liqudity ratio

is unexpectedly affect.

same apply to remove and swap. Without check the deadline, the user may swap at bad rate in much later timeframe or remove a suboptimal amount of liquidity.

Tools Used

VIM

Recommended Mitigation Steps

restore the modifier

Outdated comments can be removed.

https://github.com/Plex-Engineer/manifest-v2/blob/f6ebfe679973edf4f64832e64480ff5250ef8486/contracts/Proposal-Store.sol#L6

// This is an evil token. Whenever an A -> B transfer is called, half of the amount goes to B
// and half to a predefined C

can be removed.

use require with error message to check conditions.

https://github.com/Plex-Engineer/manifest-v2/blob/f6ebfe679973edf4f64832e64480ff5250ef8486/contracts/Proposal-Store.sol#L44

can change

require(msg.sender == UniGovModAcct);

to

require(msg.sender == UniGovModAcct, "invalid account address");

Use WETH implement from ethereum

https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/WETH.sol

Developer can reuse the implementation of WETH from ethereum mainnet

https://etherscan.io/address/0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2#code

Rename forked contract

https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/Governance/GovernorBravoDelegate.sol#L9

can change

string public constant name = "Compound Governor Bravo";

to 'project Governor Bravo'

Make QueryProp public in Proposal-store to save gas

https://github.com/Plex-Engineer/manifest-v2/blob/f6ebfe679973edf4f64832e64480ff5250ef8486/contracts/Proposal-Store.sol#L49

https://github.com/Plex-Engineer/manifest-v2/blob/f6ebfe679973edf4f64832e64480ff5250ef8486/contracts/Proposal-Store.sol#L33

Can make the proposals public so in QueryProp,

    function QueryProp(uint propId) public view returns(Proposal memory){
        if (proposals[propId].id == propId) {
            return proposals[propId];
        }
	    return Proposal(0, "", "", new address[](0), new uint[](0), new string[](0), new bytes[](0));
    }

We can just return proposals[propIds] to save gas.

make variable that only init one immutable

https://github.com/Plex-Engineer/manifest-v2/blob/f6ebfe679973edf4f64832e64480ff5250ef8486/contracts/Proposal-Store.sol#L31

The address can be declared as immutable to save gas.

Function can be declared external instead of public to save gas

https://github.com/Plex-Engineer/manifest-v2/blob/f6ebfe679973edf4f64832e64480ff5250ef8486/contracts/Proposal-Store.sol#L42

https://github.com/Plex-Engineer/manifest-v2/blob/f6ebfe679973edf4f64832e64480ff5250ef8486/contracts/Proposal-Store.sol#L49

function AddProposal()
 function QueryProp()

can be declared as external

https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/Stableswap/BaseV1-periphery.sol#L203

quoteRemoveLiqudity can be declared as external

https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/WETH.sol#L28

Withdraw function can be as external

https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/Comptroller.sol#L1479

setWETH can be changed from public to external.

Remove debugging console log

https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/Stableswap/BaseV1-core.sol#L4

https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/Stableswap/BaseV1-core.sol#L207

Remove console log to save gas.

Unused modifier

https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/Stableswap/BaseV1-periphery.sol#L86

    modifier ensure(uint deadline) {
        //require(deadline >= block.timestamp, "BaseV1Router: EXPIRED");
        _;
    }

the modifier got commented out and did nothing.