Illuminate contest - hansfriese's results

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L294 https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L354

Vulnerability details

Impact

Lender.lend() functions for Swivel and Element don't mint for a user after the swap. So the balance of the principal token for the user will still be zero and he can't get paid back the underlying asset forever.

Proof of Concept

1. Alice lends 100 unit of the underlying token using Swivel or Element.
2. Swapped principal token will be transferred to the lender contract but the balance of Alice isn't updated.
3. Alice can't redeem his underlying asset forever as his balance is zero.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

The amount of principal token must be minted for the user.

For swivel function

uint256 purchased = yield(u, y, returned, address(this));
IERC5095(principalToken(u, m)).mint(msg.sender, purchased);

For element function

uint256 purchased = IElement(e).swap(swap, fund, r, d);
IERC5095(principalToken(u, m)).mint(msg.sender, purchased);

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/redeemer/Redeemer.sol#L128

Vulnerability details

Impact

Users can't receive back underlying tokens when they redeem.

Proof of Concept

Like the above comment, this function is designed to transfer underlying token to the user, but it transfers to Redeemer contract.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

Modification for transfer logic.

Safe.transferFrom(IERC20(u), lender, msg.sender, amount);

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/redeemer/Redeemer.sol#L126

Vulnerability details

Impact

An attacker who has some positive principal balance can steal others' balance. An attacker can set an address where to burn the principal token and he can use another address that has larger than his balance. After the balance is transferred to his address, attacker's balance is remaining and he can continue again.

Proof of Concept

1. Alice lends 100 unit of underlying asset using Lender.lend() for illuminate.
2. His PT balance is 99 and he can know other addresses that have lent underlying balance from events of yield function.
3. He calls redeem using param o = other address that has at least 99 balance.
4. 99 unit of underlying tokens are transferred to him but his balance is still 99 and he can continue while he knows other addresses to steal funds.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

The principal token must be burned from the caller directly. We can modify this part like below.

token.burn(msg.sender, amount);

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L290

Vulnerability details

Impact

Users don't send the fee in Lender.lend() for Swivel. As a result, the total balance of the lender contract would be less than the amount it must have. So admin would lose fees or Redeemer.redeem() for some users wouldn't work correctly.

Proof of Concept

• Alice lends 100 unit of underlying assets using Swivel. After calculation, lent = 99, totalFee = 1
• Alice must transfer 100 for both lent and totalFee, but he sends 99 only.
• Alice redeems for 99 before admin withdraws fees.
• Then the lender contract doesn't have underlying assets at all and the admin can't withdraw the fee even if fees[u] = 1. If the admin withdraws the fee first, Redeemer.redeem() for some users will be reverted as the total balance is less than required.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

Users must transfer the underlying asset including fees. You can modify this part like below.

Safe.transferFrom(IERC20(u), msg.sender, address(this), lent + totalFee);

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L192-L235

Vulnerability details

Impact

Lender.lend() for Illuminate and Yield doesn't add fees properly. As a result, admin would withdraw smaller fee than it should.

Proof of Concept

The estimated fee was transferred to the lender contract properly but that amount is not added to fees. So when admin withdraws fees, it will be less than it should.

But I don't think admin would lose it forever as admin can withdraw whole balance of contract using withdraw().

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

We can insert this part here.

fees[u] += calculateFee(a);

Summary

I've found 4 low-risk issues and some non-critical issues.

Low-Risk Issues

1. Missing events for only functions that change critical parameters. The fee ratio is very important for users and it should emit an event.

• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L137-L140

function setFee(uint256 f) external authorized(admin) returns (bool) {
    feenominator = f;
    return true;
}

2. Event functions emit wrong value.

• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L297

emit Lend(p, u, m, lent);

Originally Lend event should emit returned amount of PT. So it should emit the return value of yield() function. I've explain in detail in my High risk finding "Lender.lend() functions for Swivel and Element don't mint for user after swap."

• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/redeemer/Redeemer.sol#L147

emit Redeem(0, u, m, amount);

0 must be changed to p because p != 0 in this case.

emit Redeem(p, u, m, amount);

3. Lender.lend() for Swivel returns wrong value.

• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L298

return lent;

It's same as the above L-02 issue. It should return the result of yield() function

4. Wrong comments

• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/marketplace/MarketPlace.sol#L135

/// @return uint128 amount of PT bought

@return uint128 amount of PT sold

• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L305

/// @param a amount of principal tokens to lend

a is amount of underlying token for lend() functions. Below links are same issues.

• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L364
• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L415
• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L576

Non-critical Issues

1. Natspec incomplete. Below functions returns 3 outputs but only one for Natspec.

• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/marketplace/MarketPlace.sol#L201
• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/marketplace/MarketPlace.sol#L218
• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/marketplace/MarketPlace.sol#L230

2. Typo

• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L83

// max is the maximum integer value for a 256 unsighed integer

unsighed => unsigned

3. Needless @notice

• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L55

4. Needless code

• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L608

address illuminateToken = principalToken(u, m);(u, m);

(u, m);

5. Needless ()

• (address(uToken))

1. Use != 0 instead of > 0 for uint variables

• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L662

return feenominator > 0 ? a / feenominator : 0;

2. No need to initialize variables with default values

• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L265

for (uint256 i = 0; i < o.length; ) {

3. An array’s length should be cached to save gas in for-loops

• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L265

for (uint256 i = 0; i < o.length; ) {

4. Usage of unchecked can reduce the gas cost

• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L502

uint256 lent = a - fee;

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

uint256 lent = a - fee;

5. Better to use msg.sender instead of owner() when they are same

• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L654

Safe.transfer(token, admin, balance);

• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L698

Safe.transfer(token, admin, token.balanceOf(address(this)));

6. State variables only set in the constructor should be declared as immutable

• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/redeemer/Redeemer.sol#L33

address public apwineAddr;