code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

If owner has given spender type(uint256).max allowance, which is very common, then the allowance does not get checked or reduced in transferFrom().

Impact: Once approved for type(uint256).max allowance, any spender contract can keep doing transferFroms without having its allowance checked/reduced ever. This breaks the fundamental approve-transferFrom constraint on allowances between owners and spenders. This incorrect conditional optimization (the comment notes: “// Max approval (saves an SSTORE)”) puts approved user funds at risk for all Pools, Synths and SPARTA tokens from malicious contracts.

Proof of Concept

https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Pool.sol#L107-L112

https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Synth.sol#L102-L107

https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/outside-scope/Sparta.sol#L103-L108

Typical transferFrom(): https://github.com/OpenZeppelin/openzeppelin-contracts/blob/3935b907d40c9a23b04b721c2f61758df1caf722/contracts/token/ERC20/ERC20.sol#L148-L162

Tools Used

Manual Analysis

Recommended Mitigation Steps

Remove conditional optimization to allow checking/reduction of allowance even if it is currently at uint256.max.

verifyfirst commented 3 years ago

By design max.approval was always the intention, we consider it low risk and a major UX helper, however we will adjust this based on consensus.

ghoul-sol commented 3 years ago

Keeping this as high risk as it's rather uncommon practice.