Illuminate contest - Metatron's results

Lines of code

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

Vulnerability details

Impact

Will result in either failure to lend or loss of funds for the lender

Proof of Concept

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

• Not very clear what was the original intention, but I find it hard to believe that subtracting the whole balanceOf iPTs from the return result from Tempus, is what was intended.
• If the balance is higher than the return value - the operation reverts on underflow - failure to execute lend.
• If the balance is equal or lower than the return value - lender receive less minted iPTs then expected - loss of funds for him.
• If you argue that the balance will remain zero and this will go smoothly, please consider that an attacker can maliciously transfer iPTs to the contract address in order to cause the above problems to the project.

Recommended Mitigation Steps

• Remove the subtraction of illuminateToken.balanceOf(address(this))

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol?plain=1#L486-L534

Vulnerability details

Impact

Attacker can steal funds and inflate or break the system

Proof of Concept

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol?plain=1#L486-L534

• Attacker recognize a 'sense' market place opened by admin, with underlying u and maturity m.
• Attacker deploys a malicious contract that agrees on ISense interface, returning the maturity m on .maturity() (to pass L504 if) and returning an arbitrary big value on .swapUnderlyingForPTs(..)
• Attacker calls the lend function as he would for appropriate interaction, BUT passes a small amount a and address to his malicious contract as x.
• At L524 his contract returns an arbitrary big amount and at L530 attacker receives this amount as iPTs minted for him.
• Attacker sells these many iPTs immediately on secondary market, or just wait for maturity.
• As a result, attacker gained funds he did not invest, the iPT value inflates to nothing, or the project left with no funds on maturity (respectively).

Recommended Mitigation Steps

• Limit the amount minted to no more than the amount invested by the user.
• Make sure that x is actually a trustable AMM for sense contracts (I don't know sense enough to point how to do so)

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L247-L305 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L317-L367 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L192-L235

Vulnerability details

Impact

Using any of the lend function mentioned, will result in loss of funds to the lender - as the funds are transferred from them but no iPTs are sent back to them! Basically making lending via these external PTs unusable.

Proof of Concept

There is no minting of iPTs to the lender (or at all) in the 2 lend functions below: https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L247-L305 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L317-L367

Corresponding to lending of (respectively): swivel element

Furthermore, in: https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L227-L234 Comment says "Purchase illuminate PTs directly to msg.sender", but this is not happening. sending yield PTs at best.

Recommended Mitigation Steps

Mint the appropriate amount of iPTs to the lender - like in the rest of the lend functions.

Lines of code

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

Vulnerability details

Impact

Wrong accounting calculation may result in malfunctioning and loss of funds for the project.

Proof of Concept

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

• As seen in the lines above, the amount transferred from the lender is after subtracting the fee he actually needs to pay
• On the other hand, the amount swapped on 'swivel' is the original (but should be the subtracted amount - fee) and this takes out from lender.sol more than received.
• On other lend functions the accounting is done as I described, the opposite of the current code.

Recommended Mitigation Steps

I recommend doing the accounting like this:

uint256 amount = a[i];
lent += amount; 
uint256 fee = calculateFee(amount);
a[i] -= fee  // correct the amount vector
amountToMint += a[i]  // amount minus the fee

Then

• transferFrom with corrected lent (including the fees)
• initiate swivel with the corrected amounts vector a
• mint only amountToMint to the lender (sum of amounts minus the fees)

Lines of code

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

Vulnerability details

Proof of Concept

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

• The full amount a is transferFrom lender on L215
• The amount - fee is invested on L219 and L229.
• Unlike other lend function, this one is missing crediting the project with the fees they entitled to.
• As a result, these fees cannot be collected via withdrawFee or otherwise.

Recommended Mitigation Steps

Adding these lines:

uint256 fee = calculateFee(amount);
fees[u] += fee;

Before the if at L217, And can use calculated fee fee at L219 and L229 instead of calculating again.

Lines of code

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

Vulnerability details

Impact

Assuming admin decides to pause an external principle when it's dangerous, malicious or unprofitable, Bypassing the admins decision can result in loss of funds for the project.

Proof of Concept

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

• The principals enum p is only used for unpaused(p) modifier, and to emit an event.
• Attacker can bypass the unpaused(p) modifier check by simply passing an enum of another principle that is not paused.
• The function will just continue as normal, without any other side-effect, as if the pause is simple ignored.

Recommended Mitigation Steps

Add this check at the beginning of the function (just like in similar functions of this solution) if (p != uint8(MarketPlace.Principals.Swivel)) { revert Invalid('principal'); }

[L-01] duplicated comment https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L313

[L-02] Change revert string to "principal already been set" https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L100