Cally contest - 0x1f8b's results

The rederSvg method is vulnerable to json and XML injections, and could facilitate an XSS attack. Sanitize user inputs is mandatory because the user can inject into SVG's javascript logic.

This is one example of XSS inside a SVG

<svg xmlns="http://www.w3.org/2000/svg" width="300" height="300" style="background:#000">
   <script type="text/javascript">
      alert("Ghostlulz XSS");
   </script>
</svg>

Reference:

• http://ghostlulz.com/xss-svg/
• https://research.securitum.com/do-you-allow-to-load-svg-files-you-have-xss/

Source affected:

• https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/CallyNft.sol#L137-L145

Change important values without emitting an event

It is important to issue an event when features are changed that imply changes in the economics or ecosystem of the project, so that users and dapps can react accordingly.

Source affected:

• https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L120

Call initiateWithdraw when isExercised only will cost gas

Because is not possible to call withdraw when vault.isExercised == true, the method initiateWithdraw should check that isExercised == false

Emit the event before the modifications in order to save one variable.

• CallyNft.sol#L44

    function _forceTransfer(address to, uint256 id) internal {
        require(to != address(0), "INVALID_RECIPIENT");
        emit Transfer(_ownerOf[id], to, id);
        _ownerOf[id] = to;
        delete getApproved[id];

    }

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

LendTicket.sol:39:            balanceOf[from]--;
LendTicket.sol:41:            balanceOf[to]++;

I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Affected source:

• CallyNft.sol#L244

Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

Source affected:

• https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L250
• https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L245
• https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L188
• https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L333

Improve logic

Moving the line 208 to 216 it's possible to save gas under specific circumstances because the variable vault is not used before.

Reduce boolean checks

It's compared a boolean value using == false, instead of using the boolean value, or NOT opcode, it's cheaper to use NOT when the value it's false, or just the value without == true, when it's true, because it will use less opcode inside the VM.

• https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L217-L220
• https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L328