Illuminate contest - Lambda's results

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/217ddfef05bc9df5c7b94f1c3226a46ee136b57d/marketplace/ERC5095.sol#L116

Vulnerability details

Impact

In redeem, it is checked that the allowance is larger than underlyingAmount, which is the return parameter (i.e., equal to 0 at that point). Therefore, this check is always true and there is no actual allowance check, allowing anyone to redeem for another user.

Recommended Mitigation Steps

Change the underlyingAmount to principalAmount, which is the intended parameter.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/217ddfef05bc9df5c7b94f1c3226a46ee136b57d/marketplace/ERC5095.sol#L100 https://github.com/code-423n4/2022-06-illuminate/blob/217ddfef05bc9df5c7b94f1c3226a46ee136b57d/marketplace/ERC5095.sol#L116

Vulnerability details

Impact

In withdraw and redeem (where there is another issue that the wrong amount is checked, for which an own submission exists), the allowance is not decreased after withdrawal / redemption. A user that has an allowance for only a small value can therefore call the function multiple times to withdraw / redeem more than what would be allowed by the allowance.

Proof Of Concept

We assume that Bob has 100 tokens gave Alice an allowance to spend 10 tokens. Alice now can call withdraw 10 times and pass 10 as underlyingAmount. Therefore, she gets all 100 tokens of Bob, although he did not allow that.

Recommended Mitigation Steps

Change the underlyingAmount to principalAmount, which is the intended parameter.

Lines of code

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

Vulnerability details

Impact

In redeem, the original underlying token should be transferred back to the user when an iPT is redeemed. But the target of the transfer is actually address(this), i.e. the Redeemer.

Recommended Mitigation Steps

Transfer the original underlying token to the user, as it is intended.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/217ddfef05bc9df5c7b94f1c3226a46ee136b57d/redeemer/Redeemer.sol#L143

Vulnerability details

Impact

In redeem, the maturity is not checked. For Tempus, this can lead to problems (loss of funds), because it supports early redemption with a smaller payout. See here for the code of Tempus that sets the early withdrawal flag before the maturity. Therefore, when someone (that can be different to the user) initiates a redemption for Tempus, a user may get much less tokens back when he can initiate the iLP redemption after the maturity.

Recommended Mitigation Steps

Disallow early redemption for Tempus, i.e. check that the maturity has passed.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/217ddfef05bc9df5c7b94f1c3226a46ee136b57d/redeemer/Redeemer.sol#L193

Vulnerability details

Impact

In redeem, the following code is executed for Notional:

amount = INotional(principal).maxRedeem(address(this));

However, in contrast to the other calls that are performed there, this does not initiate any transfer and only returns the maximum amount, because the definition of maxRedeem in Notional is:

function maxRedeem(address owner) public view override returns (uint256) {
        return balanceOf(owner);
}

Recommended Mitigation Steps

Call redeem for Notional (see the definition here).

• In general, using one character variable names makes reading & understanding the contract much harder (you always have to refer to the comments to see what a varible means) and does not have any advantages. See here for a discussion: https://www.codereadability.com/i-n-k-single-letter-variable-names/
• For the different lend methods, consider validating the p parameter (i.e., that it actually refers to the principal that the method expects). Otherwise, there can be security vulnerabilities when two principals share the same method(s) (which would not be too suprising, because they have similar functionality), in which case execution could still succeed, but with wrong values / assumptions.
• Missing error message in Cast.sol
• In Lender.sol, there is no way to overwrite an existing marketplace which might be undesirable (in case of upgrades / new deployments)
• In Lender.sol, minting for a protocol is still possible, even if it is paused. Consider also checking if it is paused there.
• In Line 72 of ERC20Permit.sol, there is duplicated code. DOMAIN_SEPERATOR() could be used instead.
• In Line 77, ecrecover is called without checking for invalid v and s values, which goes against best practices because of malleability (see https://0xsomeone.medium.com/b002-solidity-ec-signature-pitfalls-b24a0f91aef4 for details). Consider checking for those values or using OpenZeppelin's ECDSA library, which performs these checks for you.
• In Line 147, p is set to 0 when Redeem is emitted. This is wrong in this case, as p is not 0 in the else branch.
• In authRedeem within Redeemer.sol, no Redeem event is emitted, which is inconsistent with the other redeem functions.

• The subtraction in Line 190 of ERC20.sol can be unchecked.
• In Line 265 of Lender.sol, the length of the array can be cached.