Error on array .pop()

Question

I have a function to modify an array, if I comment out the .pop() line, it passes, if not, I get this error:

VM error.revert

Code:

function moveShipsToSpace(uint[] memory ships, uint16 fromSpace, uint16 toSpace) public{
 for(uint8 i= 0; i&lt;ships.length; i++) { 
    require(containsInSpace(ships[i],fromSpace), &quot;dont have that ship&quot;);            
    // Move the last element into the place to delete
    whichShipsAtSpace[msg.sender][fromSpace][i] = whichShipsAtSpace[msg.sender][fromSpace][whichShipsAtSpace[msg.sender][fromSpace].length - 1];
    // Delete
    whichShipsAtSpace[msg.sender][fromSpace].pop();
    // Insert in new location
    whichShipsAtSpace[msg.sender][toSpace].push(shipsArray(ships[i]));
    emit LogUint(ships[i]);
 }

}

Also, if I pass a single number, it passes with .pop(), if I pass 2 or more it fails, so not sure what is the issue, please halp!

https://stackoverflow.com/questions/49051856/is-there-a-pop-functionality-for-solidity-arrays — MadeInDreams, Oct 27 '21 at 20:13

score 0 · Accepted Answer · answered Oct 27 '21 at 20:38

It actually fails here :

whichShipsAtSpace[msg.sender][fromSpace][i] = ...

because you assume that the index 'i' is a valid index for both the arrays ships and whichShipsAtSpace[msg.sender][fromSpace]. But it's not.

Ships will always stay the same length, but whichShipsAtSpace[msg.sender][fromSpace] length will decrease as you pop items out of it. At some point, the variable i is an out of range index for whichShipsAtSpace[msg.sender][fromSpace].

To fix this, you need to replace :

whichShipsAtSpace[msg.sender][fromSpace][i] = ...

by :

whichShipsAtSpace[msg.sender][fromSpace][shipIndexInFromSpace(ships[i])] = ...

Where shipIndexInFromSpace is a function returning the index of ships[i] inside the whichShipsAtSpace[msg.sender][fromSpace] array.

It's actually a good thing that your code crashed because it's not even doing what you want it to do in its current state. As the index 'i' points to an unknown ship and the order of whichShipsAtSpace[msg.sender][fromSpace][i] elements is being shifted, I beleive your code can potentially silently delete a different ship than the one you intended to.

You are the realest mvp of this site. Do you accept a tip in crypto? A coin with low fees like Dash would work. I tried to dm you, but apparently there is no such functionality here. — Angel Guastaferro, Oct 27 '21 at 22:22
Thanks but no need, really, that's what those websites are for. — hroussille, Oct 28 '21 at 07:10

Error on array .pop()

1 Answers1