AbraNFT contest - bobi's results

[L1] Check array lengths for out of bounds access:

Assessed risk: 2/10

Urgency: N/A

Codebase frequency: 1

[L1 - Impact]:

Accessing multiple arrays with the same for loop’s index often leads to out of bounds access, should the arrays be of different lengths. This is typically bad user experience, even if it’s the user’s fault, since the transaction will eventually get reverted consuming all the gas allocated until the out of bounds access.

for (uint256 i = 0; i < actions.length; i++) {
	uint8 action = actions[i];
	...
  (uint256 tokenId, bool skim) = abi.decode(datas[i], (uint256, bool));
	...
	(value1, value2) = _bentoDeposit(datas[i], values[i], value1, value2);

[L1 - References]:

NFTPair.sol#L636

[L1 - Mitigation]:

Add a require that checks the lengths of the arrays between one another.

require(
	actions.length == values.length && actions.length == datas.length,
	"arrays have different length"
);

[L2] Mapping not properly deleted.

Assessed risk: 3/10

Urgency: N/A

Codebase frequency: 2

[L2 - Impact]:

The tokenLoanParams does not get deleted at the same time as tokenLoan . Although this is not a security issue per se, it’s confusing for users that might check their loan parameters via interacting with the contract.

[L2 - References]:

NFTPair.sol#L265
NFTPair.sol#L537

[L2 - Mitigation]:

Essentially, an extra delete operation on the tokenLoanParams is needed wherever the delete tokenLoan[tokenId] is called.

delete tokenLoanParams[tokenId];

[G1] Cache arrays length:

[G1 - Details]:

Caching arrays is a powerful way of lowering gas fees. Since in a for loop accessing a memory array’s indexes would consume more gas than accessing the same array’s cached indexes as the length of the array becomes larger and larger, it is crucial to do the caching wherever possible.

[G1 - References]:

NFTPair.sol#L641

[G1 - Mitigation]:

Cache the array in a similar way:

function cook(
    uint8[] calldata actions,
    uint256[] calldata values,
    bytes[] calldata datas
) external payable returns (uint256 value1, uint256 value2) {
    uint256 actions_len = actions.length;
    for (uint256 i = 0; i < actions_len; i++) {
	...