Sherlock contest - pauliax's results

Handle

pauliax

Vulnerability details

Impact

function withdraw in ILendingPool returns the actual withdrawn amount, however, function withdraw in AaveV2 strategy does not check this return value so e.g. function strategyWithdraw may actually withdraw less but still add the full amount to the staked balance: ps.strategy.withdraw(_amount); ps.stakeBalance = ps.stakeBalance.add(_amount);

Recommended Mitigation Steps

function withdraw in IStrategy should return uint indicating the actual withdrawn amount and functions that use it should account for that.

Handle

pauliax

Vulnerability details

Impact

initializeSherXERC20 sets the name and symbol of the SherX token. 'initialize' in the name indicates that it should be called only once. It can only be called by a contractOwner but still, I think it would be better to either prevent multiple initializations or rename function to remove this confusion.

Recommended Mitigation Steps

Possible solutions:

1. You can forbid multiple initializations, e.g.: require(bytes(sx20.name).length == 0, 'OLD_NAME'); require(bytes(sx20.symbol).length == 0, 'OLD_SYMBOL');
2. Rename function to 'setSherXERC20' or something like that.

Handle

pauliax

Vulnerability details

Impact

contract AaveV2 does not cache the lending pool, it retrieves it when necessary by calling a function getLp(). This is great as the implementation may change, however, this contract also approves an unlimited amount of want in the constructor: ILendingPool lp = getLp(); want.approve(address(lp), uint256(-1)); so if the implementation changes, the approval will reset. This will break the deposit function as it will try to deposit to this new lending pool with 0 approval.

For reference, function setLendingPoolImpl: https://github.com/aave/aave-protocol/blob/4b4545fb583fd4f400507b10f3c3114f45b8a037/contracts/configuration/LendingPoolAddressesProvider.sol#L58-L65

Not sure how likely is that lending pool implementation will change so marking this as 'Low'.

Recommended Mitigation Steps

Before calling lp.deposit check that the approval is sufficient and increase otherwise.

Handle

pauliax

Vulnerability details

Impact

I think these checks should be inclusive: require(_unstakeWindow < 25000000, 'MAX'); require(_period < 25000000, 'MAX'); if (_amount > oldValue) // >= will reduce gas here

Recommended Mitigation Steps

require(_unstakeWindow <= 25000000, 'MAX');
require(_period <= 25000000, 'MAX');
if (_amount >= oldValue)

Handle

pauliax

Vulnerability details

Impact

In Base struct having 3 separate fields that map from _protocol is error-prone. If you later introduce new fields, etc, you need not forget to delete them in function protocolRemove, etc. I think it would be better to have a separate struct for protocol-related data and map to that.

Recommended Mitigation Steps

An example solution, replace: mapping(bytes32 => address) protocolManagers; mapping(bytes32 => address) protocolAgents; mapping(bytes32 => bool) protocolIsCovered; with: struct ProtocolInfo { address manager; address agent; bool covered; } struct Base { ... mapping(bytes32 => ProtocolInfo) protocolInfo; ... } Then you can delete all fields this way: delete gs.protocolInfo[_protocol]; Similar solution may be applied to PoolStorage (protocolBalance, protocolPremium, isProtocol).

Handle

pauliax

Vulnerability details

Impact

I see no re-entrancy mitigations. Contracts interact with various outside sources (tokens, aave pools, other possible strategies that may be added in the future, etc). so, for instance, now you have to be careful and do not allow tokens that have a receiver callback (e.g. erc777) or untrustable sources of yield (strategies).

Recommended Mitigation Steps

Consider using ReentrancyGuard on main action functions: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol

Handle

pauliax

Vulnerability details

Impact

When the address has no unstake entries function getInitialUnstakeEntry still returns 0 index. This function is external but can still confuse the outside consumers.

Recommended Mitigation Steps

Consider requiring ps.unstakeEntries[_staker].length > 0;

Handle

pauliax

Vulnerability details

Impact

Probably you are aware of this, but as I see many for loops throughout the code iterating over dynamic arrays I suggest being very careful as the execution may exceed the block gas limit, consume all the gas provided, and fail. Some arrays have removal functions, but there is, for instance, unstakeEntries array that is never actually removed as 'delete ps.unstakeEntries[msg.sender][_id];' only resets the values to default.

Recommended Mitigation Steps

You can consider introducing max limits on items in the arrays or make sure that elements can be removed from dynamic arrays in case it becomes too large.

Handle

pauliax

Vulnerability details

Impact

Variables in contract AaveV2 can be marked as immutable (constant) as their values do not change. All of the variables are eligible: lpAddressProvider, aaveIncentivesController, want, aWant, sherlock, aaveLmReceiver. Immutable greatly reduces gas costs.

Recommended Mitigation Steps

Add the 'immutable' or 'constant' keyword to the declarations of variables that are only set once.

Handle

pauliax

Vulnerability details

Impact

There are some massive structs. Gas efficiency can be achieved by tightly packing the struct. Struct variables are stored in 32 bytes each so you can group smaller types to occupy less storage. You can read more here: https://fravoll.github.io/solidity-patterns/tight_variable_packing.html or in the official documentation: https://docs.soliditylang.org/en/v0.4.21/miscellaneous.html

Recommended Mitigation Steps

It is hard to predict which order is the most gas efficient as, for example, library PoolStorage struct Base contains over 20 members with all different types. It will need several iterations of changing and testing to find an optimal order (with the help of gasReporter).

Handle

pauliax

Vulnerability details

Impact

.length in a loop can be extracted into a variable and used where necessary to reduce the number of storage reads. Examples where this is relevant: for (uint256 i = 0; i < ps.protocols.length; i++) for (uint256 i; i < gs.tokensStaker.length; i++) for (uint256 i; i < gs.tokensSherX.length; i++) Cache the length of the array and use this local variable when iterating over the storage array. For instance, function calcUnderlying reads gs.tokensSherX.length multiple times before so a cache variable should significantly reduce gas costs.

Recommended Mitigation Steps

uint protocolsLength = ps.protocols.length; for (uint256 i = 0; i < protocolsLength; i++) and so on

Handle

pauliax

Vulnerability details

Impact

There is no difference between functions aBalance and balanceOf in contract AaveV2, they both return aWant, so there is no point in having them separately.

Recommended Mitigation Steps

Remove internal function aBalance and make balanceOf public.

Handle

pauliax

Vulnerability details

Impact

Call to LibDiamond.contractOwner() can be cached here: require(msg.sender == LibDiamond.contractOwner(), 'NOT_DEV'); require(_govDev != LibDiamond.contractOwner(), 'SAME_DEV');

Recommended Mitigation Steps

address contractOwner = LibDiamond.contractOwner();
require(msg.sender == contractOwner , 'NOT_DEV');
require(_govDev != contractOwner, 'SAME_DEV');