OpenLeverage contest - defsec's results

Handle

defsec

Vulnerability details

Impact

On the helper contract, It has been observed that to safeIncreaseAllowance and safeDecreaseAllowance are commented out. (https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2268) safeApprove function is deprecated.

Proof of Concept

1. Navigate to the following contract.

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/lib/TransferHelper.sol#L49

Tools Used

Code Review

Recommended Mitigation Steps

Consider to enable functions and use safeIncreaseAllowance and safeDecreaseAllowance instead of safeApprove.

Handle

defsec

Vulnerability details

Impact

++i is more gas efficient than i++ in loops forwarding.

Proof of Concept

1. Navigate to the following contracts.

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/Airdrop.sol#L64

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/ControllerV1.sol#L303

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/OpenLevV1.sol#L52

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/OpenLevV1Lib.sol#L260

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/dex/bsc/BscDexAggregatorV1.sol#L46

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/dex/bsc/UniV2ClassDex.sol#L60

Tools Used

Code Review

Recommended Mitigation Steps

It is recommend to use unchecked{++i} and change i declaration to uint256.

Handle

defsec

Vulnerability details

Impact

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Proof of Concept

1. Navigate to the following smart contract line.

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/Airdrop.sol#L64

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/ControllerV1.sol#L303

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/OpenLevV1.sol#L52

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/OpenLevV1Lib.sol#L260

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/dex/bsc/BscDexAggregatorV1.sol#L46

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/dex/bsc/UniV2ClassDex.sol#L60

Tools Used

None

Recommended Mitigation Steps

Consider to cache array length.

Handle

defsec

Vulnerability details

Impact

During the code review, It has been observed that startTime and endTime do not have necessity checks and that will cause to broken functionality on the contract. That will be resulted with re-deployment.

Proof of Concept

1. Navigate to the following contract.

"https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/OLETokenLock.sol#L35"

Tools Used

Code Review

Recommended Mitigation Steps

Consider initialize variables with comparision block.timestamp. Endtime should be bigger than starttime.

Handle

defsec

Vulnerability details

Impact

Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.

The advantages of versions 0.8.* over <0.8.0 are:

• Safemath by default from 0.8.0 (can be more gas efficient than library based safemath.)
• Low level inliner : from 0.8.2, leads to cheaper runtime gas. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls.
• Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs.
• Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

Proof of Concept

1. The contest repository contracts contain floating pragma ^0.8.0. The contracts pragma version can be updated to 0.8.4 for the gas optimization.

• Affected Contracts

All Contracts

Tools Used

None

Recommended Mitigation Steps

Consider to upgrade pragma to at least 0.8.4.

Handle

defsec

Vulnerability details

Impact

In the Lpooldepositor contract, re-entrancy guard is defined but Its not used. Removing re-entrancy guard from the contract will provide gas optimization.

Proof of Concept

1. Navigate to the following contract.

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/liquidity/LPoolDepositor.sol#L14

2. Re-entrancy guard is not used.

Tools Used

Code Remove

Recommended Mitigation Steps

IT is recommended to delete un-used & redundant inheritance.

Handle

defsec

Vulnerability details

Impact

In some cases, having function arguments in calldata instead of memory is more optimal.

Consider the following generic example:

contract C {
function add(uint[] memory arr) external returns (uint sum) {
uint length = arr.length;
for (uint i = 0; i < arr.length; i++) {
sum += arr[i];
}
}
}

In the above example, the dynamic array arr has the storage location memory. When the function gets called externally, the array values are kept in calldata and copied to memory during ABI decoding (using the opcode calldataload and mstore). And during the for loop, arr[i] accesses the value in memory using a mload. However, for the above example this is inefficient. Consider the following snippet instead:

contract C {
function add(uint[] calldata arr) external returns (uint sum) {
uint length = arr.length;
for (uint i = 0; i < arr.length; i++) {
sum += arr[i];
}
}
}

In the above snippet, instead of going via memory, the value is directly read from calldata using calldataload. That is, there are no intermediate memory operations that carries this value.

Gas savings: In the former example, the ABI decoding begins with copying value from calldata to memory in a for loop. Each iteration would cost at least 60 gas. In the latter example, this can be completely avoided. This will also reduce the number of instructions and therefore reduces the deploy time cost of the contract.

In short, use calldata instead of memory if the function argument is only read.

Note that in older Solidity versions, changing some function arguments from memory to calldata may cause "unimplemented feature error". This can be avoided by using a newer (0.8.*) Solidity compiler.

Examples Note: The following pattern is prevalent in the codebase:

function f(bytes memory data) external {
(...) = abi.decode(data, (..., types, ...));
}

Here, changing to bytes calldata will decrease the gas. The total savings for this change across all such uses would be quite significant.

Proof Of Concept

Examples:

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/OpenLevV1.sol#L52

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/OpenLevV1Lib.sol#L260

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/dex/bsc/BscDexAggregatorV1.sol#L46

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/dex/bsc/UniV2ClassDex.sol#L60

Tools Used

None

Recommended Mitigation Steps

Change memory definition with calldata.

Handle

defsec

Vulnerability details

Impact

The assert keyword is used in several instances within the OpenLeverage smart contract suite. This keyword will revert a transaction in the event the condition is not satisfied, however, no gas is refunded to the user. As a result, this may lead to poor user experience and other issues if the supplied gas/gas price is considerably high.

Proof of Concept

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/XOLE.sol#L304

https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/farming/FarmingPools.sol#L78

Tools Used

Code Review

Recommended Mitigation Steps

Consider replacing all instances of the assert keyword with require.