approve() return value not checked

[code423n4]

Handle

sirhashalot

Vulnerability details

Impact

The approveToken() function in TreasuryManager.sol calls approve() on an ERC20 token, but does not check the return value of this call to confirm it completed as expected. Some tokens do not follow the ERC20 specifications as expected and do not revert if the approve failed, but checking if the function returns true can help to catch these edge cases.

This finding was identified in the last Notional contest as a medium risk, but I conservatively marked it as low risk.

Proof of Concept

Line 79 of TreasuryManager.sol is where approve() is called without any return value checks

Recommended Mitigation Steps

Check the return value of ERC20 operations to validate that they were successfully completed.

[jeffywu]

Duplicate #146

[pauliax]

approveToken is an external function only callable by the owner, so an owner can reset the approval first if necessary. I am marking this issue as a non-critical recommendation.

[pauliax]

Making this a primary issue as it contains the most concise description.