search

code-423n4 / 2021-09-swivel-findings

0 stars 0 forks source link

Prevent underflow in require #62

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

gpersoon

Vulnerability details

Impact

The contract Swivel.sol contains a few of the following requires: require(a <= (o.premium - filled[hash]), 'taker amount > available volume');

If "o.premium" happens to be smaller than "filled[hash]", a revert will occur at "o.premium - filled[hash]" and no error message will be displayed.

Also note the statements use slightly different syntax with the parentheses.

Proof of Concept

Swivel.sol: require(a <= (o.premium - filled[hash]), 'taker amount > available volume'); Swivel.sol: require((a <= o.principal - filled[hash]), 'taker amount > available volume'); // slightly different syntax Swivel.sol: require(a <= ((o.principal - filled[hash])), 'taker amount > available volume'); // slightly different syntax Swivel.sol: require(a <= (o.premium - filled[hash]), 'taker amount > available volume'); Swivel.sol: require(a <= (o.premium - filled[hash]), 'taker amount > available volume'); Swivel.sol: require(a <= (o.principal - filled[hash]), 'taker amount > available volume'); Swivel.sol: require(a <= (o.principal - filled[hash]), 'taker amount > available volume'); Swivel.sol: require(a <= (o.premium - filled[hash]), 'taker amount > available volume');

Tools Used

Recommended Mitigation Steps

replace require(a <= (o.xxxx - filled[hash]), 'taker amount > available volume');
with require( (a + filled[hash]) <= o.xxxx), 'taker amount > available volume');

Use the same parentheses structure everywhere.

JTraversa commented 3 years ago

This is really 2 issues.

  1. Potential underflow:
    • I believe this is not needed/should be disputed, given the require itself ensures that o.premium is always less than filled[hash] (filled[hash] = filled[hash] + a from a previous fill, where the require was true)
  2. Inconsistent syntax (non-critical):
    • Will correct.
0xean commented 3 years ago

downgrading both issues to non-critical, the warden suggestion is a better algorithm and should be implemented.

JTraversa commented 2 years ago

https://github.com/Swivel-Finance/gost/commit/92fde6c7b0697fb16bd82bd46022068d10cdf956