Brief audit of `wlands_staking.ride`

MEDIUM: dangerous else its better to throw (same as Brief audit of economics.ride #3): https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L219-L220 Considered to be safe - we support early users who joined before backpack development

LOW: payments count not verified: https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L374 https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L420 Fixed

MEDIUM: unused landNumInt leads to bypass parseInt(): https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L385 Fixed

LOW: undefined constant 25 in many places may be defined somewhere near: https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L164-L168 Fixed

LOW: global key-value is hidden inside a function and initializes on the fly: https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L138 See no problem with that

LOW: managing 2 timestamp keys with same value looks excessive (you can always find address by asset, and always know its land or duck): https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L393-L394 (and other places) (and same for ducks staking) Considered

HIGH: unstakeLand() needs a staked duck but stakeLand() not which can lead to unstakable land for an user: https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L405 https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L290 Fixed

MEDIUM: Unmanaged/erroneous keys after unstakeLand(): https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L395-L396 Fixed

CRITICAL: negative amount leads to future: https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L477 https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L314 https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L319 Fixed

LOW: is there a real usage of availRes != amount (this can be simplified to more simple and robust code in the first place): https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L313 We do use availRes < amount condition

MEDIUM: unused interface argument landAssetIdStr: https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L328 Now we use it

MEDIUM: strange checks instead of calling some Waves Ducks dapp-to-dapp function to verify a duck: https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L425-L426 We are not going to check their interface changes every time, but believe contract addresses will remain

LOW: duplicate checks at unstakeDuck() which are already done at stakeDuck(): https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L452-L454 Fixed

HIGH: saved health regardless of unstakeDuck() can be a surprise for a new owner: https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L443 https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L462-L466 Fixed

CRITICAL: a trusted message: https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L533 does not guarantee 1) it binds to a caller 2) it binds to a caller current state 2) it cannot be reused by caller/anyone with manipulation of other functions in between. These defenses are not enough: https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L542-L543 https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L547 (same for expeditionCommon()) We considered it acceptable for development stage, but will implement something nicer

LOW: NUMRES is out of index: https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L570 Fixed

HIGH: as transactionId is easily can be manipulated this just converts to manually selected continentIdx for advanced users, which may be not good and may be its better provide an interface to choose continentIdx for everyone: https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L249 https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L252 even terrain can be generated manually: https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L261 For now we decided not to implement commit-reveal approach as it does not harm a lot, but will consider in future

LOW: strange constant 501: https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L250 hm... and 500: https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L362 Fixed

HIGH: whats the point for an user to get such result (health -> 0): https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L243 Negative health mean user should heal and repeat

HIGH: same line as above plus second transfer to economyAddr: https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L607 Fixed

MEDIUM: landAssetIdIgnored it is really ignored but why it is here: https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L619 https://github.com/waveslands/ride/blob/f7e1f0f5c69fc39e63cb3a51b1ff34842f262166/wlands_staking.ride#L625 It is not ignored anymore

waveslands / ride

Brief audit of `wlands_staking.ride` #2