Contest entry (Smart Contract Cracking Competition)

[starlightduck]

Hello, this is my entry for the Smart Contract Cracking Competition.
There has been lots of work done on it, I will try to list it in chronological order, but this is not exhaustive list since I might have missed small fixes in list, or I still have some time to add them until contest ends.

• I have analyzed storage scheme, laid out comments for easier understanding (especially of unpack functions)
• Then I have created visual diagrams to display and understand the storage structure, I expected to use them for R/W analysis in future to try to find unprotected writes, but that turned out to be unnecessary;
• Shallow look at execution paths in code turned out to contain a problem in 3 execution paths, logic analysis resulted in that internal message getters can be moved higher to prevent 2 of them (and decrease gas usage), the last problematic execution path was mitigated by saving data if explicitly this path is followed;
• Checking out some smaller functions I have discovered that it is possible to create auction with arbitrary MsgAddress, despite that in TLB only MsgAddressInt is permitted as beneficiary_address, doing so can cause troubles when auction ends and contract attempts to send money to some MsgAddressExt, maybe even MsgAddressNone, mitigated with checking using parse_std_addr;
• In my opinion, due to naming (valid since, valid until) time checking in unwrap signed cmd should be less strict, this change is incorporated and suggested for review;
• Further analysis of small functions turned out that despite common sense and TLB royalty params are signed, changed them to uint16 from int16. That is not so critical because they are checked for > 0 when used, but this cuts off some precision (15 bits instead of 16) and is an error;
• I have noticed that there are some "magic numbers" such as error codes and moved them to constants. On the other hand there are some constants that are not used, and I have marked them as such, because that may mean coding problem or not yet implemented functionality;
• Now I have moved away from read-write functions and analyzed read-only (getters), laid out comments about their in/out structure and discovered (and fixed) a single logic error;
• Afterwards, I have performed manual execution and data flow analysis of root functions (for int and ext messages) and created corresponding diagrams. This was very difficult and time consuming, but resulted in discovering few more potential problems.
• One of the problems is that process new bid does send message carrying noticeable amount of value but does not update my balance in code, in one execution path (as basic as processing new bid in running auction) process new bid is called, and afterwards maybe end auction is called with unchanged value of my balance, that may result in inadequate behavior of maybe end auction in some situations. Fixed by correctly accounting for sent value and returning updated my balance.
• I could not find any more real issues at the moment, so I have decided to polish the code using the latest FunC features that I have invented and that have been accepted to the main repo. More precisely:
  • Assembler "# pushint" functions are not needed because integer constants exactly replicate usage of numeric literals in-place, therefore they can be supported anywhere and are even more optimal because they can participate in compiler optimizations (such as zero equality check) unlike asm "constants". Therefore I have replaced workchain and min tons for storage asm functions with corresponding integer constants;
  • I have decided to use wonderful #include keyword to include relevant stdlib and common files from the main contract files to make it easier to compile - just run compiler on the main file and there you go;
  • The constant "one_ton" was renamed to more expressive "tons", therefore it can be used as 1 * tons to make it very easy to read that you have meant one ton. Due to numeric literal optimizations this will not result in unoptimal multiplication. Another constant "days" was introduced to make easy to read comparisons with "7 * days" and "365 * days" as optimal as previous "60 * 60 * 24 * 7" and "60 * 60 * 24 * 365" contraptions;
  • I have replaced message flags with corresponding "mf::" constants, that allow to easily see what flags are used when sending message without thinking about numbers. Moreover, since logic operations between integer constants are now supported and precompiling even combinations of such flags can be easily used for more complex messages (not used in these contracts);
• Afterwards I have paid attention how forward fees are estimated and accounted for them in my balance calculation in process new bid method;
• Revisiting the fix for the first found bug (inpersistency if auction ends under some conditions) I have made it more prominent for code reader what is going on (by adding need_to_save_item_data constant) and even further optimize it by saving data only when needed - that is, only if auction is null after maybe end auction call;
• Last polishing of code for now that I have decided to do is make zero op-code more expressive by wrapping it into op::simple_transfer_0 that does not lose its numeric perception and separates zero opcode from many other zeros that are in code for easier understanding.

[oliveltours]

H

[starlightduck]

Merged main with new commits into contest branch, hope that is allowed, because otherwise it has conflicts.