Sublime contest - TomFrenchBlockchain's results

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Loss of ETH sent to CreditLine when borrowing.

Proof of Concept

https://github.com/code-423n4/2021-12-sublime/blob/9df1b7c4247f8631647c7627a8da9bdc16db8b11/contracts/CreditLine/CreditLine.sol#L691

Borrow accepts ETH which is sent by the lender but doesn't do anything with it resulting in lost funds.

Recommended Mitigation Steps

Make this function nonpayable.

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

Take for example the depositCollateral function. It's payable but the pool may not use ETH as collateral

https://github.com/code-423n4/2021-12-sublime/blob/9df1b7c4247f8631647c7627a8da9bdc16db8b11/contracts/Pool/Pool.sol#L175-L179

In the case where the user is performing a direct deposit then we'll pull in the erc20 collateral asset using the transferTokens function however there's no check for msg.value == 0 when transferring an ERC20 asset.

https://github.com/code-423n4/2021-12-sublime/blob/9df1b7c4247f8631647c7627a8da9bdc16db8b11/contracts/SavingsAccount/SavingsAccountUtil.sol#L98-L127

Similarly when depositing ETH from a savings account any ETH in msg.value will be lost.

Recommended Mitigation Steps

Add a check that msg.value is zero on all code paths which do not handle ETH.

Consider whether to simplify to just use WETH.

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Inability to set a sensible range which covers all borrow assets without being overly wide.

Proof of Concept

PoolFactory has a set requirement that created pool must ask to borrow an amount of assets which are within a certain range as encoded in the poolSizeLimit.

https://github.com/code-423n4/2021-12-sublime/blob/9df1b7c4247f8631647c7627a8da9bdc16db8b11/contracts/Pool/PoolFactory.sol#L286

This doesn't take into account the relative values of each borrow asset so it's hard to choose a one-size-fits-all value for these limits (A 100 WBTC loan could be sensible but a 100 USDC loan will be dwarfs by the gas costs to deploy the pool.)

Recommended Mitigation Steps

Place limits on the USD value of the borrowed amount as reported by the factory's price oracle.

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Gas costs on linking/unlinking addresses

Proof of Concept

https://github.com/code-423n4/2021-12-sublime/blob/9df1b7c4247f8631647c7627a8da9bdc16db8b11/contracts/Verification/Verification.sol#L10-L13

The activation timestamp can be restricted to a uint64 variable so that it shares a slot with the masterAddress. This will save an SSTORE and a substantial amount of gas.

Recommended Mitigation Steps

As above.

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Extra gas costs

Proof of Concept

As we always deposit the full amount which is approved immediately we know that the allowance will be reset to zero. This line which protects against tokens which require resetting the allowance to zero before approving is therefore redundant.

https://github.com/code-423n4/2021-12-sublime/blob/9df1b7c4247f8631647c7627a8da9bdc16db8b11/contracts/yield/AaveYield.sol#L297

Recommended Mitigation Steps

Remove this line and its equivalent in other yield contracts

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Gas costs

Proof of Concept

https://github.com/code-423n4/2021-12-sublime/blob/9df1b7c4247f8631647c7627a8da9bdc16db8b11/contracts/Pool/Pool.sol#L322-L340

In Pool.withdrawBorrowedAmount we set the loan to active and delete the withdrawal deadline (see link).

https://github.com/code-423n4/2021-12-sublime/blob/9df1b7c4247f8631647c7627a8da9bdc16db8b11/contracts/Pool/Pool.sol#L802

When checking whether we can liquidate the pool we check that the loan is active and that block.timestamp > poolConstants.loanWithdrawalDeadline. This second condition always resolves true as poolConstants.loanWithdrawalDeadline = 0 after deletion. We can then save an SLOAD by skipping this second check.

Recommended Mitigation Steps

As above.

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Gas costs

Proof of Concept

In the PoolConstants struct we have a number of fields which don't need to take up an entire storage slot. Easy examples are the timestamps which can be placed in a uint64 variable and packed with an address.

https://github.com/code-423n4/2021-12-sublime/blob/9df1b7c4247f8631647c7627a8da9bdc16db8b11/contracts/Pool/Pool.sol#L46-L59

This will save gas when performing the initial writes of these structs.

Recommended Mitigation Steps

Use smaller types for variables which don't need 256 bits when saving to storage. Consider which values are being written/read together when choosing how to lay out the packed struct.

Handle

TomFrenchBlockchain

Vulnerability details

Impact

gas costs

Proof of Concept

https://github.com/code-423n4/2021-12-sublime/blob/9df1b7c4247f8631647c7627a8da9bdc16db8b11/contracts/CreditLine/CreditLine.sol#L600-L604

(msg.sender == creditLineConstants[_id].borrower && _requestByLender) || (msg.sender == creditLineConstants[_id].lender && !_requestByLender)

is equivalent to

_requestByLender ? (msg.sender == creditLineConstants[_id].borrower) : (msg.sender == creditLineConstants[_id].lender)

or

msg.sender == (_requestByLender ? creditLineConstants[_id].borrower : creditLineConstants[_id].lender)

Which avoid loading the borrower address in the case where the borrower made the request.

Recommended Mitigation Steps

Use simplified logic

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Gas costs

Proof of Concept

Nested mappings cost N * (30+2*6) = 42N gas to calculate the storage slot where N is the number of layers of the mapping.

https://docs.soliditylang.org/en/v0.8.10/internals/layout_in_storage.html#mappings-and-dynamic-arrays

https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/SavingsAccount/SavingsAccount.sol#L35-L41

In SavingsAccount we have two 3 layer mappings which cost 126 gas to calculate the slot for. If we flatten this mapping to use a single id balanceId = keccak(abi.encodePacked(user, token, strategy)) (this is secure as the packed encoding is non-ambiguous) we then just have to pay (30+2*6) + (30+3*6) = 90 gas.

Recommended Mitigation Steps

Flatten these mappings and add private functions for calculating the relevant ids