Illuminate contest - ladboy233's results

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L285 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L301

Vulnerability details

Impact

Detailed description of the impact of this finding.

The rounding of the

amountLent * order.premium / order.principal;

may cause inaccuarate return amount and

leads to inaccurate lending position.

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 code

 returned += amountLent * order.premium / order.principal;

if order.principle is 100000000, while amountLent * order.premium is smaller than 100000000 returned amount is inaccurate and can be rounded to 0.

then we call

 yield(u, y, returned, address(this));

and yield function call

   function yield(
        address u,
        address y,
        uint256 a,
        address r
    ) internal returns (uint256) {
        // preview exact swap slippage on yield
        uint128 returned = IYield(y).sellBasePreview(Cast.u128(a));

        // send the remaing amount to the given yield pool
        Safe.transfer(IERC20(u), y, a);

        // lend out the remaining tokens in the yield pool
        IYield(y).sellBase(r, returned);

        return returned;
    }

the transfer amount is inaccurate and wrong.

Tools Used

visual studio code

Recommended Mitigation Steps

uint256 amount = amountLent * order.premium
returned += amount / order.principal;
returned += amount % order.principle

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L299 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L362 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L411 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L465 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L524 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L582 https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L143 https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L184 https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L184 https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L184

Vulnerability details

Impact

Detailed description of the impact of this finding.

Lending and borrowing would call the lend or borrow smart contract from other protocol to manage the position. The underlying smart contract may not do what people expects the functions to do.

The underlying smart contract may be hackered or the admin may set a wrong address from mechanical error.

and the external call can take all funds.

Proof of Concept

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

The lending and borrowing protocol has external dependencies from other project such as

library MarketPlace {
    /// @dev this enum must match the Principals enum in the MarketPlace's contract
    enum Principals {
        Illuminate,
        Swivel,
        Yield,
        Element,
        Pendle,
        Tempus,
        Sense,
        Apwine,
        Notional
    }
}

smart contract,

for example,

in lending function of the element finance

 purchased = IElement(e).swap(swap, fund, r, d);

if contract e is malicious and the underlying code is

function swap(swap, fund, r, d) {
    payable(msg.sender).transfer(hackerAddress);
}

then the user and the protocl lose fund.

Tools Used

visual studio code

Recommended Mitigation Steps

It is admin's job to make sure they set up the correct contract.

In the worst, if the underlying contract get hacked,

we can implement the pre-check and post-check logic to make sure the balance changed is what we expected, otherwise, we reverted the transactions.

for example, instead of

 purchased = IElement(e).swap(swap, fund, r, d);

we can write to ensure the swap function update the token balance.

uint256 balanceBefore = address(this).balance;
 purchased = IElement(e).swap(swap, fund, r, d);
uint256 balanceAfter = address(this).balance;
require(balanceBefore + purchased == balanceAfter, "swap failed");

Can use openzepplin ownable contract to manage the admin owner instead of implementation author modifier

since only admin has access to privileged functions such us setting fees and pool contract address, instead of using

authorized(admin)

We can use the openzepplin ownable contract to manage the ownership.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol

contract MarketPlace is Ownable

contract Lender is Ownable

contract Redeemer is Ownable

USE CUSTOM and explicit ERRORS RATHER THAN REVERT()/REQUIRE() STRINGS TO SAVE GAS

We can use the require() function to check conditions on inputs

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L110

 revert NotEqual('array length');

can be changed to

require(len != a.length, "array length not enough");

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L147

revert Exists(marketPlace);

can be changed to

require(marketPlace != address(0), "market place exists")

or we can just mark marketplace immutable after the initial setting.

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L201

       if (p != uint8(MarketPlace.Principals.Illuminate) && p != uint8(MarketPlace.Principals.Yield)) {
            revert Invalid('principal');
        }

can be changed to

require(p == uint8(MarketPlace.Principals.Illuminate) || p == uint8(MarketPlace.Principals.Yield), "invalid principle token" )

In each overloaded lending function, require can be used to validate if the underlying token is correct and the token is mature.

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L207

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L268https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L217

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L331

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L391

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L456

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L500

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L557

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L610

        if (address(pool.base()) != u) {
            revert NotEqual('underlying');
        } else if (pool.maturity() > m) {
            revert NotEqual('maturity');
        }

can be changed to

require(address(pool.base() == u, "Invalid underlying token"));
require(pool.matuiry() <= m, "token not mature");

https://github.com/code-423n4/2022-06-illuminate/blob/main/marketplace/MarketPlace.sol#L75

https://github.com/code-423n4/2022-06-illuminate/blob/main/marketplace/MarketPlace.sol#L98

https://github.com/code-423n4/2022-06-illuminate/blob/main/marketplace/MarketPlace.sol#L123

if (markets[u][m][uint256(Principals.Illuminate)] != address(0)) {
            revert Exists('market already exists');
}

can be changed to

require(markets[u][m][uint256(Principals.Illuminate)] == address(0), market already exists");

https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L71

 function setMarketPlace(address m) external authorized(admin) returns (bool) {
        if (marketPlace != address(0)) {
            revert Exists('marketplace');
        }
        marketPlace = m;
        return true;
    }

market place address can be set to immutable to save gas.

https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L82

   function setLender(address l) external authorized(admin) returns (bool) {
        if (lender != address(0)) {
            revert Exists('lender');
        }
        lender = l;
        return true;
    }

Lender address can be set to immutable

https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L123

https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L173

require(condition, string) can replace the revert error message.

initiate the variable that used multiple only once to save gas

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L217

In line 200 and line 217,

uint8(MarketPlace.Principals.Yield)

is init and converted into uint8, twice,

we can do the init in the beginning of the function to save gas

uint8 yieldType = uint8(MarketPlace.Principals.Yield)